Linaro-mm-sig July 2021

linaro-mm-sig@lists.linaro.org

17 participants
54 discussions

Re: [Linaro-mm-sig] [PATCH] dma_buf: remove dmabuf sysfs teardown before release/detach

by Christian König

Am 19.07.21 um 07:19 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Dmabuf sysfs stat is used for dmabuf info track. > but these file maybe still use after buffer release/detach, > should clear it before buffer release/detach. Please rebase on current drm-misc-next. The attachment sysfs files have been removed in the meantime. Thanks, Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 510b42771974..9fa4620bd4bb 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,12 +76,12 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - dma_buf_stats_teardown(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > @@ -875,10 +875,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(dmabuf->resv, NULL); > list_del(&attach->node); > dma_resv_unlock(dmabuf->resv); > + > + dma_buf_attach_stats_teardown(attach); > if (dmabuf->ops->detach) > dmabuf->ops->detach(dmabuf, attach); > > - dma_buf_attach_stats_teardown(attach); > kfree(attach); > } > EXPORT_SYMBOL_GPL(dma_buf_detach);

3 years, 5 months

Re: [Linaro-mm-sig] [PATCH] dma_buf: remove dmabuf sysfs teardown before release/detach

by Christian König

Hi Guangming, Am 19.07.21 um 07:19 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Dmabuf sysfs stat is used for dmabuf info track. > but these file maybe still use after buffer release/detach, > should clear it before buffer release/detach. In general looks correct to me, but Hridya already send out a patch to partially revert the attachment sysfs files since they caused a bunch of more problems for some users. Please wait for that to land in branch drm-misc-next and then rebase yours on top of it. I will give you a ping when that is done. Thanks, Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 510b42771974..9fa4620bd4bb 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,12 +76,12 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - dma_buf_stats_teardown(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > @@ -875,10 +875,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(dmabuf->resv, NULL); > list_del(&attach->node); > dma_resv_unlock(dmabuf->resv); > + > + dma_buf_attach_stats_teardown(attach); > if (dmabuf->ops->detach) > dmabuf->ops->detach(dmabuf, attach); > > - dma_buf_attach_stats_teardown(attach); > kfree(attach); > } > EXPORT_SYMBOL_GPL(dma_buf_detach);

3 years, 5 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: support users to change dma_buf.name

by Christian König

