Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

1 participants
2905 discussions

[PATCH AUTOSEL 5.7 06/53] drm/ttm: Fix dma_fence refcnt leak in ttm_bo_vm_fault_reserved

by Sasha Levin

From: Xiyu Yang <xiyuyang19(a)fudan.edu.cn> [ Upstream commit 37cc4b95d13f311c04aa8e9daacca3905ad45ca7 ] ttm_bo_vm_fault_reserved() invokes dma_fence_get(), which returns a reference of the specified dma_fence object to "moving" with increased refcnt. When ttm_bo_vm_fault_reserved() returns, local variable "moving" becomes invalid, so the refcount should be decreased to keep refcount balanced. The reference counting issue happens in several exception handling paths of ttm_bo_vm_fault_reserved(). When those error scenarios occur such as "err" equals to -EBUSY, the function forgets to decrease the refcnt increased by dma_fence_get(), causing a refcnt leak. Fix this issue by calling dma_fence_put() when no_wait_gpu flag is equals to true. Signed-off-by: Xiyu Yang <xiyuyang19(a)fudan.edu.cn> Signed-off-by: Xin Tan <tanxin.ctf(a)gmail.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/370219/ Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index 0ad30b1129821..72100b84c7a90 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -300,8 +300,10 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf, break; case -EBUSY: case -ERESTARTSYS: + dma_fence_put(moving); return VM_FAULT_NOPAGE; default: + dma_fence_put(moving); return VM_FAULT_SIGBUS; } -- 2.25.1

4 years, 12 months

[PATCH] MAINTAINERS: Add myself as DMA-buf maintainer

by Christian König

As discussed on the list. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 40474982a21d..5d7130f8d342 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5089,6 +5089,7 @@ F: fs/dlm/ DMA BUFFER SHARING FRAMEWORK M: Sumit Semwal <sumit.semwal(a)linaro.org> +M: Christian König <christian.koenig(a)amd.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) -- 2.20.1

5 years

Re: [Linaro-mm-sig] [PATCH v7 31/36] staging: tegra-vde: fix common struct sg_table related issues

by Marek Szyprowski

