Re: [PATCH] dma-buf: fix dma_buf_export init order v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 9, 2022 at 8:29 AM Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> wrote:
>
> >-----Original Message-----
> >From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> >Christian König
> >Sent: Friday, December 9, 2022 2:16 AM
> >To: quic_charante@xxxxxxxxxxx; cuigaosheng1@xxxxxxxxxx;
> >sumit.semwal@xxxxxxxxxx
> >Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> >media@xxxxxxxxxxxxxxx
> >Subject: [PATCH] dma-buf: fix dma_buf_export init order v2
> >
> >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.
> >
> >v2: add some missing changes to dma_bug_getfile() and a missing NULL
> >    check in dma_buf_file_release()
>
> Looks good.
>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
>
> Mike
>
+1

Reviewed-by: T.J. Mercier <tjmercier@xxxxxxxxxx>

> >Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> >---
> > drivers/dma-buf/dma-buf-sysfs-stats.c |  7 +--
> > drivers/dma-buf/dma-buf-sysfs-stats.h |  4 +-
> > drivers/dma-buf/dma-buf.c             | 84 +++++++++++++--------------
> > 3 files changed, 43 insertions(+), 52 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..eb6b59363c4f 100644
> >--- a/drivers/dma-buf/dma-buf.c
> >+++ b/drivers/dma-buf/dma-buf.c
> >@@ -95,10 +95,11 @@ static int dma_buf_file_release(struct inode *inode,
> >struct file *file)
> >               return -EINVAL;
> >
> >       dmabuf = file->private_data;
> >-
> >-      mutex_lock(&db_list.lock);
> >-      list_del(&dmabuf->list_node);
> >-      mutex_unlock(&db_list.lock);
> >+      if (dmabuf) {
> >+              mutex_lock(&db_list.lock);
> >+              list_del(&dmabuf->list_node);
> >+              mutex_unlock(&db_list.lock);
> >+      }
> >
> >       return 0;
> > }
> >@@ -523,17 +524,17 @@ static inline int is_dma_buf_file(struct file *file)
> >       return file->f_op == &dma_buf_fops;
> > }
> >
> >-static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
> >+static struct file *dma_buf_getfile(size_t size, int flags)
> > {
> >       static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
> >-      struct file *file;
> >       struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
> >+      struct file *file;
> >
> >       if (IS_ERR(inode))
> >               return ERR_CAST(inode);
> >
> >-      inode->i_size = dmabuf->size;
> >-      inode_set_bytes(inode, dmabuf->size);
> >+      inode->i_size = size;
> >+      inode_set_bytes(inode, size);
> >
> >       /*
> >        * The ->i_ino acquired from get_next_ino() is not unique thus
> >@@ -547,8 +548,6 @@ static struct file *dma_buf_getfile(struct dma_buf
> >*dmabuf, int flags)
> >                                flags, &dma_buf_fops);
> >       if (IS_ERR(file))
> >               goto err_alloc_file;
> >-      file->private_data = dmabuf;
> >-      file->f_path.dentry->d_fsdata = dmabuf;
> >
> >       return file;
> >
> >@@ -614,19 +613,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 +629,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 +655,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
>




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux