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

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

 



On Tue, Dec 6, 2022 at 7:12 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> 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@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             | 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
>




[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