On 21.06.2020 06:00, Dmitry Osipenko wrote: > В Fri, 19 Jun 2020 12:36:31 +0200 > Marek Szyprowski <m.szyprowski(a)samsung.com> пишет: > >> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() >> function returns the number of the created entries in the DMA address >> space. However the subsequent calls to the >> dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with >> the original number of the entries passed to the dma_map_sg(). >> >> struct sg_table is a common structure used for describing a >> non-contiguous memory buffer, used commonly in the DRM and graphics >> subsystems. It consists of a scatterlist with memory pages and DMA >> addresses (sgl entry), as well as the number of scatterlist entries: >> CPU pages (orig_nents entry) and DMA mapped pages (nents entry). >> >> It turned out that it was a common mistake to misuse nents and >> orig_nents entries, calling DMA-mapping functions with a wrong number >> of entries or ignoring the number of mapped entries returned by the >> dma_map_sg() function. >> >> To avoid such issues, lets use a common dma-mapping wrappers operating >> directly on the struct sg_table objects and use scatterlist page >> iterators where possible. This, almost always, hides references to the >> nents and orig_nents entries, making the code robust, easier to follow >> and copy/paste safe. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski(a)samsung.com> >> Reviewed-by: Dmitry Osipenko <digetx(a)gmail.com> >> --- >> drivers/staging/media/tegra-vde/iommu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/media/tegra-vde/iommu.c >> b/drivers/staging/media/tegra-vde/iommu.c index >> 6af863d92123..adf8dc7ee25c 100644 --- >> a/drivers/staging/media/tegra-vde/iommu.c +++ >> b/drivers/staging/media/tegra-vde/iommu.c @@ -36,8 +36,8 @@ int >> tegra_vde_iommu_map(struct tegra_vde *vde, >> addr = iova_dma_addr(&vde->iova, iova); >> >> - size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents, >> - IOMMU_READ | IOMMU_WRITE); >> + size = iommu_map_sgtable(vde->domain, addr, sgt, >> + IOMMU_READ | IOMMU_WRITE); >> if (!size) { >> __free_iova(&vde->iova, iova); >> return -ENXIO; > Ahh, I saw the build failure report. You're changing the DMA API in > this series, while DMA API isn't used by this driver, it uses IOMMU > API. Hence there is no need to touch this code. Similar problem in the > host1x driver patch. The issue is caused by the lack of iommu_map_sgtable() stub when no IOMMU support is configured. I've posted a patch for this: https://lore.kernel.org/lkml/20200630081756.18526-1-m.szyprowski@samsung.co… The patch for this driver is fine, we have to wait until the above fix gets merged and then it can be applied during the next release cycle. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

5 years

Re: [Linaro-mm-sig] [PATCH 1/2] dt-bindings: serial: add generic DT binding for announcing RTS/CTS lines

by Greg Kroah-Hartman

On Wed, May 20, 2020 at 03:39:31PM +0200, Erwan Le Ray wrote: > Add support of generic DT binding for annoucing RTS/CTS lines. The initial > binding 'st,hw-flow-control' is not needed anymore since generic binding > is available, but is kept for backward compatibility. > > Signed-off-by: Erwan Le Ray <erwan.leray(a)st.com> > > diff --git a/Documentation/devicetree/bindings/serial/st,stm32-uart.yaml b/Documentation/devicetree/bindings/serial/st,stm32-uart.yaml > index 75b8521eb7cb..06d5f251ec88 100644 > --- a/Documentation/devicetree/bindings/serial/st,stm32-uart.yaml > +++ b/Documentation/devicetree/bindings/serial/st,stm32-uart.yaml > @@ -35,9 +35,11 @@ properties: > description: label associated with this uart > > st,hw-flow-ctrl: > - description: enable hardware flow control > + description: enable hardware flow control (deprecated) > $ref: /schemas/types.yaml#/definitions/flag > > + uart-has-rtscts: true > + > dmas: > minItems: 1 > maxItems: 2 > -- > 2.17.1 > Did this get ignored by the DT maintainers? :(

5 years

Re: [Linaro-mm-sig] [PATCH 04/18] dma-fence: prime lockdep annotations

by Daniel Vetter

On Thu, Jun 11, 2020 at 09:30:12AM +0200, Thomas Hellström (Intel) wrote: > > On 6/4/20 10:12 AM, Daniel Vetter wrote: > > Two in one go: > > - it is allowed to call dma_fence_wait() while holding a > > dma_resv_lock(). This is fundamental to how eviction works with ttm, > > so required. > > > > - it is allowed to call dma_fence_wait() from memory reclaim contexts, > > specifically from shrinker callbacks (which i915 does), and from mmu > > notifier callbacks (which amdgpu does, and which i915 sometimes also > > does, and probably always should, but that's kinda a debate). Also > > for stuff like HMM we really need to be able to do this, or things > > get real dicey. > > > > Consequence is that any critical path necessary to get to a > > dma_fence_signal for a fence must never a) call dma_resv_lock nor b) > > allocate memory with GFP_KERNEL. Also by implication of > > dma_resv_lock(), no userspace faulting allowed. That's some supremely > > obnoxious limitations, which is why we need to sprinkle the right > > annotations to all relevant paths. > > > > The one big locking context we're leaving out here is mmu notifiers, > > added in > > > > commit 23b68395c7c78a764e8963fc15a7cfd318bf187f > > Author: Daniel Vetter <daniel.vetter(a)ffwll.ch> > > Date: Mon Aug 26 22:14:21 2019 +0200 > > > > mm/mmu_notifiers: add a lockdep map for invalidate_range_start/end > > > > that one covers a lot of other callsites, and it's also allowed to > > wait on dma-fences from mmu notifiers. But there's no ready-made > > functions exposed to prime this, so I've left it out for now. > > > > v2: Also track against mmu notifier context. > > > > v3: kerneldoc to spec the cross-driver contract. Note that currently > > i915 throws in a hard-coded 10s timeout on foreign fences (not sure > > why that was done, but it's there), which is why that rule is worded > > with SHOULD instead of MUST. > > > > Also some of the mmu_notifier/shrinker rules might surprise SoC > > drivers, I haven't fully audited them all. Which is infeasible anyway, > > we'll need to run them with lockdep and dma-fence annotations and see > > what goes boom. > > > > v4: A spelling fix from Mika > > > > Cc: Mika Kuoppala <mika.kuoppala(a)intel.com> > > Cc: Thomas Hellstrom <thomas.hellstrom(a)intel.com> > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > Cc: linux-rdma(a)vger.kernel.org > > Cc: amd-gfx(a)lists.freedesktop.org > > Cc: intel-gfx(a)lists.freedesktop.org > > Cc: Chris Wilson <chris(a)chris-wilson.co.uk> > > Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > > Cc: Christian König <christian.koenig(a)amd.com> > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > --- > > Documentation/driver-api/dma-buf.rst | 6 ++++ > > drivers/dma-buf/dma-fence.c | 41 ++++++++++++++++++++++++++++ > > drivers/dma-buf/dma-resv.c | 4 +++ > > include/linux/dma-fence.h | 1 + > > 4 files changed, 52 insertions(+) > > I still have my doubts about allowing fence waiting from within shrinkers. > IMO ideally they should use a trywait approach, in order to allow memory > allocation during command submission for drivers that > publish fences before command submission. (Since early reservation object > release requires that). Yeah it is a bit annoying, e.g. for drm/scheduler I think we'll end up with a mempool to make sure it can handle it's allocations. > But since drivers are already waiting from within shrinkers and I take your > word for HMM requiring this, Yeah the big trouble is HMM and mmu notifiers. That's the really awkward one, the shrinker one is a lot less established. I do wonder whether the mmu notifier constraint should only be set when mmu notifiers are enabled, since on a bunch of arm-soc gpu drivers that stuff just doesn't matter. But I expect that sooner or later these arm gpus will show up in bigger arm cores, where you might want to have kvm and maybe device virtualization and stuff, and then you need mmu notifiers. Plus having a very clear and consistent cross-driver api contract is imo better than leaving this up to drivers and then having incompatible assumptions. I've pinged a bunch of armsoc gpu driver people and ask them how much this hurts, so that we have a clear answer. On x86 I don't think we have much of a choice on this, with userptr in amd and i915 and hmm work in nouveau (but nouveau I think doesn't use dma_fence in there). I think it'll take us a while to really bottom out on this specific question here. -Daniel > > Reviewed-by: Thomas Hellström <thomas.hellstrom(a)intel.com> > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

[PATCH v2] dmabuf: use spinlock to access dmabuf->name

by Charan Teja Kalla

There exists a sleep-while-atomic bug while accessing the dmabuf->name under mutex in the dmabuffs_dname(). This is caused from the SELinux permissions checks on a process where it tries to validate the inherited files from fork() by traversing them through iterate_fd() (which traverse files under spin_lock) and call match_file(security/selinux/hooks.c) where the permission checks happen. This audit information is logged using dump_common_audit_data() where it calls d_path() to get the file path name. If the file check happen on the dmabuf's fd, then it ends up in ->dmabuffs_dname() and use mutex to access dmabuf->name. The flow will be like below: flush_unauthorized_files() iterate_fd() spin_lock() --> Start of the atomic section. match_file() file_has_perm() avc_has_perm() avc_audit() slow_avc_audit() common_lsm_audit() dump_common_audit_data() audit_log_d_path() d_path() dmabuffs_dname() mutex_lock()--> Sleep while atomic. Call trace captured (on 4.19 kernels) is below: ___might_sleep+0x204/0x208 __might_sleep+0x50/0x88 __mutex_lock_common+0x5c/0x1068 __mutex_lock_common+0x5c/0x1068 mutex_lock_nested+0x40/0x50 dmabuffs_dname+0xa0/0x170 d_path+0x84/0x290 audit_log_d_path+0x74/0x130 common_lsm_audit+0x334/0x6e8 slow_avc_audit+0xb8/0xf8 avc_has_perm+0x154/0x218 file_has_perm+0x70/0x180 match_file+0x60/0x78 iterate_fd+0x128/0x168 selinux_bprm_committing_creds+0x178/0x248 security_bprm_committing_creds+0x30/0x48 install_exec_creds+0x1c/0x68 load_elf_binary+0x3a4/0x14e0 search_binary_handler+0xb0/0x1e0 So, use spinlock to access dmabuf->name to avoid sleep-while-atomic. Cc: <stable(a)vger.kernel.org> [5.3+] Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> --- Changes in V2: Addressed review comments from Ruhl, Michael J Changes in V1: https://lore.kernel.org/patchwork/patch/1255055/ drivers/dma-buf/dma-buf.c | 11 +++++++---- include/linux/dma-buf.h | 1 + 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 01ce125..d81d298 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) size_t ret = 0; dmabuf = dentry->d_fsdata; - dma_resv_lock(dmabuf->resv, NULL); + spin_lock(&dmabuf->name_lock); if (dmabuf->name) ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); - dma_resv_unlock(dmabuf->resv); + spin_unlock(&dmabuf->name_lock); return dynamic_dname(dentry, buffer, buflen, "/%s:%s", dentry->d_name.name, ret > 0 ? name : ""); @@ -341,8 +341,10 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) kfree(name); goto out_unlock; } + spin_lock(&dmabuf->name_lock); kfree(dmabuf->name); dmabuf->name = name; + spin_unlock(&dmabuf->name_lock); out_unlock: dma_resv_unlock(dmabuf->resv); @@ -405,10 +407,10 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) /* Don't count the temporary reference taken inside procfs seq_show */ seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1); seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name); - dma_resv_lock(dmabuf->resv, NULL); + spin_lock(&dmabuf->name_lock); if (dmabuf->name) seq_printf(m, "name:\t%s\n", dmabuf->name); - dma_resv_unlock(dmabuf->resv); + spin_unlock(&dmabuf->name_lock); } static const struct file_operations dma_buf_fops = { @@ -546,6 +548,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) dmabuf->size = exp_info->size; dmabuf->exp_name = exp_info->exp_name; dmabuf->owner = exp_info->owner; + spin_lock_init(&dmabuf->name_lock); init_waitqueue_head(&dmabuf->poll); dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll; dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index ab0c156..93108fd 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -311,6 +311,7 @@ struct dma_buf { void *vmap_ptr; const char *exp_name; const char *name; + spinlock_t name_lock; struct module *owner; struct list_head list_node; void *priv; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

5 years

[PATCH 08/18] drm/amdgpu: add dma-fence annotations to atomic commit path

by Daniel Vetter

I need a canary in a ttm-based atomic driver to make sure the dma_fence_begin/end_signalling annotations actually work. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index bdba0bfd6df1..adabfa929f42 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -57,6 +57,7 @@ #include "ivsrcid/ivsrcid_vislands30.h" +#include <linux/module.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/version.h> @@ -7320,6 +7321,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) struct drm_connector_state *old_con_state, *new_con_state; struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state; int crtc_disable_count = 0; + bool fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_update_legacy_modeset_state(dev, state); @@ -7600,6 +7604,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) /* Signal HW programming completion */ drm_atomic_helper_commit_hw_done(state); + dma_fence_end_signalling(fence_cookie); + if (wait_for_vblank) drm_atomic_helper_wait_for_flip_done(dev, state); -- 2.26.2

5 years

[PATCH v2] dma-buf: Move dma_buf_release() from fops to dentry_ops

by Sumit Semwal

Charan Teja reported a 'use-after-free' in dmabuffs_dname [1], which happens if the dma_buf_release() is called while the userspace is accessing the dma_buf pseudo fs's dmabuffs_dname() in another process, and dma_buf_release() releases the dmabuf object when the last reference to the struct file goes away. I discussed with Arnd Bergmann, and he suggested that rather than tying the dma_buf_release() to the file_operations' release(), we can tie it to the dentry_operations' d_release(), which will be called when the last ref to the dentry is removed. The path exercised by __fput() calls f_op->release() first, and then calls dput, which eventually calls d_op->d_release(). In the 'normal' case, when no userspace access is happening via dma_buf pseudo fs, there should be exactly one fd, file, dentry and inode, so closing the fd will kill of everything right away. In the presented case, the dentry's d_release() will be called only when the dentry's last ref is released. Therefore, lets move dma_buf_release() from fops->release() to d_ops->d_release() Many thanks to Arnd for his FS insights :) [1]: https://lore.kernel.org/patchwork/patch/1238278/ Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") Reported-by: syzbot+3643a18836bce555bff6(a)syzkaller.appspotmail.com Cc: <stable(a)vger.kernel.org> [5.3+] Cc: Arnd Bergmann <arnd(a)arndb.de> Reported-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Arnd Bergmann <arnd(a)arndb.de> Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org> --- v2: per Arnd: Moved dma_buf_release() above to avoid forward declaration; removed dentry_ops check. --- drivers/dma-buf/dma-buf.c | 54 ++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 01ce125f8e8d..412629601ad3 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -54,37 +54,11 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) dentry->d_name.name, ret > 0 ? name : ""); } -static const struct dentry_operations dma_buf_dentry_ops = { - .d_dname = dmabuffs_dname, -}; - -static struct vfsmount *dma_buf_mnt; - -static int dma_buf_fs_init_context(struct fs_context *fc) -{ - struct pseudo_fs_context *ctx; - - ctx = init_pseudo(fc, DMA_BUF_MAGIC); - if (!ctx) - return -ENOMEM; - ctx->dops = &dma_buf_dentry_ops; - return 0; -} - -static struct file_system_type dma_buf_fs_type = { - .name = "dmabuf", - .init_fs_context = dma_buf_fs_init_context, - .kill_sb = kill_anon_super, -}; - -static int dma_buf_release(struct inode *inode, struct file *file) +static void dma_buf_release(struct dentry *dentry) { struct dma_buf *dmabuf; - if (!is_dma_buf_file(file)) - return -EINVAL; - - dmabuf = file->private_data; + dmabuf = dentry->d_fsdata; BUG_ON(dmabuf->vmapping_counter); @@ -110,9 +84,32 @@ static int dma_buf_release(struct inode *inode, struct file *file) module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); +} + +static const struct dentry_operations dma_buf_dentry_ops = { + .d_dname = dmabuffs_dname, + .d_release = dma_buf_release, +}; + +static struct vfsmount *dma_buf_mnt; + +static int dma_buf_fs_init_context(struct fs_context *fc) +{ + struct pseudo_fs_context *ctx; + + ctx = init_pseudo(fc, DMA_BUF_MAGIC); + if (!ctx) + return -ENOMEM; + ctx->dops = &dma_buf_dentry_ops; return 0; } +static struct file_system_type dma_buf_fs_type = { + .name = "dmabuf", + .init_fs_context = dma_buf_fs_init_context, + .kill_sb = kill_anon_super, +}; + static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma) { struct dma_buf *dmabuf; @@ -412,7 +409,6 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) } static const struct file_operations dma_buf_fops = { - .release = dma_buf_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, -- 2.27.0

5 years

[PATCH v6 00/36] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the. drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used a simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. To avoid similar issues in the future, I've introduced a common wrappers for DMA-mapping calls, which operate directly on the sg_table objects. I've also added wrappers for iterating over the scatterlists stored in the sg_table objects and applied them where possible. This, together with some common DRM prime helpers, allowed me to almost get rid of all nents/orig_nents usage in the drivers. I hope that such change makes the code robust, easier to follow and copy/paste safe. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. Patches are based on top of Linux next-20200618. The required changes to DMA-mapping framework has been already merged to v5.8-rc1. If possible I would like ask for merging most of the patches via DRM tree. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt [4] https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/#ma18c9… Changelog: v6: - rebased onto Linux next-20200618, which is based on v5.8-rc1; fixed conflicts v5: https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@sams… - fixed some minor style issues and typos - fixed lack of the attrs argument in ion, dmabuf, rapidio, fastrpc and vfio patches v4: https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/ - added for_each_sgtable_* wrappers and applied where possible - added drm_prime_get_contiguous_size() and applied where possible - applied drm_prime_sg_to_page_addr_arrays() where possible to remove page extraction from sg_table objects - added documentation for the introduced wrappers - improved patches description a bit v3: https://lore.kernel.org/dri-devel/20200505083926.28503-1-m.szyprowski@samsu… - introduce dma_*_sgtable_* wrappers and use them in all patches v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (36): drm: prime: add common helper to check scatterlist contiguity drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() drm: core: fix common struct sg_table related issues drm: amdgpu: fix common struct sg_table related issues drm: armada: fix common struct sg_table related issues drm: etnaviv: fix common struct sg_table related issues drm: exynos: use common helper for a scatterlist contiguity check drm: exynos: fix common struct sg_table related issues drm: i915: fix common struct sg_table related issues drm: lima: fix common struct sg_table related issues drm: mediatek: use common helper for a scatterlist contiguity check drm: mediatek: use common helper for extracting pages array drm: msm: fix common struct sg_table related issues drm: omapdrm: use common helper for extracting pages array drm: omapdrm: fix common struct sg_table related issues drm: panfrost: fix common struct sg_table related issues drm: radeon: fix common struct sg_table related issues drm: rockchip: use common helper for a scatterlist contiguity check drm: rockchip: fix common struct sg_table related issues drm: tegra: fix common struct sg_table related issues drm: v3d: fix common struct sg_table related issues drm: virtio: fix common struct sg_table related issues drm: vmwgfx: fix common struct sg_table related issues xen: gntdev: fix common struct sg_table related issues drm: host1x: fix common struct sg_table related issues drm: rcar-du: fix common struct sg_table related issues dmabuf: fix common struct sg_table related issues staging: ion: remove dead code staging: ion: fix common struct sg_table related issues staging: tegra-vde: fix common struct sg_table related issues misc: fastrpc: fix common struct sg_table related issues rapidio: fix common struct sg_table related issues samples: vfio-mdev/mbochs: fix common struct sg_table related issues media: pci: fix common ALSA DMA-mapping related codes videobuf2: use sgtable-based scatterlist wrappers drm: xen: fix common struct sg_table related issues drivers/dma-buf/heaps/heap-helpers.c | 13 ++- drivers/dma-buf/udmabuf.c | 7 +- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 +- drivers/gpu/drm/armada/armada_gem.c | 12 +-- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_cma_helper.c | 23 +---- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +-- drivers/gpu/drm/drm_prime.c | 86 ++++++++++--------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +-- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +-- drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +---- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +-- .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +- drivers/gpu/drm/lima/lima_gem.c | 11 ++- drivers/gpu/drm/lima/lima_vm.c | 5 +- drivers/gpu/drm/mediatek/mtk_drm_gem.c | 37 ++------ drivers/gpu/drm/msm/msm_gem.c | 13 ++- drivers/gpu/drm/msm/msm_gpummu.c | 14 ++- drivers/gpu/drm/msm/msm_iommu.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 20 ++--- drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +- drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 42 +++------ drivers/gpu/drm/tegra/gem.c | 27 +++--- drivers/gpu/drm/tegra/plane.c | 15 ++-- drivers/gpu/drm/v3d/v3d_mmu.c | 17 ++-- drivers/gpu/drm/virtio/virtgpu_object.c | 36 ++++---- drivers/gpu/drm/virtio/virtgpu_vq.c | 12 ++- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +--- drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +- drivers/gpu/host1x/job.c | 22 ++--- .../common/videobuf2/videobuf2-dma-contig.c | 41 ++++----- .../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++---- .../common/videobuf2/videobuf2-vmalloc.c | 12 +-- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 8 +- drivers/misc/fastrpc.c | 4 +- drivers/rapidio/devices/rio_mport_cdev.c | 8 +- drivers/staging/android/ion/ion.c | 25 +++--- drivers/staging/android/ion/ion.h | 1 - drivers/staging/android/ion/ion_heap.c | 53 +++--------- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/staging/media/tegra-vde/iommu.c | 4 +- drivers/xen/gntdev-dmabuf.c | 13 ++- include/drm/drm_prime.h | 2 + samples/vfio-mdev/mbochs.c | 3 +- 54 files changed, 311 insertions(+), 478 deletions(-) -- 2.17.1

5 years

Re: [Linaro-mm-sig] [PATCH v4 1/3] virtio: add dma-buf support for exported objects

by Guennadi Liakhovetski

Hi Michael, On Thu, Jun 04, 2020 at 03:05:23PM -0400, Michael S. Tsirkin wrote: > On Tue, May 26, 2020 at 07:58:09PM +0900, David Stevens wrote: > > This change adds a new flavor of dma-bufs that can be used by virtio > > drivers to share exported objects. A virtio dma-buf can be queried by > > virtio drivers to obtain the UUID which identifies the underlying > > exported object. > > > > Signed-off-by: David Stevens <stevensd(a)chromium.org> > > Is this just for graphics? If yes I'd rather we put it in the graphics > driver. We can always move it later ... Wouldn't this be the API that audio virtualisation will have to use to share buffers between the host and any guests? Thanks Guennadi > > --- > > drivers/virtio/Makefile | 2 +- > > drivers/virtio/virtio.c | 6 +++ > > drivers/virtio/virtio_dma_buf.c | 89 +++++++++++++++++++++++++++++++++ > > include/linux/virtio.h | 1 + > > include/linux/virtio_dma_buf.h | 58 +++++++++++++++++++++ > > 5 files changed, 155 insertions(+), 1 deletion(-) > > create mode 100644 drivers/virtio/virtio_dma_buf.c > > create mode 100644 include/linux/virtio_dma_buf.h > > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile > > index 29a1386ecc03..ecdae5b596de 100644 > > --- a/drivers/virtio/Makefile > > +++ b/drivers/virtio/Makefile > > @@ -1,5 +1,5 @@ > > # SPDX-License-Identifier: GPL-2.0 > > -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o > > +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_dma_buf.o > > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o > > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o > > virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index a977e32a88f2..5d46f0ded92d 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -357,6 +357,12 @@ int register_virtio_device(struct virtio_device *dev) > > } > > EXPORT_SYMBOL_GPL(register_virtio_device); > > > > +bool is_virtio_device(struct device *dev) > > +{ > > + return dev->bus == &virtio_bus; > > +} > > +EXPORT_SYMBOL_GPL(is_virtio_device); > > + > > void unregister_virtio_device(struct virtio_device *dev) > > { > > int index = dev->index; /* save for after device release */ > > diff --git a/drivers/virtio/virtio_dma_buf.c b/drivers/virtio/virtio_dma_buf.c > > new file mode 100644 > > index 000000000000..23e3399b11ed > > --- /dev/null > > +++ b/drivers/virtio/virtio_dma_buf.c > > @@ -0,0 +1,89 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * dma-bufs for virtio exported objects > > + * > > + * Copyright (C) 2020 Google, Inc. > > + */ > > + > > +#include <linux/virtio_dma_buf.h> > > + > > +/** > > + * virtio_dma_buf_export - Creates a new dma-buf for a virtio exported object > > + * > > + * This wraps dma_buf_export() to allow virtio drivers to create a dma-buf > > + * for an virtio exported object that can be queried by other virtio drivers > > + * for the object's UUID. > > + */ > > +struct dma_buf *virtio_dma_buf_export( > > + const struct virtio_dma_buf_export_info *virtio_exp_info) > > +{ > > + struct dma_buf_export_info exp_info; > > + > > + if (!virtio_exp_info->ops > > + || virtio_exp_info->ops->ops.attach != &virtio_dma_buf_attach > > + || !virtio_exp_info->ops->get_uuid) { > > + return ERR_PTR(-EINVAL); > > + } > > + > > + exp_info.exp_name = virtio_exp_info->exp_name; > > + exp_info.owner = virtio_exp_info->owner; > > + exp_info.ops = &virtio_exp_info->ops->ops; > > + exp_info.size = virtio_exp_info->size; > > + exp_info.flags = virtio_exp_info->flags; > > + exp_info.resv = virtio_exp_info->resv; > > + exp_info.priv = virtio_exp_info->priv; > > + BUILD_BUG_ON(sizeof(struct virtio_dma_buf_export_info) > > + != sizeof(struct dma_buf_export_info)); > > This is the only part that gives me pause. Why do we need this hack? > What's wrong with just using dma_buf_export_info directly, > and if you want the virtio ops, just using container_off? > > > > > + > > + return dma_buf_export(&exp_info); > > +} > > +EXPORT_SYMBOL(virtio_dma_buf_export); > > + > > +/** > > + * virtio_dma_buf_attach - mandatory attach callback for virtio dma-bufs > > + */ > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf, > > + struct dma_buf_attachment *attach) > > +{ > > + int ret; > > + const struct virtio_dma_buf_ops *ops = container_of( > > + dma_buf->ops, const struct virtio_dma_buf_ops, ops); > > + > > + if (ops->device_attach) { > > + ret = ops->device_attach(dma_buf, attach); > > + if (ret) > > + return ret; > > + } > > + return 0; > > +} > > +EXPORT_SYMBOL(virtio_dma_buf_attach); > > + > > +/** > > + * is_virtio_dma_buf - returns true if the given dma-buf is a virtio dma-buf > > + * @dma_buf: buffer to query > > + */ > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf) > > +{ > > + return dma_buf->ops->attach == &virtio_dma_buf_attach; > > +} > > +EXPORT_SYMBOL(is_virtio_dma_buf); > > + > > +/** > > + * virtio_dma_buf_get_uuid - gets the uuid of the virtio dma-buf's exported object > > + * @dma_buf: [in] buffer to query > > + * @uuid: [out] the uuid > > + * > > + * Returns: 0 on success, negative on failure. > > + */ > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, > > + uuid_t *uuid) > > +{ > > + const struct virtio_dma_buf_ops *ops = container_of( > > + dma_buf->ops, const struct virtio_dma_buf_ops, ops); > > + > > + if (!is_virtio_dma_buf(dma_buf)) > > + return -EINVAL; > > + > > + return ops->get_uuid(dma_buf, uuid); > > +} > > +EXPORT_SYMBOL(virtio_dma_buf_get_uuid); > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index 15f906e4a748..9397e25616c4 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -128,6 +128,7 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev) > > void virtio_add_status(struct virtio_device *dev, unsigned int status); > > int register_virtio_device(struct virtio_device *dev); > > void unregister_virtio_device(struct virtio_device *dev); > > +bool is_virtio_device(struct device *dev); > > > > void virtio_break_device(struct virtio_device *dev); > > > > diff --git a/include/linux/virtio_dma_buf.h b/include/linux/virtio_dma_buf.h > > new file mode 100644 > > index 000000000000..29fee167afbd > > --- /dev/null > > +++ b/include/linux/virtio_dma_buf.h > > @@ -0,0 +1,58 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * dma-bufs for virtio exported objects > > + * > > + * Copyright (C) 2020 Google, Inc. > > + */ > > + > > +#ifndef _LINUX_VIRTIO_DMA_BUF_H > > +#define _LINUX_VIRTIO_DMA_BUF_H > > + > > +#include <linux/dma-buf.h> > > +#include <linux/uuid.h> > > +#include <linux/virtio.h> > > + > > +/** > > + * struct virtio_dma_buf_ops - operations possible on exported object dma-buf > > + * @ops: the base dma_buf_ops. ops.attach MUST be virtio_dma_buf_attach. > > + * @device_attach: [optional] callback invoked by virtio_dma_buf_attach during > > + * all attach operations. > > + * @get_uid: [required] callback to get the uuid of the exported object. > > + */ > > +struct virtio_dma_buf_ops { > > + struct dma_buf_ops ops; > > + int (*device_attach)(struct dma_buf *dma_buf, > > + struct dma_buf_attachment *attach); > > + int (*get_uuid)(struct dma_buf *dma_buf, uuid_t *uuid); > > +}; > > + > > +/** > > + * struct virtio_dma_buf_export_info - see struct dma_buf_export_info > > + */ > > +struct virtio_dma_buf_export_info { > > + const char *exp_name; > > + struct module *owner; > > + const struct virtio_dma_buf_ops *ops; > > + size_t size; > > + int flags; > > + struct dma_resv *resv; > > + void *priv; > > +}; > > + > > +/** > > + * DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO - helper macro for exporters > > + */ > > +#define DEFINE_VIRTIO_DMA_BUF_EXPORT_INFO(name) \ > > + struct virtio_dma_buf_export_info name = { \ > > + .exp_name = KBUILD_MODNAME, \ > > + .owner = THIS_MODULE } > > + > > +int virtio_dma_buf_attach(struct dma_buf *dma_buf, > > + struct dma_buf_attachment *attach); > > + > > +struct dma_buf *virtio_dma_buf_export( > > + const struct virtio_dma_buf_export_info *virtio_exp_info); > > +bool is_virtio_dma_buf(struct dma_buf *dma_buf); > > +int virtio_dma_buf_get_uuid(struct dma_buf *dma_buf, uuid_t *uuid); > > + > > +#endif /* _LINUX_VIRTIO_DMA_BUF_H */ > > -- > > 2.27.0.rc0.183.gde8f92d652-goog > > _______________________________________________ > Virtualization mailing list > Virtualization(a)lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization

5 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig