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

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

 



On Tue, Dec 13, 2022 at 07:17:41PM +0100, Christian König wrote:
> Am 13.12.22 um 18:09 schrieb Daniel Vetter:
> > On Sat, Dec 10, 2022 at 08:43:47AM +0530, Sumit Semwal wrote:
> > > Hi Christian,
> > > 
> > > On Fri, 9 Dec 2022 at 12:45, 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.
> > > > 
> > > > v2: add some missing changes to dma_bug_getfile() and a missing NULL
> > > >      check in dma_buf_file_release()
> > > > 
> > > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > Thank you for this nice cleanup.
> > > Acked-by: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> > For reasons that aren't clear to me this ended up in drm-misc-fixes, but
> > doesn't have a Fixes: or cc: stable tag, and because it missed the merge
> > window it now wont go into any stable releases.
> 
> Mhm, crap I was hoping I would get that in before the merge window.

Well even then, if it's for -fixes it should have had cc: stable. Or
neither of those things.

> Should I push it to drm-misc-next  with a CC: stable tag then?
> 
> It's more of a theoretical issues if we run into problems during sysfs
> creation.

Nah, it's really not a practical bugfix imo, so neither -fixes nor stable
material imo.

> > So either this isn't a fix and should have landed in drm-misc-next or so,
> > or it was, and then it should have had Fixes: line (and cc: stable most
> > likely too).
> > 
> > I spotted this becaue it caused a conflict in drm-tip that was left behind
> > unresovled. I fixed it up.
> 
> Well then there is something wrong with drm-tip, cause I've fixed that up
> before.

This can happen if you have a massively mismatched git version compared to
everyone else. git tends to change it's rr format occasionally :-/ Modern
distro instead of lts works best so everyone has roughly the same git.
-Daniel

> 
> Regards,
> Christian.
> 
> > 
> > Tsk :-)
> > -Daniel
> > 
> > > Best,
> > > Sumit.
> > > > ---
> > > >   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
> > > > 
> > > 
> > > -- 
> > > Thanks and regards,
> > > 
> > > Sumit Semwal (he / him)
> > > Tech Lead - LCG, Vertical Technologies
> > > Linaro.org │ Open source software for ARM SoCs
> > > _______________________________________________
> > > Linaro-mm-sig mailing list -- linaro-mm-sig@xxxxxxxxxxxxxxxx
> > > To unsubscribe send an email to linaro-mm-sig-leave@xxxxxxxxxxxxxxxx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux