| Message ID | 20221206151207.8801-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: fix dma_buf_export init order | expand |
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Christian König >Sent: Tuesday, December 6, 2022 10:12 AM >To: quic_charante@quicinc.com; cuigaosheng1@huawei.com; >tjmercier@google.com; sumit.semwal@linaro.org >Cc: linaro-mm-sig@lists.linaro.org; dri-devel@lists.freedesktop.org; linux- >media@vger.kernel.org >Subject: [PATCH] dma-buf: fix dma_buf_export init order > >The init order and resulting error handling in dma_buf_export >was pretty messy. > >Subordinate objects like the file and the sysfs kernel objects >were initializing and wiring itself up with the object in the >wrong order resulting not only in complicating and partially >incorrect error handling, but also in publishing only halve >initialized DMA-buf objects. > >Clean this up thoughtfully by allocating the file independent >of the DMA-buf object. Then allocate and initialize the DMA-buf >object itself, before publishing it through sysfs. If everything >works as expected the file is then connected with the DMA-buf >object and publish it through debugfs. > >Also adds the missing dma_resv_fini() into the error handling. > >Signed-off-by: Christian König <christian.koenig@amd.com> >--- > drivers/dma-buf/dma-buf-sysfs-stats.c | 7 +-- > drivers/dma-buf/dma-buf-sysfs-stats.h | 4 +- > drivers/dma-buf/dma-buf.c | 65 +++++++++++++-------------- > 3 files changed, 34 insertions(+), 42 deletions(-) > >diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma- >buf-sysfs-stats.c >index 2bba0babcb62..4b680e10c15a 100644 >--- a/drivers/dma-buf/dma-buf-sysfs-stats.c >+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c >@@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void) > kset_unregister(dma_buf_stats_kset); > } > >-int dma_buf_stats_setup(struct dma_buf *dmabuf) >+int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) > { > struct dma_buf_sysfs_entry *sysfs_entry; > int ret; > >- if (!dmabuf || !dmabuf->file) >- return -EINVAL; >- > if (!dmabuf->exp_name) { > pr_err("exporter name must not be empty if stats >needed\n"); > return -EINVAL; >@@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > > /* create the directory for buffer stats */ > ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >NULL, >- "%lu", file_inode(dmabuf->file)->i_ino); >+ "%lu", file_inode(file)->i_ino); > if (ret) > goto err_sysfs_dmabuf; > >diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma- >buf-sysfs-stats.h >index a49c6e2650cc..7a8a995b75ba 100644 >--- a/drivers/dma-buf/dma-buf-sysfs-stats.h >+++ b/drivers/dma-buf/dma-buf-sysfs-stats.h >@@ -13,7 +13,7 @@ > int dma_buf_init_sysfs_statistics(void); > void dma_buf_uninit_sysfs_statistics(void); > >-int dma_buf_stats_setup(struct dma_buf *dmabuf); >+int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file); > > void dma_buf_stats_teardown(struct dma_buf *dmabuf); > #else >@@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void) > > static inline void dma_buf_uninit_sysfs_statistics(void) {} > >-static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) >+static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file >*file) > { > return 0; > } >diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >index e6f36c014c4c..ea08049b70ae 100644 >--- a/drivers/dma-buf/dma-buf.c >+++ b/drivers/dma-buf/dma-buf.c >@@ -614,19 +614,11 @@ struct dma_buf *dma_buf_export(const struct >dma_buf_export_info *exp_info) > size_t alloc_size = sizeof(struct dma_buf); > int ret; > >- if (!exp_info->resv) >- alloc_size += sizeof(struct dma_resv); >- else >- /* prevent &dma_buf[1] == dma_buf->resv */ >- alloc_size += 1; >- >- if (WARN_ON(!exp_info->priv >- || !exp_info->ops >- || !exp_info->ops->map_dma_buf >- || !exp_info->ops->unmap_dma_buf >- || !exp_info->ops->release)) { >+ if (WARN_ON(!exp_info->priv || !exp_info->ops >+ || !exp_info->ops->map_dma_buf >+ || !exp_info->ops->unmap_dma_buf >+ || !exp_info->ops->release)) > return ERR_PTR(-EINVAL); >- } > > if (WARN_ON(exp_info->ops->cache_sgt_mapping && > (exp_info->ops->pin || exp_info->ops->unpin))) >@@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct >dma_buf_export_info *exp_info) > if (!try_module_get(exp_info->owner)) > return ERR_PTR(-ENOENT); > >+ file = dma_buf_getfile(exp_info->size, exp_info->flags); Hi Christian, dma_buf_getfile takes a dmabuf, here you have a size? Did you change this function somewhere? with that addressed.... This cleanup makes sense to me. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> M >+ if (IS_ERR(file)) { >+ ret = PTR_ERR(file); >+ goto err_module; >+ } >+ >+ if (!exp_info->resv) >+ alloc_size += sizeof(struct dma_resv); >+ else >+ /* prevent &dma_buf[1] == dma_buf->resv */ >+ alloc_size += 1; > dmabuf = kzalloc(alloc_size, GFP_KERNEL); > if (!dmabuf) { > ret = -ENOMEM; >- goto err_module; >+ goto err_file; > } > > dmabuf->priv = exp_info->priv; >@@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct >dma_buf_export_info *exp_info) > init_waitqueue_head(&dmabuf->poll); > dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; > dmabuf->cb_in.active = dmabuf->cb_out.active = 0; >+ mutex_init(&dmabuf->lock); >+ INIT_LIST_HEAD(&dmabuf->attachments); > > if (!resv) { >- resv = (struct dma_resv *)&dmabuf[1]; >- dma_resv_init(resv); >+ dmabuf->resv = (struct dma_resv *)&dmabuf[1]; >+ dma_resv_init(dmabuf->resv); >+ } else { >+ dmabuf->resv = resv; > } >- dmabuf->resv = resv; > >- file = dma_buf_getfile(dmabuf, exp_info->flags); >- if (IS_ERR(file)) { >- ret = PTR_ERR(file); >+ ret = dma_buf_stats_setup(dmabuf, file); >+ if (ret) > goto err_dmabuf; >- } > >+ file->private_data = dmabuf; >+ file->f_path.dentry->d_fsdata = dmabuf; > dmabuf->file = file; > >- mutex_init(&dmabuf->lock); >- INIT_LIST_HEAD(&dmabuf->attachments); >- > mutex_lock(&db_list.lock); > list_add(&dmabuf->list_node, &db_list.head); > mutex_unlock(&db_list.lock); > >- ret = dma_buf_stats_setup(dmabuf); >- if (ret) >- goto err_sysfs; >- > return dmabuf; > >-err_sysfs: >- /* >- * Set file->f_path.dentry->d_fsdata to NULL so that when >- * dma_buf_release() gets invoked by dentry_ops, it exits >- * early before calling the release() dma_buf op. >- */ >- file->f_path.dentry->d_fsdata = NULL; >- fput(file); > err_dmabuf: >+ if (!resv) >+ dma_resv_fini(dmabuf->resv); > kfree(dmabuf); >+err_file: >+ fput(file); > err_module: > module_put(exp_info->owner); > return ERR_PTR(ret); >-- >2.34.1
Thanks Christian for this nice cleanup!! On 12/6/2022 8:42 PM, Christian König wrote: > @@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > if (!try_module_get(exp_info->owner)) > return ERR_PTR(-ENOENT); > > + file = dma_buf_getfile(exp_info->size, exp_info->flags); > + if (IS_ERR(file)) { > + ret = PTR_ERR(file); > + goto err_module; > + } > + > + if (!exp_info->resv) > + alloc_size += sizeof(struct dma_resv); > + else > + /* prevent &dma_buf[1] == dma_buf->resv */ > + alloc_size += 1; > dmabuf = kzalloc(alloc_size, GFP_KERNEL); > if (!dmabuf) { > ret = -ENOMEM; > - goto err_module; > + goto err_file; > } > > dmabuf->priv = exp_info->priv; > @@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > init_waitqueue_head(&dmabuf->poll); > dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; > dmabuf->cb_in.active = dmabuf->cb_out.active = 0; > + mutex_init(&dmabuf->lock); > + INIT_LIST_HEAD(&dmabuf->attachments); > > if (!resv) { > - resv = (struct dma_resv *)&dmabuf[1]; > - dma_resv_init(resv); > + dmabuf->resv = (struct dma_resv *)&dmabuf[1]; > + dma_resv_init(dmabuf->resv); > + } else { > + dmabuf->resv = resv; > } > - dmabuf->resv = resv; > > - file = dma_buf_getfile(dmabuf, exp_info->flags); > - if (IS_ERR(file)) { > - ret = PTR_ERR(file); > + ret = dma_buf_stats_setup(dmabuf, file); > + if (ret) > goto err_dmabuf; > - } > > + file->private_data = dmabuf; > + file->f_path.dentry->d_fsdata = dmabuf; > dmabuf->file = file; > > - mutex_init(&dmabuf->lock); > - INIT_LIST_HEAD(&dmabuf->attachments); > - > mutex_lock(&db_list.lock); > list_add(&dmabuf->list_node, &db_list.head); > mutex_unlock(&db_list.lock); > > - ret = dma_buf_stats_setup(dmabuf); > - if (ret) > - goto err_sysfs; > - > return dmabuf; > > -err_sysfs: > - /* > - * Set file->f_path.dentry->d_fsdata to NULL so that when > - * dma_buf_release() gets invoked by dentry_ops, it exits > - * early before calling the release() dma_buf op. > - */ > - file->f_path.dentry->d_fsdata = NULL; > - fput(file); > err_dmabuf: > + if (!resv) > + dma_resv_fini(dmabuf->resv); > kfree(dmabuf); > +err_file: > + fput(file); This fput() still endup in calling the dma_buf_file_release() where there are no checks before accessing the file->private_data(=dmabuf) which can result in null pointer access. Am I missing something trivial? > err_module: > module_put(exp_info->owner); > return ERR_PTR(ret); > -- 2.34.1
Am 06.12.22 um 17:20 schrieb Ruhl, Michael J: >> -----Original Message----- >> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >> Christian König >> Sent: Tuesday, December 6, 2022 10:12 AM >> To: quic_charante@quicinc.com; cuigaosheng1@huawei.com; >> tjmercier@google.com; sumit.semwal@linaro.org >> Cc: linaro-mm-sig@lists.linaro.org; dri-devel@lists.freedesktop.org; linux- >> media@vger.kernel.org >> Subject: [PATCH] dma-buf: fix dma_buf_export init order >> >> The init order and resulting error handling in dma_buf_export >> was pretty messy. >> >> Subordinate objects like the file and the sysfs kernel objects >> were initializing and wiring itself up with the object in the >> wrong order resulting not only in complicating and partially >> incorrect error handling, but also in publishing only halve >> initialized DMA-buf objects. >> >> Clean this up thoughtfully by allocating the file independent >> of the DMA-buf object. Then allocate and initialize the DMA-buf >> object itself, before publishing it through sysfs. If everything >> works as expected the file is then connected with the DMA-buf >> object and publish it through debugfs. >> >> Also adds the missing dma_resv_fini() into the error handling. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/dma-buf-sysfs-stats.c | 7 +-- >> drivers/dma-buf/dma-buf-sysfs-stats.h | 4 +- >> drivers/dma-buf/dma-buf.c | 65 +++++++++++++-------------- >> 3 files changed, 34 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma- >> buf-sysfs-stats.c >> index 2bba0babcb62..4b680e10c15a 100644 >> --- a/drivers/dma-buf/dma-buf-sysfs-stats.c >> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c >> @@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void) >> kset_unregister(dma_buf_stats_kset); >> } >> >> -int dma_buf_stats_setup(struct dma_buf *dmabuf) >> +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) >> { >> struct dma_buf_sysfs_entry *sysfs_entry; >> int ret; >> >> - if (!dmabuf || !dmabuf->file) >> - return -EINVAL; >> - >> if (!dmabuf->exp_name) { >> pr_err("exporter name must not be empty if stats >> needed\n"); >> return -EINVAL; >> @@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) >> >> /* create the directory for buffer stats */ >> ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, >> NULL, >> - "%lu", file_inode(dmabuf->file)->i_ino); >> + "%lu", file_inode(file)->i_ino); >> if (ret) >> goto err_sysfs_dmabuf; >> >> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma- >> buf-sysfs-stats.h >> index a49c6e2650cc..7a8a995b75ba 100644 >> --- a/drivers/dma-buf/dma-buf-sysfs-stats.h >> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h >> @@ -13,7 +13,7 @@ >> int dma_buf_init_sysfs_statistics(void); >> void dma_buf_uninit_sysfs_statistics(void); >> >> -int dma_buf_stats_setup(struct dma_buf *dmabuf); >> +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file); >> >> void dma_buf_stats_teardown(struct dma_buf *dmabuf); >> #else >> @@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void) >> >> static inline void dma_buf_uninit_sysfs_statistics(void) {} >> >> -static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) >> +static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file >> *file) >> { >> return 0; >> } >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index e6f36c014c4c..ea08049b70ae 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -614,19 +614,11 @@ struct dma_buf *dma_buf_export(const struct >> dma_buf_export_info *exp_info) >> size_t alloc_size = sizeof(struct dma_buf); >> int ret; >> >> - if (!exp_info->resv) >> - alloc_size += sizeof(struct dma_resv); >> - else >> - /* prevent &dma_buf[1] == dma_buf->resv */ >> - alloc_size += 1; >> - >> - if (WARN_ON(!exp_info->priv >> - || !exp_info->ops >> - || !exp_info->ops->map_dma_buf >> - || !exp_info->ops->unmap_dma_buf >> - || !exp_info->ops->release)) { >> + if (WARN_ON(!exp_info->priv || !exp_info->ops >> + || !exp_info->ops->map_dma_buf >> + || !exp_info->ops->unmap_dma_buf >> + || !exp_info->ops->release)) >> return ERR_PTR(-EINVAL); >> - } >> >> if (WARN_ON(exp_info->ops->cache_sgt_mapping && >> (exp_info->ops->pin || exp_info->ops->unpin))) >> @@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct >> dma_buf_export_info *exp_info) >> if (!try_module_get(exp_info->owner)) >> return ERR_PTR(-ENOENT); >> >> + file = dma_buf_getfile(exp_info->size, exp_info->flags); > Hi Christian, > > dma_buf_getfile takes a dmabuf, here you have a size? > > Did you change this function somewhere? Ups forgot to add that change to the patch. I shouldn't code when I'm in a hurry. Addressed this and Charans comment in v2. Thanks, Christian. > > with that addressed.... > > This cleanup makes sense to me. > > Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > > M > >> + if (IS_ERR(file)) { >> + ret = PTR_ERR(file); >> + goto err_module; >> + } >> + >> + if (!exp_info->resv) >> + alloc_size += sizeof(struct dma_resv); >> + else >> + /* prevent &dma_buf[1] == dma_buf->resv */ >> + alloc_size += 1; >> dmabuf = kzalloc(alloc_size, GFP_KERNEL); >> if (!dmabuf) { >> ret = -ENOMEM; >> - goto err_module; >> + goto err_file; >> } >> >> dmabuf->priv = exp_info->priv; >> @@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct >> dma_buf_export_info *exp_info) >> init_waitqueue_head(&dmabuf->poll); >> dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; >> dmabuf->cb_in.active = dmabuf->cb_out.active = 0; >> + mutex_init(&dmabuf->lock); >> + INIT_LIST_HEAD(&dmabuf->attachments); >> >> if (!resv) { >> - resv = (struct dma_resv *)&dmabuf[1]; >> - dma_resv_init(resv); >> + dmabuf->resv = (struct dma_resv *)&dmabuf[1]; >> + dma_resv_init(dmabuf->resv); >> + } else { >> + dmabuf->resv = resv; >> } >> - dmabuf->resv = resv; >> >> - file = dma_buf_getfile(dmabuf, exp_info->flags); >> - if (IS_ERR(file)) { >> - ret = PTR_ERR(file); >> + ret = dma_buf_stats_setup(dmabuf, file); >> + if (ret) >> goto err_dmabuf; >> - } >> >> + file->private_data = dmabuf; >> + file->f_path.dentry->d_fsdata = dmabuf; >> dmabuf->file = file; >> >> - mutex_init(&dmabuf->lock); >> - INIT_LIST_HEAD(&dmabuf->attachments); >> - >> mutex_lock(&db_list.lock); >> list_add(&dmabuf->list_node, &db_list.head); >> mutex_unlock(&db_list.lock); >> >> - ret = dma_buf_stats_setup(dmabuf); >> - if (ret) >> - goto err_sysfs; >> - >> return dmabuf; >> >> -err_sysfs: >> - /* >> - * Set file->f_path.dentry->d_fsdata to NULL so that when >> - * dma_buf_release() gets invoked by dentry_ops, it exits >> - * early before calling the release() dma_buf op. >> - */ >> - file->f_path.dentry->d_fsdata = NULL; >> - fput(file); >> err_dmabuf: >> + if (!resv) >> + dma_resv_fini(dmabuf->resv); >> kfree(dmabuf); >> +err_file: >> + fput(file); >> err_module: >> module_put(exp_info->owner); >> return ERR_PTR(ret); >> -- >> 2.34.1
On Tue, Dec 6, 2022 at 7:12 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > The init order and resulting error handling in dma_buf_export > was pretty messy. > > Subordinate objects like the file and the sysfs kernel objects > were initializing and wiring itself up with the object in the > wrong order resulting not only in complicating and partially > incorrect error handling, but also in publishing only halve > initialized DMA-buf objects. > > Clean this up thoughtfully by allocating the file independent > of the DMA-buf object. Then allocate and initialize the DMA-buf > object itself, before publishing it through sysfs. If everything > works as expected the file is then connected with the DMA-buf > object and publish it through debugfs. > > Also adds the missing dma_resv_fini() into the error handling. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-buf-sysfs-stats.c | 7 +-- > drivers/dma-buf/dma-buf-sysfs-stats.h | 4 +- > drivers/dma-buf/dma-buf.c | 65 +++++++++++++-------------- > 3 files changed, 34 insertions(+), 42 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > index 2bba0babcb62..4b680e10c15a 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void) > kset_unregister(dma_buf_stats_kset); > } > > -int dma_buf_stats_setup(struct dma_buf *dmabuf) > +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) > { > struct dma_buf_sysfs_entry *sysfs_entry; > int ret; > > - if (!dmabuf || !dmabuf->file) > - return -EINVAL; > - > if (!dmabuf->exp_name) { > pr_err("exporter name must not be empty if stats needed\n"); > return -EINVAL; > @@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > > /* create the directory for buffer stats */ > ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, > - "%lu", file_inode(dmabuf->file)->i_ino); > + "%lu", file_inode(file)->i_ino); > if (ret) > goto err_sysfs_dmabuf; > > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h > index a49c6e2650cc..7a8a995b75ba 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.h > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h > @@ -13,7 +13,7 @@ > int dma_buf_init_sysfs_statistics(void); > void dma_buf_uninit_sysfs_statistics(void); > > -int dma_buf_stats_setup(struct dma_buf *dmabuf); > +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file); > > void dma_buf_stats_teardown(struct dma_buf *dmabuf); > #else > @@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void) > > static inline void dma_buf_uninit_sysfs_statistics(void) {} > > -static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) > +static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) > { > return 0; > } > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e6f36c014c4c..ea08049b70ae 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -614,19 +614,11 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > size_t alloc_size = sizeof(struct dma_buf); > int ret; > > - if (!exp_info->resv) > - alloc_size += sizeof(struct dma_resv); > - else > - /* prevent &dma_buf[1] == dma_buf->resv */ > - alloc_size += 1; > - > - if (WARN_ON(!exp_info->priv > - || !exp_info->ops > - || !exp_info->ops->map_dma_buf > - || !exp_info->ops->unmap_dma_buf > - || !exp_info->ops->release)) { > + if (WARN_ON(!exp_info->priv || !exp_info->ops > + || !exp_info->ops->map_dma_buf > + || !exp_info->ops->unmap_dma_buf > + || !exp_info->ops->release)) > return ERR_PTR(-EINVAL); > - } > > if (WARN_ON(exp_info->ops->cache_sgt_mapping && > (exp_info->ops->pin || exp_info->ops->unpin))) > @@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > if (!try_module_get(exp_info->owner)) > return ERR_PTR(-ENOENT); > > + file = dma_buf_getfile(exp_info->size, exp_info->flags); > + if (IS_ERR(file)) { > + ret = PTR_ERR(file); > + goto err_module; > + } > + > + if (!exp_info->resv) > + alloc_size += sizeof(struct dma_resv); > + else > + /* prevent &dma_buf[1] == dma_buf->resv */ > + alloc_size += 1; > dmabuf = kzalloc(alloc_size, GFP_KERNEL); > if (!dmabuf) { > ret = -ENOMEM; > - goto err_module; > + goto err_file; > } > > dmabuf->priv = exp_info->priv; > @@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > init_waitqueue_head(&dmabuf->poll); > dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; > dmabuf->cb_in.active = dmabuf->cb_out.active = 0; > + mutex_init(&dmabuf->lock); > + INIT_LIST_HEAD(&dmabuf->attachments); > > if (!resv) { > - resv = (struct dma_resv *)&dmabuf[1]; > - dma_resv_init(resv); > + dmabuf->resv = (struct dma_resv *)&dmabuf[1]; > + dma_resv_init(dmabuf->resv); > + } else { > + dmabuf->resv = resv; > } I like this, but I think it'd be even better to remove the local struct dma_resv *resv variable since it's just a copy of exp_info->resv and doesn't get modified any longer. More typing but less to keep track of. > - dmabuf->resv = resv; > > - file = dma_buf_getfile(dmabuf, exp_info->flags); > - if (IS_ERR(file)) { > - ret = PTR_ERR(file); > + ret = dma_buf_stats_setup(dmabuf, file); > + if (ret) > goto err_dmabuf; > - } > > + file->private_data = dmabuf; > + file->f_path.dentry->d_fsdata = dmabuf; > dmabuf->file = file; > > - mutex_init(&dmabuf->lock); > - INIT_LIST_HEAD(&dmabuf->attachments); > - > mutex_lock(&db_list.lock); > list_add(&dmabuf->list_node, &db_list.head); > mutex_unlock(&db_list.lock); Unrelated: I'm kind of surprised we bother with this db_list when CONFIG_DEBUG_FS isn't defined. > > - ret = dma_buf_stats_setup(dmabuf); > - if (ret) > - goto err_sysfs; > - > return dmabuf; > > -err_sysfs: > - /* > - * Set file->f_path.dentry->d_fsdata to NULL so that when > - * dma_buf_release() gets invoked by dentry_ops, it exits > - * early before calling the release() dma_buf op. > - */ > - file->f_path.dentry->d_fsdata = NULL; > - fput(file); > err_dmabuf: > + if (!resv) > + dma_resv_fini(dmabuf->resv); > kfree(dmabuf); > +err_file: > + fput(file); > err_module: > module_put(exp_info->owner); > return ERR_PTR(ret); > -- > 2.34.1 >
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.1-rc8]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/dma-buf/dma-buf.c: In function 'dma_buf_export':
>> drivers/dma-buf/dma-buf.c:633:40: warning: passing argument 1 of 'dma_buf_getfile' makes pointer from integer without a cast [-Wint-conversion]
633 | file = dma_buf_getfile(exp_info->size, exp_info->flags);
| ~~~~~~~~^~~~~~
| |
| size_t {aka unsigned int}
drivers/dma-buf/dma-buf.c:526:53: note: expected 'struct dma_buf *' but argument is of type 'size_t' {aka 'unsigned int'}
526 | static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
| ~~~~~~~~~~~~~~~~^~~~~~
vim +/dma_buf_getfile +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
Hi Christian,
I love your patch! Yet something to improve:
[auto build test ERROR on v6.1-rc8]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: s390-randconfig-r044-20221206
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6e4cea55f0d1104408b26ac574566a0e4de48036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/dma-buf/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/dma-buf/dma-buf.c:633:25: error: incompatible integer to pointer conversion passing 'const size_t' (aka 'const unsigned long') to parameter of type 'struct dma_buf *' [-Wint-conversion]
file = dma_buf_getfile(exp_info->size, exp_info->flags);
^~~~~~~~~~~~~~
drivers/dma-buf/dma-buf.c:526:53: note: passing argument to parameter 'dmabuf' here
static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
^
12 warnings and 1 error generated.
vim +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.1-rc8]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: x86_64-randconfig-a016
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/dma-buf/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/dma-buf/dma-buf.c:633:25: warning: incompatible integer to pointer conversion passing 'const size_t' (aka 'const unsigned long') to parameter of type 'struct dma_buf *' [-Wint-conversion]
file = dma_buf_getfile(exp_info->size, exp_info->flags);
^~~~~~~~~~~~~~
drivers/dma-buf/dma-buf.c:526:53: note: passing argument to parameter 'dmabuf' here
static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
^
1 warning generated.
vim +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
Hi Christian,
I love your patch! Yet something to improve:
[auto build test ERROR on v6.1-rc8]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: hexagon-randconfig-r022-20221206
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6e4cea55f0d1104408b26ac574566a0e4de48036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/dma-buf/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/dma-buf/dma-buf.c:16:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/dma-buf/dma-buf.c:633:25: error: incompatible integer to pointer conversion passing 'const size_t' (aka 'const unsigned int') to parameter of type 'struct dma_buf *' [-Wint-conversion]
file = dma_buf_getfile(exp_info->size, exp_info->flags);
^~~~~~~~~~~~~~
drivers/dma-buf/dma-buf.c:526:53: note: passing argument to parameter 'dmabuf' here
static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
^
6 warnings and 1 error generated.
vim +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.1-rc8]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: arm64-randconfig-s051-20221206
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/dma-buf/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/dma-buf/dma-buf.c:633:40: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected struct dma_buf *dmabuf @@ got unsigned long const [usertype] size @@
drivers/dma-buf/dma-buf.c:633:40: sparse: expected struct dma_buf *dmabuf
drivers/dma-buf/dma-buf.c:633:40: sparse: got unsigned long const [usertype] size
vim +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.1-rc8]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
patch link: https://lore.kernel.org/r/20221206151207.8801-1-christian.koenig%40amd.com
patch subject: [PATCH] dma-buf: fix dma_buf_export init order
config: i386-randconfig-s001
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/dma-buf-fix-dma_buf_export-init-order/20221206-231311
git checkout aa1f28d40a4dc78408327baefb83ee8fd34b6ef9
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/dma-buf/dma-buf.c:633:40: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected struct dma_buf *dmabuf @@ got unsigned int const [usertype] size @@
drivers/dma-buf/dma-buf.c:633:40: sparse: expected struct dma_buf *dmabuf
drivers/dma-buf/dma-buf.c:633:40: sparse: got unsigned int const [usertype] size
vim +633 drivers/dma-buf/dma-buf.c
559
560 /**
561 * DOC: dma buf device access
562 *
563 * For device DMA access to a shared DMA buffer the usual sequence of operations
564 * is fairly simple:
565 *
566 * 1. The exporter defines his exporter instance using
567 * DEFINE_DMA_BUF_EXPORT_INFO() and calls dma_buf_export() to wrap a private
568 * buffer object into a &dma_buf. It then exports that &dma_buf to userspace
569 * as a file descriptor by calling dma_buf_fd().
570 *
571 * 2. Userspace passes this file-descriptors to all drivers it wants this buffer
572 * to share with: First the file descriptor is converted to a &dma_buf using
573 * dma_buf_get(). Then the buffer is attached to the device using
574 * dma_buf_attach().
575 *
576 * Up to this stage the exporter is still free to migrate or reallocate the
577 * backing storage.
578 *
579 * 3. Once the buffer is attached to all devices userspace can initiate DMA
580 * access to the shared buffer. In the kernel this is done by calling
581 * dma_buf_map_attachment() and dma_buf_unmap_attachment().
582 *
583 * 4. Once a driver is done with a shared buffer it needs to call
584 * dma_buf_detach() (after cleaning up any mappings) and then release the
585 * reference acquired with dma_buf_get() by calling dma_buf_put().
586 *
587 * For the detailed semantics exporters are expected to implement see
588 * &dma_buf_ops.
589 */
590
591 /**
592 * dma_buf_export - Creates a new dma_buf, and associates an anon file
593 * with this buffer, so it can be exported.
594 * Also connect the allocator specific data and ops to the buffer.
595 * Additionally, provide a name string for exporter; useful in debugging.
596 *
597 * @exp_info: [in] holds all the export related information provided
598 * by the exporter. see &struct dma_buf_export_info
599 * for further details.
600 *
601 * Returns, on success, a newly created struct dma_buf object, which wraps the
602 * supplied private data and operations for struct dma_buf_ops. On either
603 * missing ops, or error in allocating struct dma_buf, will return negative
604 * error.
605 *
606 * For most cases the easiest way to create @exp_info is through the
607 * %DEFINE_DMA_BUF_EXPORT_INFO macro.
608 */
609 struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
610 {
611 struct dma_buf *dmabuf;
612 struct dma_resv *resv = exp_info->resv;
613 struct file *file;
614 size_t alloc_size = sizeof(struct dma_buf);
615 int ret;
616
617 if (WARN_ON(!exp_info->priv || !exp_info->ops
618 || !exp_info->ops->map_dma_buf
619 || !exp_info->ops->unmap_dma_buf
620 || !exp_info->ops->release))
621 return ERR_PTR(-EINVAL);
622
623 if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
624 (exp_info->ops->pin || exp_info->ops->unpin)))
625 return ERR_PTR(-EINVAL);
626
627 if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin))
628 return ERR_PTR(-EINVAL);
629
630 if (!try_module_get(exp_info->owner))
631 return ERR_PTR(-ENOENT);
632
> 633 file = dma_buf_getfile(exp_info->size, exp_info->flags);
634 if (IS_ERR(file)) {
635 ret = PTR_ERR(file);
636 goto err_module;
637 }
638
639 if (!exp_info->resv)
640 alloc_size += sizeof(struct dma_resv);
641 else
642 /* prevent &dma_buf[1] == dma_buf->resv */
643 alloc_size += 1;
644 dmabuf = kzalloc(alloc_size, GFP_KERNEL);
645 if (!dmabuf) {
646 ret = -ENOMEM;
647 goto err_file;
648 }
649
650 dmabuf->priv = exp_info->priv;
651 dmabuf->ops = exp_info->ops;
652 dmabuf->size = exp_info->size;
653 dmabuf->exp_name = exp_info->exp_name;
654 dmabuf->owner = exp_info->owner;
655 spin_lock_init(&dmabuf->name_lock);
656 init_waitqueue_head(&dmabuf->poll);
657 dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
658 dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
659 mutex_init(&dmabuf->lock);
660 INIT_LIST_HEAD(&dmabuf->attachments);
661
662 if (!resv) {
663 dmabuf->resv = (struct dma_resv *)&dmabuf[1];
664 dma_resv_init(dmabuf->resv);
665 } else {
666 dmabuf->resv = resv;
667 }
668
669 ret = dma_buf_stats_setup(dmabuf, file);
670 if (ret)
671 goto err_dmabuf;
672
673 file->private_data = dmabuf;
674 file->f_path.dentry->d_fsdata = dmabuf;
675 dmabuf->file = file;
676
677 mutex_lock(&db_list.lock);
678 list_add(&dmabuf->list_node, &db_list.head);
679 mutex_unlock(&db_list.lock);
680
681 return dmabuf;
682
683 err_dmabuf:
684 if (!resv)
685 dma_resv_fini(dmabuf->resv);
686 kfree(dmabuf);
687 err_file:
688 fput(file);
689 err_module:
690 module_put(exp_info->owner);
691 return ERR_PTR(ret);
692 }
693 EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF);
694
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c index 2bba0babcb62..4b680e10c15a 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.c +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void) kset_unregister(dma_buf_stats_kset); } -int dma_buf_stats_setup(struct dma_buf *dmabuf) +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) { struct dma_buf_sysfs_entry *sysfs_entry; int ret; - if (!dmabuf || !dmabuf->file) - return -EINVAL; - if (!dmabuf->exp_name) { pr_err("exporter name must not be empty if stats needed\n"); return -EINVAL; @@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) /* create the directory for buffer stats */ ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, - "%lu", file_inode(dmabuf->file)->i_ino); + "%lu", file_inode(file)->i_ino); if (ret) goto err_sysfs_dmabuf; diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h index a49c6e2650cc..7a8a995b75ba 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.h +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -13,7 +13,7 @@ int dma_buf_init_sysfs_statistics(void); void dma_buf_uninit_sysfs_statistics(void); -int dma_buf_stats_setup(struct dma_buf *dmabuf); +int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file); void dma_buf_stats_teardown(struct dma_buf *dmabuf); #else @@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void) static inline void dma_buf_uninit_sysfs_statistics(void) {} -static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) +static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file) { return 0; } diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index e6f36c014c4c..ea08049b70ae 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -614,19 +614,11 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) size_t alloc_size = sizeof(struct dma_buf); int ret; - if (!exp_info->resv) - alloc_size += sizeof(struct dma_resv); - else - /* prevent &dma_buf[1] == dma_buf->resv */ - alloc_size += 1; - - if (WARN_ON(!exp_info->priv - || !exp_info->ops - || !exp_info->ops->map_dma_buf - || !exp_info->ops->unmap_dma_buf - || !exp_info->ops->release)) { + if (WARN_ON(!exp_info->priv || !exp_info->ops + || !exp_info->ops->map_dma_buf + || !exp_info->ops->unmap_dma_buf + || !exp_info->ops->release)) return ERR_PTR(-EINVAL); - } if (WARN_ON(exp_info->ops->cache_sgt_mapping && (exp_info->ops->pin || exp_info->ops->unpin))) @@ -638,10 +630,21 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) if (!try_module_get(exp_info->owner)) return ERR_PTR(-ENOENT); + file = dma_buf_getfile(exp_info->size, exp_info->flags); + if (IS_ERR(file)) { + ret = PTR_ERR(file); + goto err_module; + } + + if (!exp_info->resv) + alloc_size += sizeof(struct dma_resv); + else + /* prevent &dma_buf[1] == dma_buf->resv */ + alloc_size += 1; dmabuf = kzalloc(alloc_size, GFP_KERNEL); if (!dmabuf) { ret = -ENOMEM; - goto err_module; + goto err_file; } dmabuf->priv = exp_info->priv; @@ -653,44 +656,36 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) init_waitqueue_head(&dmabuf->poll); dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; dmabuf->cb_in.active = dmabuf->cb_out.active = 0; + mutex_init(&dmabuf->lock); + INIT_LIST_HEAD(&dmabuf->attachments); if (!resv) { - resv = (struct dma_resv *)&dmabuf[1]; - dma_resv_init(resv); + dmabuf->resv = (struct dma_resv *)&dmabuf[1]; + dma_resv_init(dmabuf->resv); + } else { + dmabuf->resv = resv; } - dmabuf->resv = resv; - file = dma_buf_getfile(dmabuf, exp_info->flags); - if (IS_ERR(file)) { - ret = PTR_ERR(file); + ret = dma_buf_stats_setup(dmabuf, file); + if (ret) goto err_dmabuf; - } + file->private_data = dmabuf; + file->f_path.dentry->d_fsdata = dmabuf; dmabuf->file = file; - mutex_init(&dmabuf->lock); - INIT_LIST_HEAD(&dmabuf->attachments); - mutex_lock(&db_list.lock); list_add(&dmabuf->list_node, &db_list.head); mutex_unlock(&db_list.lock); - ret = dma_buf_stats_setup(dmabuf); - if (ret) - goto err_sysfs; - return dmabuf; -err_sysfs: - /* - * Set file->f_path.dentry->d_fsdata to NULL so that when - * dma_buf_release() gets invoked by dentry_ops, it exits - * early before calling the release() dma_buf op. - */ - file->f_path.dentry->d_fsdata = NULL; - fput(file); err_dmabuf: + if (!resv) + dma_resv_fini(dmabuf->resv); kfree(dmabuf); +err_file: + fput(file); err_module: module_put(exp_info->owner); return ERR_PTR(ret);
The init order and resulting error handling in dma_buf_export was pretty messy. Subordinate objects like the file and the sysfs kernel objects were initializing and wiring itself up with the object in the wrong order resulting not only in complicating and partially incorrect error handling, but also in publishing only halve initialized DMA-buf objects. Clean this up thoughtfully by allocating the file independent of the DMA-buf object. Then allocate and initialize the DMA-buf object itself, before publishing it through sysfs. If everything works as expected the file is then connected with the DMA-buf object and publish it through debugfs. Also adds the missing dma_resv_fini() into the error handling. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-buf-sysfs-stats.c | 7 +-- drivers/dma-buf/dma-buf-sysfs-stats.h | 4 +- drivers/dma-buf/dma-buf.c | 65 +++++++++++++-------------- 3 files changed, 34 insertions(+), 42 deletions(-)