Am 14.07.21 um 14:29 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > User space user can call DMA_BUF_SET_NAME to set dma_buf.name, > also add a kernel api for users to do same thing at kernel side. Well if you want to add a kernel API to set the name you also need to provide an user for this. Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 28 ++++++++++++++++++++++------ > include/linux/dma-buf.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..949af232c644 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -331,20 +331,20 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > * purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > - * @buf: [in] A piece of userspace memory that contains the name of > + * @buf: [in] A piece of memory that contains the name of > * the dma-buf. > * > * Returns 0 on success. If the dma-buf buffer is already attached to > * devices, return -EBUSY. > * > */ > -static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > +long dma_buf_set_name(struct dma_buf *dmabuf, const char *buf) > { > - char *name = strndup_user(buf, DMA_BUF_NAME_LEN); > + char *name = kstrndup(buf, DMA_BUF_NAME_LEN, GFP_KERNEL); > long ret = 0; > > - if (IS_ERR(name)) > - return PTR_ERR(name); > + if (!name) > + return -ENOMEM; > > dma_resv_lock(dmabuf->resv, NULL); > if (!list_empty(&dmabuf->attachments)) { > @@ -361,6 +361,22 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > dma_resv_unlock(dmabuf->resv); > return ret; > } > +EXPORT_SYMBOL_GPL(dma_buf_set_name); > + > +static long > +dma_buf_set_name_user(struct dma_buf *dmabuf, const char __user *buf) > +{ > + char *name = strndup_user(buf, DMA_BUF_NAME_LEN); > + long ret = 0; > + > + if (IS_ERR(name)) > + return PTR_ERR(name); > + > + ret = dma_buf_set_name(dmabuf, name); > + kfree(name); > + > + return ret; > +} > > static long dma_buf_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > @@ -403,7 +419,7 @@ static long dma_buf_ioctl(struct file *file, > > case DMA_BUF_SET_NAME_A: > case DMA_BUF_SET_NAME_B: > - return dma_buf_set_name(dmabuf, (const char __user *)arg); > + return dma_buf_set_name_user(dmabuf, (const char __user *)arg); > > default: > return -ENOTTY; > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..e6612ab59a59 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > +long dma_buf_set_name(struct dma_buf *dmabuf, const char *name); > #endif /* __DMA_BUF_H__ */

3 years, 5 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: add kernel count for dma_buf

by Christian König

Am 14.07.21 um 14:03 schrieb guangming.cao(a)mediatek.com: > From: Guangming.Cao <guangming.cao(a)mediatek.com> > > On Wed, 2021-07-14 at 12:43 +0200, Christian K鰊ig wrote: >> Am 14.07.21 um 11:44 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> On Wed, 2021-07-14 at 10:46 +0200, Christian K鰊ig wrote: >>>> Am 14.07.21 um 09:11 schrieb guangming.cao(a)mediatek.com: >>>>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>>>> >>>>> Add a refcount for kernel to prevent UAF(Use After Free) issue. >>>> Well NAK on so many levels. >>>> >>>>> We can assume a case like below: >>>>> 1. kernel space alloc dma_buf(file count = 1) >>>>> 2. kernel use dma_buf to get fd(file count = 1) >>>>> 3. userspace use fd to do mapping (file count = 2) >>>> Creating an userspace mapping increases the reference count for >>>> the >>>> underlying file object. >>>> >>>> See the implementation of mmap_region(): >>>> ... >>>> vma->vm_file = get_file(file); >>>> error = call_mmap(file, vma); >>>> ... >>>> >>>> What can happen is the the underlying exporter redirects the mmap >>>> to >>>> a >>>> different file, e.g. TTM or GEM drivers do that all the time. >>>> >>>> But this is fine since then the VA mapping is independent of the >>>> DMA- >>>> buf. >>>> >>>>> 4. kernel call dma_buf_put (file count = 1) >>>>> 5. userpsace close buffer fd(file count = 0) >>>>> 6. at this time, buffer is released, but va is valid!! >>>>> So we still can read/write buffer via mmap va, >>>>> it maybe cause memory leak, or kernel exception. >>>>> And also, if we use "ls -ll" to watch corresponding >>>>> process >>>>> fd link info, it also will cause kernel exception. >>>>> >>>>> Another case: >>>>> Using dma_buf_fd to generate more than 1 fd, because >>>>> dma_buf_fd will not increase file count, thus, when >>>>> close >>>>> the second fd, it maybe occurs error. >>>> Each opened fd will increase the reference count so this is >>>> certainly >>>> not correct what you describe here. >>>> >>>> Regards, >>>> Christian. >>>> >>> Yes, mmap will increase file count by calling get_file, so step[2] >>> -> >>> step[3], file count increase 1. >>> >>> But, dma_buf_fd() will not increase file count. >>> function "dma_buf_fd(struct dma_buf *dmabuf, int flags)" just get >>> an >>> unused fd, via call "get_unused_fd_flags(flags)", and call >>> "fd_install(fd, dmabuf->file)", it will let associated "struct >>> file*" >>> in task's fdt->fd[fd] points to this dma_buf.file, not increase the >>> file count of dma_buf.file. >>> I think this is confusing, I can get more than 1 fds via >>> dma_buf_fd, >>> but they don't need to close it because they don't increase file >>> count. >>> >>> However, dma_buf_put() can decrease file count at kernel side >>> directly. >>> If somebody write a ko to put file count of dma_buf.file many >>> times, it >>> will cause buffer freed earlier than except. At last on Android, I >>> think this is a little bit dangerous. >> dma_buf_fd() takes the dma_buf pointer and converts it into a fd. So >> the >> reference is consumed. >> >> That's why users of this interface make sure to get a separate >> reference, see drm_gem_prime_handle_to_fd() for example: >> >> ... >> out_have_handle: >> ret = dma_buf_fd(dmabuf, flags); >> /* >> * We must _not_ remove the buffer from the handle cache since >> the >> newly >> * created dma buf is already linked in the global obj->dma_buf >> pointer, >> * and that is invariant as long as a userspace gem handle >> exists. >> * Closing the handle will clean out the cache anyway, so we >> don't >> leak. >> */ >> if (ret < 0) { >> goto fail_put_dmabuf; >> } else { >> *prime_fd = ret; >> ret = 0; >> } >> >> goto out; >> >> fail_put_dmabuf: >> dma_buf_put(dmabuf); >> out: >> ... >> >> You could submit a patch to improve the documentation and explicitly >> note on dma_buf_fd() that the reference is consumed, but all of this >> is >> working perfectly fine. >> >> Regards, >> Christian. >> > Thanks for your reply! > > Yes, drm works fine because it fully understand what dma-buf api will > do. Improve the documentation is really good idea to prevent this case. > > But, what I can't understand is, for kernel api exported to > corresponding users, we don't need to ensure all api is safe? Well the API is perfectly safe, it is just not what you are expecting. > And for general cases, dma-buf framework also need to prevent this > case, isn't it, it will make dma-buf framework more strong? What we could do is to move getting the reference into that function if all users of that function does that anyway. This would then be more defensive because new users of dma_buf_fd() can't forget to grab a reference. But this needs a complete audit of the kernel with all of the users of dma_buf_fd(). Regards, Christian. > > > BRs! > Guangming >>>>> Solution: >>>>> Add a kernel count for dma_buf, and make sure the file >>>>> count >>>>> of dma_buf.file hold by kernel is 1. >>>>> >>>>> Notes: For this solution, kref couldn't work because kernel ref >>>>> maybe added from 0, but kref don't allow it. >>>>> >>>>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >>>>> --- >>>>> drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++---- >>>>> include/linux/dma-buf.h | 6 ++++-- >>>>> 2 files changed, 23 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma- >>>>> buf.c >>>>> index 511fe0d217a0..04ee92aac8b9 100644 >>>>> --- a/drivers/dma-buf/dma-buf.c >>>>> +++ b/drivers/dma-buf/dma-buf.c >>>>> @@ -62,6 +62,7 @@ static void dma_buf_release(struct dentry >>>>> *dentry) >>>>> if (unlikely(!dmabuf)) >>>>> return; >>>>> >>>>> + WARN_ON(atomic64_read(&dmabuf->kernel_ref)); >>>>> BUG_ON(dmabuf->vmapping_counter); >>>>> >>>>> /* >>>>> @@ -555,6 +556,7 @@ struct dma_buf *dma_buf_export(const struct >>>>> dma_buf_export_info *exp_info) >>>>> goto err_module; >>>>> } >>>>> >>>>> + atomic64_set(&dmabuf->kernel_ref, 1); >>>>> dmabuf->priv = exp_info->priv; >>>>> dmabuf->ops = exp_info->ops; >>>>> dmabuf->size = exp_info->size; >>>>> @@ -617,6 +619,9 @@ int dma_buf_fd(struct dma_buf *dmabuf, int >>>>> flags) >>>>> >>>>> fd_install(fd, dmabuf->file); >>>>> >>>>> + /* Add file cnt for each new fd */ >>>>> + get_file(dmabuf->file); >>>>> + >>>>> return fd; >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_fd); >>>>> @@ -626,12 +631,13 @@ EXPORT_SYMBOL_GPL(dma_buf_fd); >>>>> * @fd: [in] fd associated with the struct dma_buf to >>>>> be >>>>> returned >>>>> * >>>>> * On success, returns the struct dma_buf associated with an >>>>> fd; >>>>> uses >>>>> - * file's refcounting done by fget to increase refcount. >>>>> returns >>>>> ERR_PTR >>>>> - * otherwise. >>>>> + * dmabuf's ref refcounting done by kref_get to increase >>>>> refcount. >>>>> + * Returns ERR_PTR otherwise. >>>>> */ >>>>> struct dma_buf *dma_buf_get(int fd) >>>>> { >>>>> struct file *file; >>>>> + struct dma_buf *dmabuf; >>>>> >>>>> file = fget(fd); >>>>> >>>>> @@ -643,7 +649,12 @@ struct dma_buf *dma_buf_get(int fd) >>>>> return ERR_PTR(-EINVAL); >>>>> } >>>>> >>>>> - return file->private_data; >>>>> + dmabuf = file->private_data; >>>>> + /* replace file count increase as ref increase for kernel >>>>> user >>>>> */ >>>>> + get_dma_buf(dmabuf); >>>>> + fput(file); >>>>> + >>>>> + return dmabuf; >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_get); >>>>> >>>>> @@ -662,7 +673,11 @@ void dma_buf_put(struct dma_buf *dmabuf) >>>>> if (WARN_ON(!dmabuf || !dmabuf->file)) >>>>> return; >>>>> >>>>> - fput(dmabuf->file); >>>>> + if (WARN_ON(!atomic64_read(&dmabuf->kernel_ref))) >>>>> + return; >>>>> + >>>>> + if (!atomic64_dec_return(&dmabuf->kernel_ref)) >>>>> + fput(dmabuf->file); >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_put); >>>>> >>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>>>> index efdc56b9d95f..bc790cb028eb 100644 >>>>> --- a/include/linux/dma-buf.h >>>>> +++ b/include/linux/dma-buf.h >>>>> @@ -308,6 +308,7 @@ struct dma_buf_ops { >>>>> struct dma_buf { >>>>> size_t size; >>>>> struct file *file; >>>>> + atomic64_t kernel_ref; >>>>> struct list_head attachments; >>>>> const struct dma_buf_ops *ops; >>>>> struct mutex lock; >>>>> @@ -436,7 +437,7 @@ struct dma_buf_export_info { >>>>> .owner = THIS_MODULE } >>>>> >>>>> /** >>>>> - * get_dma_buf - convenience wrapper for get_file. >>>>> + * get_dma_buf - increase a kernel ref of dma-buf >>>>> * @dmabuf: [in] pointer to dma_buf >>>>> * >>>>> * Increments the reference count on the dma-buf, needed in >>>>> case >>>>> of drivers >>>>> @@ -446,7 +447,8 @@ struct dma_buf_export_info { >>>>> */ >>>>> static inline void get_dma_buf(struct dma_buf *dmabuf) >>>>> { >>>>> - get_file(dmabuf->file); >>>>> + if (atomic64_inc_return(&dmabuf->kernel_ref) == 1) >>>>> + get_file(dmabuf->file); >>>>> } >>>>> >>>>> /** >>

3 years, 5 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: add kernel count for dma_buf

by Christian König

Am 14.07.21 um 09:11 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Add a refcount for kernel to prevent UAF(Use After Free) issue. Well NAK on so many levels. > > We can assume a case like below: > 1. kernel space alloc dma_buf(file count = 1) > 2. kernel use dma_buf to get fd(file count = 1) > 3. userspace use fd to do mapping (file count = 2) Creating an userspace mapping increases the reference count for the underlying file object. See the implementation of mmap_region(): ... vma->vm_file = get_file(file); error = call_mmap(file, vma); ... What can happen is the the underlying exporter redirects the mmap to a different file, e.g. TTM or GEM drivers do that all the time. But this is fine since then the VA mapping is independent of the DMA-buf. > 4. kernel call dma_buf_put (file count = 1) > 5. userpsace close buffer fd(file count = 0) > 6. at this time, buffer is released, but va is valid!! > So we still can read/write buffer via mmap va, > it maybe cause memory leak, or kernel exception. > And also, if we use "ls -ll" to watch corresponding process > fd link info, it also will cause kernel exception. > > Another case: > Using dma_buf_fd to generate more than 1 fd, because > dma_buf_fd will not increase file count, thus, when close > the second fd, it maybe occurs error. Each opened fd will increase the reference count so this is certainly not correct what you describe here. Regards, Christian. > > Solution: > Add a kernel count for dma_buf, and make sure the file count > of dma_buf.file hold by kernel is 1. > > Notes: For this solution, kref couldn't work because kernel ref > maybe added from 0, but kref don't allow it. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++---- > include/linux/dma-buf.h | 6 ++++-- > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..04ee92aac8b9 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -62,6 +62,7 @@ static void dma_buf_release(struct dentry *dentry) > if (unlikely(!dmabuf)) > return; > > + WARN_ON(atomic64_read(&dmabuf->kernel_ref)); > BUG_ON(dmabuf->vmapping_counter); > > /* > @@ -555,6 +556,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > goto err_module; > } > > + atomic64_set(&dmabuf->kernel_ref, 1); > dmabuf->priv = exp_info->priv; > dmabuf->ops = exp_info->ops; > dmabuf->size = exp_info->size; > @@ -617,6 +619,9 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags) > > fd_install(fd, dmabuf->file); > > + /* Add file cnt for each new fd */ > + get_file(dmabuf->file); > + > return fd; > } > EXPORT_SYMBOL_GPL(dma_buf_fd); > @@ -626,12 +631,13 @@ EXPORT_SYMBOL_GPL(dma_buf_fd); > * @fd: [in] fd associated with the struct dma_buf to be returned > * > * On success, returns the struct dma_buf associated with an fd; uses > - * file's refcounting done by fget to increase refcount. returns ERR_PTR > - * otherwise. > + * dmabuf's ref refcounting done by kref_get to increase refcount. > + * Returns ERR_PTR otherwise. > */ > struct dma_buf *dma_buf_get(int fd) > { > struct file *file; > + struct dma_buf *dmabuf; > > file = fget(fd); > > @@ -643,7 +649,12 @@ struct dma_buf *dma_buf_get(int fd) > return ERR_PTR(-EINVAL); > } > > - return file->private_data; > + dmabuf = file->private_data; > + /* replace file count increase as ref increase for kernel user */ > + get_dma_buf(dmabuf); > + fput(file); > + > + return dmabuf; > } > EXPORT_SYMBOL_GPL(dma_buf_get); > > @@ -662,7 +673,11 @@ void dma_buf_put(struct dma_buf *dmabuf) > if (WARN_ON(!dmabuf || !dmabuf->file)) > return; > > - fput(dmabuf->file); > + if (WARN_ON(!atomic64_read(&dmabuf->kernel_ref))) > + return; > + > + if (!atomic64_dec_return(&dmabuf->kernel_ref)) > + fput(dmabuf->file); > } > EXPORT_SYMBOL_GPL(dma_buf_put); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..bc790cb028eb 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -308,6 +308,7 @@ struct dma_buf_ops { > struct dma_buf { > size_t size; > struct file *file; > + atomic64_t kernel_ref; > struct list_head attachments; > const struct dma_buf_ops *ops; > struct mutex lock; > @@ -436,7 +437,7 @@ struct dma_buf_export_info { > .owner = THIS_MODULE } > > /** > - * get_dma_buf - convenience wrapper for get_file. > + * get_dma_buf - increase a kernel ref of dma-buf > * @dmabuf: [in] pointer to dma_buf > * > * Increments the reference count on the dma-buf, needed in case of drivers > @@ -446,7 +447,8 @@ struct dma_buf_export_info { > */ > static inline void get_dma_buf(struct dma_buf *dmabuf) > { > - get_file(dmabuf->file); > + if (atomic64_inc_return(&dmabuf->kernel_ref) == 1) > + get_file(dmabuf->file); > } > > /**

3 years, 5 months

[PATCH v4 1/4] dma-buf: Require VM_PFNMAP vma for mmap

by Daniel Vetter

tldr; DMA buffers aren't normal memory, expecting that you can use them like that (like calling get_user_pages works, or that they're accounting like any other normal memory) cannot be guaranteed. Since some userspace only runs on integrated devices, where all buffers are actually all resident system memory, there's a huge temptation to assume that a struct page is always present and useable like for any more pagecache backed mmap. This has the potential to result in a uapi nightmare. To stop this gap require that DMA buffer mmaps are VM_PFNMAP, which blocks get_user_pages and all the other struct page based infrastructure for everyone. In spirit this is the uapi counterpart to the kernel-internal CONFIG_DMABUF_DEBUG. Motivated by a recent patch which wanted to swich the system dma-buf heap to vm_insert_page instead of vm_insert_pfn. v2: Jason brought up that we also want to guarantee that all ptes have the pte_special flag set, to catch fast get_user_pages (on architectures that support this). Allowing VM_MIXEDMAP (like VM_SPECIAL does) would still allow vm_insert_page, but limiting to VM_PFNMAP will catch that. >From auditing the various functions to insert pfn pte entires (vm_insert_pfn_prot, remap_pfn_range and all it's callers like dma_mmap_wc) it looks like VM_PFNMAP is already required anyway, so this should be the correct flag to check for. References: https://lore.kernel.org/lkml/CAKMK7uHi+mG0z0HUmNt13QCCvutuRVjpcR0NjRL12k-Wb… Acked-by: Christian König <christian.koenig(a)amd.com> Cc: Jason Gunthorpe <jgg(a)ziepe.ca> Cc: Suren Baghdasaryan <surenb(a)google.com> Cc: Matthew Wilcox <willy(a)infradead.org> Cc: John Stultz <john.stultz(a)linaro.org> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org -- Resending this so I can test the next two patches for vgem/shmem in intel-gfx-ci. Last round failed somehow, but I can't repro that at all locally here. No immediate plans to merge this patch here since ttm isn't addressed yet (and there we have the hugepte issue, for which I don't think we have a clear consensus yet). -Daniel --- drivers/dma-buf/dma-buf.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 510b42771974..65cbd7f0f16a 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -130,6 +130,7 @@ static struct file_system_type dma_buf_fs_type = { static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) { struct dma_buf *dmabuf; + int ret; if (!is_dma_buf_file(file)) return -EINVAL; @@ -145,7 +146,11 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) dmabuf->size >> PAGE_SHIFT) return -EINVAL; - return dmabuf->ops->mmap(dmabuf, vma); + ret = dmabuf->ops->mmap(dmabuf, vma); + + WARN_ON(!(vma->vm_flags & VM_PFNMAP)); + + return ret; } static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) @@ -1276,6 +1281,8 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { + int ret; + if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1296,7 +1303,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff; - return dmabuf->ops->mmap(dmabuf, vma); + ret = dmabuf->ops->mmap(dmabuf, vma); + + WARN_ON(!(vma->vm_flags & VM_PFNMAP)); + + return ret; } EXPORT_SYMBOL_GPL(dma_buf_mmap); -- 2.32.0

3 years, 5 months

[PATCH] dma-buf: Delete the DMA-BUF attachment sysfs statistics

by Hridya Valsaraju

The DMA-BUF attachment statistics form a subset of the DMA-BUF sysfs statistics that recently merged to the drm-misc tree. Since there has been a reported a performance regression due to the overhead of sysfs directory creation/teardown during dma_buf_attach()/dma_buf_detach(), this patch deletes the DMA-BUF attachment statistics from sysfs. Fixes: bdb8d06dfefd (dmabuf: Add the capability to expose DMA-BUF stats in sysfs) Signed-off-by: Hridya Valsaraju <hridya(a)google.com> --- Hello all, One of our partners recently reported a perf regression in a driver which was being caused due to the overhead of setup/teardown of the sysfs attachment statistics in the dma_buf_attach()/dma_buf_detach() invocations. Since the driver's latency requirements were of the order of microseconds(~100us), the overhead was significant. Since this indicates that the solution might not work well for all DMA-BUF importers, I think the right thing to do might be to delete the same before it reaches upstream and becomes ABI :( I apologize for the inconvenience. This patch is based on the drm-misc-next branch. Please feel free to let me know if you would prefer that I send a full revert and new patch that adds the rest of the statistics. Regards, Hridya .../ABI/testing/sysfs-kernel-dmabuf-buffers | 28 ---- drivers/dma-buf/dma-buf-sysfs-stats.c | 140 +----------------- drivers/dma-buf/dma-buf-sysfs-stats.h | 27 ---- drivers/dma-buf/dma-buf.c | 16 -- include/linux/dma-buf.h | 17 --- 5 files changed, 4 insertions(+), 224 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers index a243984ed420..5d3bc997dc64 100644 --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -22,31 +22,3 @@ KernelVersion: v5.13 Contact: Hridya Valsaraju <hridya(a)google.com> Description: This file is read-only and specifies the size of the DMA-BUF in bytes. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This directory will contain subdirectories representing every - attachment of the DMA-BUF. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid> -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This directory will contain information on the attached device - and the number of current distinct device mappings. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/device -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This file is read-only and is a symlink to the attached device's - sysfs entry. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/map_counter -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This file is read-only and contains a map_counter indicating the - number of distinct device mappings of the attachment. diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c index a2638e84199c..053baadcada9 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.c +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -40,14 +40,11 @@ * * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device`` - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter`` * - * The information in the interface can also be used to derive per-exporter and - * per-device usage statistics. The data from the interface can be gathered - * on error conditions or other important events to provide a snapshot of - * DMA-BUF usage. It can also be collected periodically by telemetry to monitor - * various metrics. + * The information in the interface can also be used to derive per-exporter + * statistics. The data from the interface can be gathered on error conditions + * or other important events to provide a snapshot of DMA-BUF usage. + * It can also be collected periodically by telemetry to monitor various metrics. * * Detailed documentation about the interface is present in * Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers. @@ -121,120 +118,6 @@ static struct kobj_type dma_buf_ktype = { .default_groups = dma_buf_stats_default_groups, }; -#define to_dma_buf_attach_entry_from_kobj(x) container_of(x, struct dma_buf_attach_sysfs_entry, kobj) - -struct dma_buf_attach_stats_attribute { - struct attribute attr; - ssize_t (*show)(struct dma_buf_attach_sysfs_entry *sysfs_entry, - struct dma_buf_attach_stats_attribute *attr, char *buf); -}; -#define to_dma_buf_attach_stats_attr(x) container_of(x, struct dma_buf_attach_stats_attribute, attr) - -static ssize_t dma_buf_attach_stats_attribute_show(struct kobject *kobj, - struct attribute *attr, - char *buf) -{ - struct dma_buf_attach_stats_attribute *attribute; - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - attribute = to_dma_buf_attach_stats_attr(attr); - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); - - if (!attribute->show) - return -EIO; - - return attribute->show(sysfs_entry, attribute, buf); -} - -static const struct sysfs_ops dma_buf_attach_stats_sysfs_ops = { - .show = dma_buf_attach_stats_attribute_show, -}; - -static ssize_t map_counter_show(struct dma_buf_attach_sysfs_entry *sysfs_entry, - struct dma_buf_attach_stats_attribute *attr, - char *buf) -{ - return sysfs_emit(buf, "%u\n", sysfs_entry->map_counter); -} - -static struct dma_buf_attach_stats_attribute map_counter_attribute = - __ATTR_RO(map_counter); - -static struct attribute *dma_buf_attach_stats_default_attrs[] = { - &map_counter_attribute.attr, - NULL, -}; -ATTRIBUTE_GROUPS(dma_buf_attach_stats_default); - -static void dma_buf_attach_sysfs_release(struct kobject *kobj) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); - kfree(sysfs_entry); -} - -static struct kobj_type dma_buf_attach_ktype = { - .sysfs_ops = &dma_buf_attach_stats_sysfs_ops, - .release = dma_buf_attach_sysfs_release, - .default_groups = dma_buf_attach_stats_default_groups, -}; - -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - sysfs_entry = attach->sysfs_entry; - if (!sysfs_entry) - return; - - sysfs_delete_link(&sysfs_entry->kobj, &attach->dev->kobj, "device"); - - kobject_del(&sysfs_entry->kobj); - kobject_put(&sysfs_entry->kobj); -} - -int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - int ret; - struct dma_buf *dmabuf; - - if (!attach) - return -EINVAL; - - dmabuf = attach->dmabuf; - - sysfs_entry = kzalloc(sizeof(struct dma_buf_attach_sysfs_entry), - GFP_KERNEL); - if (!sysfs_entry) - return -ENOMEM; - - sysfs_entry->kobj.kset = dmabuf->sysfs_entry->attach_stats_kset; - - attach->sysfs_entry = sysfs_entry; - - ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_attach_ktype, - NULL, "%u", uid); - if (ret) - goto kobj_err; - - ret = sysfs_create_link(&sysfs_entry->kobj, &attach->dev->kobj, - "device"); - if (ret) - goto link_err; - - return 0; - -link_err: - kobject_del(&sysfs_entry->kobj); -kobj_err: - kobject_put(&sysfs_entry->kobj); - attach->sysfs_entry = NULL; - - return ret; -} void dma_buf_stats_teardown(struct dma_buf *dmabuf) { struct dma_buf_sysfs_entry *sysfs_entry; @@ -243,7 +126,6 @@ void dma_buf_stats_teardown(struct dma_buf *dmabuf) if (!sysfs_entry) return; - kset_unregister(sysfs_entry->attach_stats_kset); kobject_del(&sysfs_entry->kobj); kobject_put(&sysfs_entry->kobj); } @@ -290,7 +172,6 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) { struct dma_buf_sysfs_entry *sysfs_entry; int ret; - struct kset *attach_stats_kset; if (!dmabuf || !dmabuf->file) return -EINVAL; @@ -315,21 +196,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) if (ret) goto err_sysfs_dmabuf; - /* create the directory for attachment stats */ - attach_stats_kset = kset_create_and_add("attachments", - &dmabuf_sysfs_no_uevent_ops, - &sysfs_entry->kobj); - if (!attach_stats_kset) { - ret = -ENOMEM; - goto err_sysfs_attach; - } - - sysfs_entry->attach_stats_kset = attach_stats_kset; - return 0; -err_sysfs_attach: - kobject_del(&sysfs_entry->kobj); err_sysfs_dmabuf: kobject_put(&sysfs_entry->kobj); dmabuf->sysfs_entry = NULL; diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h index 5f4703249117..a49c6e2650cc 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.h +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -14,23 +14,8 @@ 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_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid); -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, - int delta) -{ - struct dma_buf_attach_sysfs_entry *entry = attach->sysfs_entry; - entry->map_counter += delta; -} void dma_buf_stats_teardown(struct dma_buf *dmabuf); -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach); -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) -{ - struct dma_buf_sysfs_entry *entry = dmabuf->sysfs_entry; - - return entry->attachment_uid++; -} #else static inline int dma_buf_init_sysfs_statistics(void) @@ -44,19 +29,7 @@ static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) { return 0; } -static inline int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid) -{ - return 0; -} static inline void dma_buf_stats_teardown(struct dma_buf *dmabuf) {} -static inline void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) {} -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, - int delta) {} -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) -{ - return 0; -} #endif #endif // _DMA_BUF_SYSFS_STATS_H diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 510b42771974..b1a6db71c656 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -738,7 +738,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, { struct dma_buf_attachment *attach; int ret; - unsigned int attach_uid; if (WARN_ON(!dmabuf || !dev)) return ERR_PTR(-EINVAL); @@ -764,13 +763,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, } dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); - attach_uid = dma_buf_update_attach_uid(dmabuf); dma_resv_unlock(dmabuf->resv); - ret = dma_buf_attach_stats_setup(attach, attach_uid); - if (ret) - goto err_sysfs; - /* When either the importer or the exporter can't handle dynamic * mappings we cache the mapping here to avoid issues with the * reservation object lock. @@ -797,7 +791,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, dma_resv_unlock(attach->dmabuf->resv); attach->sgt = sgt; attach->dir = DMA_BIDIRECTIONAL; - dma_buf_update_attachment_map_count(attach, 1 /* delta */); } return attach; @@ -814,7 +807,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv); -err_sysfs: dma_buf_detach(dmabuf, attach); return ERR_PTR(ret); } @@ -864,7 +856,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) dma_resv_lock(attach->dmabuf->resv, NULL); __unmap_dma_buf(attach, attach->sgt, attach->dir); - dma_buf_update_attachment_map_count(attach, -1 /* delta */); if (dma_buf_is_dynamic(attach->dmabuf)) { dmabuf->ops->unpin(attach); @@ -878,7 +869,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); - dma_buf_attach_stats_teardown(attach); kfree(attach); } EXPORT_SYMBOL_GPL(dma_buf_detach); @@ -1020,10 +1010,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } } #endif /* CONFIG_DMA_API_DEBUG */ - - if (!IS_ERR(sg_table)) - dma_buf_update_attachment_map_count(attach, 1 /* delta */); - return sg_table; } EXPORT_SYMBOL_GPL(dma_buf_map_attachment); @@ -1061,8 +1047,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) dma_buf_unpin(attach); - - dma_buf_update_attachment_map_count(attach, -1 /* delta */); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 2b814fde0d11..678b2006be78 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -444,15 +444,6 @@ struct dma_buf { struct dma_buf_sysfs_entry { struct kobject kobj; struct dma_buf *dmabuf; - - /** - * @sysfs_entry.attachment_uid: - * - * This is protected by the dma_resv_lock() on @resv and is - * incremented on each attach. - */ - unsigned int attachment_uid; - struct kset *attach_stats_kset; } *sysfs_entry; #endif }; @@ -504,7 +495,6 @@ struct dma_buf_attach_ops { * @importer_ops: importer operations for this attachment, if provided * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. * @importer_priv: importer specific attachment data. - * @sysfs_entry: For exposing information about this attachment in sysfs. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -525,13 +515,6 @@ struct dma_buf_attachment { const struct dma_buf_attach_ops *importer_ops; void *importer_priv; void *priv; -#ifdef CONFIG_DMABUF_SYSFS_STATS - /* for sysfs stats */ - struct dma_buf_attach_sysfs_entry { - struct kobject kobj; - unsigned int map_counter; - } *sysfs_entry; -#endif }; /** -- 2.32.0.93.g670b81a890-goog

3 years, 5 months

[PATCH v4 18/18] dma-resv: Give the docs a do-over

by Daniel Vetter

Specifically document the new/clarified rules around how the shared fences do not have any ordering requirements against the exclusive fence. But also document all the things a bit better, given how central struct dma_resv to dynamic buffer management the docs have been very inadequat. - Lots more links to other pieces of the puzzle. Unfortunately ttm_buffer_object has no docs, so no links :-( - Explain/complain a bit about dma_resv_locking_ctx(). I still don't like that one, but fixing the ttm call chains is going to be horrible. Plus we want to plug in real slowpath locking when we do that anyway. - Main part of the patch is some actual docs for struct dma_resv. Overall I think we still have a lot of bad naming in this area (e.g. dma_resv.fence is singular, but contains the multiple shared fences), but I think that's more indicative of how the semantics and rules are just not great. Another thing that's real awkard is how chaining exclusive fences right now means direct dma_resv.exclusive_fence pointer access with an rcu_assign_pointer. Not so great either. v2: - Fix a pile of typos (Matt, Jason) - Hammer it in that breaking the rules leads to use-after-free issues around dma-buf sharing (Christian) Reviewed-by: Christian König <christian.koenig(a)amd.com> Cc: Jason Ekstrand <jason(a)jlekstrand.net> Cc: Matthew Auld <matthew.auld(a)intel.com> Reviewed-by: Matthew Auld <matthew.auld(a)intel.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-resv.c | 24 ++++++--- include/linux/dma-buf.h | 7 +++ include/linux/dma-resv.h | 104 +++++++++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index e744fd87c63c..84fbe60629e3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -48,6 +48,8 @@ * write operations) or N shared fences (read operations). The RCU * mechanism is used to protect read access to fences from locked * write-side updates. + * + * See struct dma_resv for more details. */ DEFINE_WD_CLASS(reservation_ww_class); @@ -137,7 +139,11 @@ EXPORT_SYMBOL(dma_resv_fini); * @num_fences: number of fences we want to add * * Should be called before dma_resv_add_shared_fence(). Must - * be called with obj->lock held. + * be called with @obj locked through dma_resv_lock(). + * + * Note that the preallocated slots need to be re-reserved if @obj is unlocked + * at any time before calling dma_resv_add_shared_fence(). This is validated + * when CONFIG_DEBUG_MUTEXES is enabled. * * RETURNS * Zero for success, or -errno @@ -234,8 +240,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max); * @obj: the reservation object * @fence: the shared fence to add * - * Add a fence to a shared slot, obj->lock must be held, and + * Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and * dma_resv_reserve_shared() has been called. + * + * See also &dma_resv.fence for a discussion of the semantics. */ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -278,9 +286,11 @@ EXPORT_SYMBOL(dma_resv_add_shared_fence); /** * dma_resv_add_excl_fence - Add an exclusive fence. * @obj: the reservation object - * @fence: the shared fence to add + * @fence: the exclusive fence to add * - * Add a fence to the exclusive slot. The obj->lock must be held. + * Add a fence to the exclusive slot. @obj must be locked with dma_resv_lock(). + * Note that this function replaces all fences attached to @obj, see also + * &dma_resv.fence_excl for a discussion of the semantics. */ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -609,9 +619,11 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) * fence * * Callers are not required to hold specific locks, but maybe hold - * dma_resv_lock() already + * dma_resv_lock() already. + * * RETURNS - * true if all fences signaled, else false + * + * True if all fences signaled, else false. */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 2b814fde0d11..8cc0c55877a6 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -420,6 +420,13 @@ struct dma_buf { * - Dynamic importers should set fences for any access that they can't * disable immediately from their &dma_buf_attach_ops.move_notify * callback. + * + * IMPORTANT: + * + * All drivers must obey the struct dma_resv rules, specifically the + * rules for updating fences, see &dma_resv.fence_excl and + * &dma_resv.fence. If these dependency rules are broken access tracking + * can be lost resulting in use after free issues. */ struct dma_resv *resv; diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index e1ca2080a1ff..9100dd3dc21f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -62,16 +62,90 @@ struct dma_resv_list { /** * struct dma_resv - a reservation object manages fences for a buffer - * @lock: update side lock - * @seq: sequence count for managing RCU read-side synchronization - * @fence_excl: the exclusive fence, if there is one currently - * @fence: list of current shared fences + * + * There are multiple uses for this, with sometimes slightly different rules in + * how the fence slots are used. + * + * One use is to synchronize cross-driver access to a struct dma_buf, either for + * dynamic buffer management or just to handle implicit synchronization between + * different users of the buffer in userspace. See &dma_buf.resv for a more + * in-depth discussion. + * + * The other major use is to manage access and locking within a driver in a + * buffer based memory manager. struct ttm_buffer_object is the canonical + * example here, since this is where reservation objects originated from. But + * use in drivers is spreading and some drivers also manage struct + * drm_gem_object with the same scheme. */ struct dma_resv { + /** + * @lock: + * + * Update side lock. Don't use directly, instead use the wrapper + * functions like dma_resv_lock() and dma_resv_unlock(). + * + * Drivers which use the reservation object to manage memory dynamically + * also use this lock to protect buffer object state like placement, + * allocation policies or throughout command submission. + */ struct ww_mutex lock; + + /** + * @seq: + * + * Sequence count for managing RCU read-side synchronization, allows + * read-only access to @fence_excl and @fence while ensuring we take a + * consistent snapshot. + */ seqcount_ww_mutex_t seq; + /** + * @fence_excl: + * + * The exclusive fence, if there is one currently. + * + * There are two ways to update this fence: + * + * - First by calling dma_resv_add_excl_fence(), which replaces all + * fences attached to the reservation object. To guarantee that no + * fences are lost, this new fence must signal only after all previous + * fences, both shared and exclusive, have signalled. In some cases it + * is convenient to achieve that by attaching a struct dma_fence_array + * with all the new and old fences. + * + * - Alternatively the fence can be set directly, which leaves the + * shared fences unchanged. To guarantee that no fences are lost, this + * new fence must signal only after the previous exclusive fence has + * signalled. Since the shared fences are staying intact, it is not + * necessary to maintain any ordering against those. If semantically + * only a new access is added without actually treating the previous + * one as a dependency the exclusive fences can be strung together + * using struct dma_fence_chain. + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_fence __rcu *fence_excl; + + /** + * @fence: + * + * List of current shared fences. + * + * There are no ordering constraints of shared fences against the + * exclusive fence slot. If a waiter needs to wait for all access, it + * has to wait for both sets of fences to signal. + * + * A new fence is added by calling dma_resv_add_shared_fence(). Since + * this often needs to be done past the point of no return in command + * submission it cannot fail, and therefore sufficient slots need to be + * reserved by calling dma_resv_reserve_shared(). + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_resv_list __rcu *fence; }; @@ -98,6 +172,13 @@ static inline void dma_resv_reset_shared_max(struct dma_resv *obj) {} * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow() called on @obj. + * + * Unlocked by calling dma_resv_unlock(). + * + * See also dma_resv_lock_interruptible() for the interruptible variant. */ static inline int dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -119,6 +200,12 @@ static inline int dma_resv_lock(struct dma_resv *obj, * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow_interruptible() called on + * @obj. + * + * Unlocked by calling dma_resv_unlock(). */ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -134,6 +221,8 @@ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, * Acquires the reservation object after a die case. This function * will sleep until the lock becomes available. See dma_resv_lock() as * well. + * + * See also dma_resv_lock_slow_interruptible() for the interruptible variant. */ static inline void dma_resv_lock_slow(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -167,7 +256,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj, * if they overlap with a writer. * * Also note that since no context is provided, no deadlock protection is - * possible. + * possible, which is also not needed for a trylock. * * Returns true if the lock was acquired, false otherwise. */ @@ -193,6 +282,11 @@ static inline bool dma_resv_is_locked(struct dma_resv *obj) * * Returns the context used to lock a reservation object or NULL if no context * was used or the object is not locked at all. + * + * WARNING: This interface is pretty horrible, but TTM needs it because it + * doesn't pass the struct ww_acquire_ctx around in some very long callchains. + * Everyone else just uses it to check whether they're holding a reservation or + * not. */ static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) { -- 2.32.0

3 years, 5 months

[PATCH v4 11/18] drm/gem: Delete gem array fencing helpers

by Daniel Vetter

Integrated into the scheduler now and all users converted over. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/drm_gem.c | 96 --------------------------------------- include/drm/drm_gem.h | 5 -- 2 files changed, 101 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 68deb1de8235..24d49a2636e0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1294,99 +1294,3 @@ drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, ww_acquire_fini(acquire_ctx); } EXPORT_SYMBOL(drm_gem_unlock_reservations); - -/** - * drm_gem_fence_array_add - Adds the fence to an array of fences to be - * waited on, deduplicating fences from the same context. - * - * @fence_array: array of dma_fence * for the job to block on. - * @fence: the dma_fence to add to the list of dependencies. - * - * This functions consumes the reference for @fence both on success and error - * cases. - * - * Returns: - * 0 on success, or an error on failing to expand the array. - */ -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence) -{ - struct dma_fence *entry; - unsigned long index; - u32 id = 0; - int ret; - - if (!fence) - return 0; - - /* Deduplicate if we already depend on a fence from the same context. - * This lets the size of the array of deps scale with the number of - * engines involved, rather than the number of BOs. - */ - xa_for_each(fence_array, index, entry) { - if (entry->context != fence->context) - continue; - - if (dma_fence_is_later(fence, entry)) { - dma_fence_put(entry); - xa_store(fence_array, index, fence, GFP_KERNEL); - } else { - dma_fence_put(fence); - } - return 0; - } - - ret = xa_alloc(fence_array, &id, fence, xa_limit_32b, GFP_KERNEL); - if (ret != 0) - dma_fence_put(fence); - - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add); - -/** - * drm_gem_fence_array_add_implicit - Adds the implicit dependencies tracked - * in the GEM object's reservation object to an array of dma_fences for use in - * scheduling a rendering job. - * - * This should be called after drm_gem_lock_reservations() on your array of - * GEM objects used in the job but before updating the reservations with your - * own fences. - * - * @fence_array: array of dma_fence * for the job to block on. - * @obj: the gem object to add new dependencies from. - * @write: whether the job might write the object (so we need to depend on - * shared fences in the reservation object). - */ -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write) -{ - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); - if (ret) - break; - } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 240049566592..6d5e33b89074 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -409,11 +409,6 @@ int drm_gem_lock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence); -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write); int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset); -- 2.32.0

3 years, 5 months

[PATCH v4 10/18] drm/etnaviv: Use scheduler dependency handling

by Daniel Vetter

We need to pull the drm_sched_job_init much earlier, but that's very minor surgery. v2: Actually fix up cleanup paths by calling drm_sched_job_init, which I wanted to to in the previous round (and did, for all other drivers). Spotted by Lucas. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: etnaviv(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 5 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 58 +++++++++--------- drivers/gpu/drm/etnaviv/etnaviv_sched.c | 63 +------------------- drivers/gpu/drm/etnaviv/etnaviv_sched.h | 3 +- 4 files changed, 35 insertions(+), 94 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index 98e60df882b6..63688e6e4580 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -80,9 +80,6 @@ struct etnaviv_gem_submit_bo { u64 va; struct etnaviv_gem_object *obj; struct etnaviv_vram_mapping *mapping; - struct dma_fence *excl; - unsigned int nr_shared; - struct dma_fence **shared; }; /* Created per submit-ioctl, to track bo's and cmdstream bufs, etc, @@ -95,7 +92,7 @@ struct etnaviv_gem_submit { struct etnaviv_file_private *ctx; struct etnaviv_gpu *gpu; struct etnaviv_iommu_context *mmu_context, *prev_mmu_context; - struct dma_fence *out_fence, *in_fence; + struct dma_fence *out_fence; int out_fence_id; struct list_head node; /* GPU active submit list */ struct etnaviv_cmdbuf cmdbuf; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..5b97ce1299ad 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -188,16 +188,10 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) continue; - if (bo->flags & ETNA_SUBMIT_BO_WRITE) { - ret = dma_resv_get_fences(robj, &bo->excl, - &bo->nr_shared, - &bo->shared); - if (ret) - return ret; - } else { - bo->excl = dma_resv_get_excl_unlocked(robj); - } - + ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, + bo->flags & ETNA_SUBMIT_BO_WRITE); + if (ret) + return ret; } return ret; @@ -403,8 +397,6 @@ static void submit_cleanup(struct kref *kref) wake_up_all(&submit->gpu->fence_event); - if (submit->in_fence) - dma_fence_put(submit->in_fence); if (submit->out_fence) { /* first remove from IDR, so fence can not be found anymore */ mutex_lock(&submit->gpu->fence_lock); @@ -529,7 +521,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &submit->cmdbuf, ALIGN(args->stream_size, 8) + 8); if (ret) - goto err_submit_objects; + goto err_submit_put; submit->ctx = file->driver_priv; etnaviv_iommu_context_get(submit->ctx->mmu); @@ -537,51 +529,61 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, submit->exec_state = args->exec_state; submit->flags = args->flags; + ret = drm_sched_job_init(&submit->sched_job, + &ctx->sched_entity[args->pipe], + submit->ctx); + if (ret) + goto err_submit_put; + ret = submit_lookup_objects(submit, file, bos, args->nr_bos); if (ret) - goto err_submit_objects; + goto err_submit_job; if ((priv->mmu_global->version != ETNAVIV_IOMMU_V2) && !etnaviv_cmd_validate_one(gpu, stream, args->stream_size / 4, relocs, args->nr_relocs)) { ret = -EINVAL; - goto err_submit_objects; + goto err_submit_job; } if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) { - submit->in_fence = sync_file_get_fence(args->fence_fd); - if (!submit->in_fence) { + struct dma_fence *in_fence = sync_file_get_fence(args->fence_fd); + if (!in_fence) { ret = -EINVAL; - goto err_submit_objects; + goto err_submit_job; } + + ret = drm_sched_job_await_fence(&submit->sched_job, in_fence); + if (ret) + goto err_submit_job; } ret = submit_pin_objects(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_reloc(submit, stream, args->stream_size / 4, relocs, args->nr_relocs); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_perfmon_validate(submit, args->exec_state, pmrs); if (ret) - goto err_submit_objects; + goto err_submit_job; memcpy(submit->cmdbuf.vaddr, stream, args->stream_size); ret = submit_lock_objects(submit, &ticket); if (ret) - goto err_submit_objects; + goto err_submit_job; ret = submit_fence_sync(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; - ret = etnaviv_sched_push_job(&ctx->sched_entity[args->pipe], submit); + ret = etnaviv_sched_push_job(submit); if (ret) - goto err_submit_objects; + goto err_submit_job; submit_attach_object_fences(submit); @@ -595,7 +597,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, sync_file = sync_file_create(submit->out_fence); if (!sync_file) { ret = -ENOMEM; - goto err_submit_objects; + goto err_submit_job; } fd_install(out_fence_fd, sync_file->file); } @@ -603,7 +605,9 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, args->fence_fd = out_fence_fd; args->fence = submit->out_fence_id; -err_submit_objects: +err_submit_job: + drm_sched_job_cleanup(&submit->sched_job); +err_submit_put: etnaviv_submit_put(submit); err_submit_ww_acquire: diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 180bb633d5c5..2bbbd6ccc95e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -17,58 +17,6 @@ module_param_named(job_hang_limit, etnaviv_job_hang_limit, int , 0444); static int etnaviv_hw_jobs_limit = 4; module_param_named(hw_job_limit, etnaviv_hw_jobs_limit, int , 0444); -static struct dma_fence * -etnaviv_sched_dependency(struct drm_sched_job *sched_job, - struct drm_sched_entity *entity) -{ - struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); - struct dma_fence *fence; - int i; - - if (unlikely(submit->in_fence)) { - fence = submit->in_fence; - submit->in_fence = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - - for (i = 0; i < submit->nr_bos; i++) { - struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; - int j; - - if (bo->excl) { - fence = bo->excl; - bo->excl = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - - for (j = 0; j < bo->nr_shared; j++) { - if (!bo->shared[j]) - continue; - - fence = bo->shared[j]; - bo->shared[j] = NULL; - - if (!dma_fence_is_signaled(fence)) - return fence; - - dma_fence_put(fence); - } - kfree(bo->shared); - bo->nr_shared = 0; - bo->shared = NULL; - } - - return NULL; -} - static struct dma_fence *etnaviv_sched_run_job(struct drm_sched_job *sched_job) { struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); @@ -140,29 +88,22 @@ static void etnaviv_sched_free_job(struct drm_sched_job *sched_job) } static const struct drm_sched_backend_ops etnaviv_sched_ops = { - .dependency = etnaviv_sched_dependency, .run_job = etnaviv_sched_run_job, .timedout_job = etnaviv_sched_timedout_job, .free_job = etnaviv_sched_free_job, }; -int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, - struct etnaviv_gem_submit *submit) +int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit) { int ret = 0; /* * Hold the fence lock across the whole operation to avoid jobs being * pushed out of order with regard to their sched fence seqnos as - * allocated in drm_sched_job_init. + * allocated in drm_sched_job_arm. */ mutex_lock(&submit->gpu->fence_lock); - ret = drm_sched_job_init(&submit->sched_job, sched_entity, - submit->ctx); - if (ret) - goto out_unlock; - drm_sched_job_arm(&submit->sched_job); submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.h b/drivers/gpu/drm/etnaviv/etnaviv_sched.h index c0a6796e22c9..baebfa069afc 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.h @@ -18,7 +18,6 @@ struct etnaviv_gem_submit *to_etnaviv_submit(struct drm_sched_job *sched_job) int etnaviv_sched_init(struct etnaviv_gpu *gpu); void etnaviv_sched_fini(struct etnaviv_gpu *gpu); -int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, - struct etnaviv_gem_submit *submit); +int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit); #endif /* __ETNAVIV_SCHED_H__ */ -- 2.32.0

3 years, 5 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig July 2021