- Linaro-mm-sig - lists.linaro.org

[PATCH AUTOSEL 5.14 001/138] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 511fe0d217a08..733c8b1c8467c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

4 years, 1 month

1
1
0 0

[PATCH AUTOSEL 5.15 001/146] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 63d32261b63ff..474de2d988ca7 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -82,6 +82,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

4 years, 1 month

1
1
0 0

[PATCH] dma-buf/poll: Get a file reference for outstanding fence callbacks

by Michel Dänzer

From: Michel Dänzer <mdaenzer(a)redhat.com> This makes sure we don't hit the BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); in dma_buf_release, which could be triggered by user space closing the dma-buf file description while there are outstanding fence callbacks from dma_buf_poll. Cc: stable(a)vger.kernel.org Signed-off-by: Michel Dänzer <mdaenzer(a)redhat.com> --- drivers/dma-buf/dma-buf.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 6c520c9bd93c..ec25498a971f 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -65,12 +65,9 @@ static void dma_buf_release(struct dentry *dentry) BUG_ON(dmabuf->vmapping_counter); /* - * Any fences that a dma-buf poll can wait on should be signaled - * before releasing dma-buf. This is the responsibility of each - * driver that uses the reservation objects. - * - * If you hit this BUG() it means someone dropped their ref to the - * dma-buf while still having pending operation to the buffer. + * If you hit this BUG() it could mean: + * * There's a file reference imbalance in dma_buf_poll / dma_buf_poll_cb or somewhere else + * * dmabuf->cb_in/out.active are non-0 despite no pending fence callback */ BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); @@ -196,6 +193,7 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) { struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb; + struct dma_buf *dmabuf = container_of(dcb->poll, struct dma_buf, poll); unsigned long flags; spin_lock_irqsave(&dcb->poll->lock, flags); @@ -203,6 +201,8 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) dcb->active = 0; spin_unlock_irqrestore(&dcb->poll->lock, flags); dma_fence_put(fence); + /* Paired with get_file in dma_buf_poll */ + fput(dmabuf->file); } static bool dma_buf_poll_shared(struct dma_resv *resv, @@ -278,6 +278,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLOUT) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_shared(resv, dcb) && !dma_buf_poll_excl(resv, dcb)) /* No callback queued, wake up any other waiters */ @@ -299,6 +302,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLIN) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_excl(resv, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); -- 2.32.0

4 years, 1 month

4
7
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 29.10.21 um 04:15 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > On Fri, 2021-10-08 at 12:24 +0200, Christian König wrote: >> Am 08.10.21 um 09:54 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> Because dma-buf.name can be freed in func: "dma_buf_set_name", >>> so, we need to acquire lock first before we read/write dma_buf.name >>> to prevent Use After Free(UAF) issue. >>> >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >> Reviewed-by: Christian König <christian.koenig(a)amd.com> >> >> Going to push that upstream if nobody else objects. >> >> Thanks, >> Christian. > Just a gentle ping for this patch, please kindly let me know how is it going. Ah, yes. Thanks for the reminder. I've just pushed this to drm-misc-fixes. Christian. > >>> --- >>> drivers/dma-buf/dma-buf.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 511fe0d217a0..a7f6fd13a635 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -1372,6 +1372,8 @@ static int dma_buf_debug_show(struct seq_file >>> *s, void *unused) >>> if (ret) >>> goto error_unlock; >>> >>> + >>> + spin_lock(&buf_obj->name_lock); >>> seq_printf(s, >>> "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", >>> buf_obj->size, >>> buf_obj->file->f_flags, buf_obj->file- >>>> f_mode, >>> @@ -1379,6 +1381,7 @@ static int dma_buf_debug_show(struct seq_file >>> *s, void *unused) >>> buf_obj->exp_name, >>> file_inode(buf_obj->file)->i_ino, >>> buf_obj->name ?: ""); >>> + spin_unlock(&buf_obj->name_lock); >>> >>> robj = buf_obj->resv; >>> fence = dma_resv_excl_fence(robj); >> >> _______________________________________________ >> Linux-mediatek mailing list >> Linux-mediatek(a)lists.infradead.org >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr…

4 years, 1 month

1
0
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi guys, a few more bug fixes, looks like the more selftests I add the more odies I find. Assuming the CI tests now pass I will start pushing patches I've already got an rb for to drm-misc-next. Please review and/or comment, Christian.

4 years, 1 month

5
61
0 0

Re: [Linaro-mm-sig] [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Christian König

Am 26.10.21 um 13:52 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > On Tue, 2021-10-26 at 13:18 +0200, Christian König wrote: >> Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> In this patch(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0), >>> it add a new IOCTL to support dma-buf user to set debug name. >>> >>> But it also added a limitation of this IOCTL, it needs the >>> attachments of dmabuf should be empty, otherwise it will fail. >>> >>> For the original series, the idea was that allowing name change >>> mid-use could confuse the users about the dma-buf. >>> However, the rest of the series also makes sure each dma-buf have a >>> unique >>> inode(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0), and any >>> accounting >>> should probably use that, without relying on the name as much. >>> >>> So, removing this restriction will let dma-buf userspace users to >>> use it >>> more comfortably and without any side effect. >>> >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >> We could now cleanup the return value from dma_buf_set_name() into a >> void since that function can't fail any more as far as I can see. >> >> But that isn't mandatory I think, patch is Reviewed-by: Christian >> König >> <christian.koenig(a)amd.com> >> > So, here is no need to check return value of 'strndup_user', > just return without error code if the almost impossible error occurs? Good point, totally missed that one. In that case I'm going to push the patch to drm-misc-next as is. Regards, Christian. > > Guangming. > >> Regards, >> Christian. >> >>> --- >>> drivers/dma-buf/dma-buf.c | 17 +++-------------- >>> 1 file changed, 3 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 511fe0d217a0..5fbb3a2068a3 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file >>> *file, poll_table *poll) >>> >>> /** >>> * dma_buf_set_name - Set a name to a specific dma_buf to track >>> the usage. >>> - * The name of the dma-buf buffer can only be set when the dma-buf >>> is not >>> - * attached to any devices. It could theoritically support >>> changing the >>> - * name of the dma-buf if the same piece of memory is used for >>> multiple >>> - * purpose between different devices. >>> + * It could support changing the name of the dma-buf if the same >>> + * piece of memory is used for multiple purpose between different >>> devices. >>> * >>> * @dmabuf: [in] dmabuf buffer that will be renamed. >>> * @buf: [in] A piece of userspace memory that contains >>> the name of >>> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file >>> *file, poll_table *poll) >>> static long dma_buf_set_name(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); >>> >>> - dma_resv_lock(dmabuf->resv, NULL); >>> - if (!list_empty(&dmabuf->attachments)) { >>> - ret = -EBUSY; >>> - 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); >>> - return ret; >>> + return 0; >>> } >>> >>> static long dma_buf_ioctl(struct file *file, >>

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Christian König

Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > In this patch(https://patchwork.freedesktop.org/patch/310349), > it add a new IOCTL to support dma-buf user to set debug name. > > But it also added a limitation of this IOCTL, it needs the > attachments of dmabuf should be empty, otherwise it will fail. > > For the original series, the idea was that allowing name change > mid-use could confuse the users about the dma-buf. > However, the rest of the series also makes sure each dma-buf have a unique > inode(https://patchwork.freedesktop.org/patch/310387/), and any accounting > should probably use that, without relying on the name as much. > > So, removing this restriction will let dma-buf userspace users to use it > more comfortably and without any side effect. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> We could now cleanup the return value from dma_buf_set_name() into a void since that function can't fail any more as far as I can see. But that isn't mandatory I think, patch is Reviewed-by: Christian König <christian.koenig(a)amd.com> Regards, Christian. > --- > drivers/dma-buf/dma-buf.c | 17 +++-------------- > 1 file changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..5fbb3a2068a3 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > > /** > * dma_buf_set_name - Set a name to a specific dma_buf to track the usage. > - * The name of the dma-buf buffer can only be set when the dma-buf is not > - * attached to any devices. It could theoritically support changing the > - * name of the dma-buf if the same piece of memory is used for multiple > - * purpose between different devices. > + * It could support changing the name of the dma-buf if the same > + * piece of memory is used for multiple purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > * @buf: [in] A piece of userspace memory that contains the name of > @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > static long dma_buf_set_name(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); > > - dma_resv_lock(dmabuf->resv, NULL); > - if (!list_empty(&dmabuf->attachments)) { > - ret = -EBUSY; > - 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); > - return ret; > + return 0; > } > > static long dma_buf_ioctl(struct file *file,

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: st: fix error handling in test_get_fences()

by Christian König

Am 26.10.21 um 10:34 schrieb Arnd Bergmann: > From: Arnd Bergmann <arnd(a)arndb.de> > > The new driver incorrectly unwinds after errors, as clang points out: > > drivers/dma-buf/st-dma-resv.c:295:7: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (r) { > ^ > drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here > while (i--) > ^ > drivers/dma-buf/st-dma-resv.c:295:3: note: remove the 'if' if its condition is always false > if (r) { > ^~~~~~~~ > drivers/dma-buf/st-dma-resv.c:288:6: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (r) { > ^ > drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here > while (i--) > ^ > drivers/dma-buf/st-dma-resv.c:288:2: note: remove the 'if' if its condition is always false > if (r) { > ^~~~~~~~ > drivers/dma-buf/st-dma-resv.c:280:10: note: initialize the variable 'i' to silence this warning > int r, i; > ^ > = 0 > > Skip cleaning up the bits that have not been allocated at this point. > > Fixes: 1d51775cd3f5 ("dma-buf: add dma_resv selftest v4") > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> I already send out a patch to fix this up, but forgot to fix both gotos. Going to add my rb and using that one here instead. Thanks, Christian. > --- > I'm not familiar with these interfaces, so I'm just guessing where > we should jump after an error, please double-check and fix if necessary. > --- > drivers/dma-buf/st-dma-resv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c > index 6f3ba756da3e..bc32b3eedcb6 100644 > --- a/drivers/dma-buf/st-dma-resv.c > +++ b/drivers/dma-buf/st-dma-resv.c > @@ -287,7 +287,7 @@ static int test_get_fences(void *arg, bool shared) > r = dma_resv_lock(&resv, NULL); > if (r) { > pr_err("Resv locking failed\n"); > - goto err_free; > + goto err_resv; > } > > if (shared) { > @@ -295,7 +295,7 @@ static int test_get_fences(void *arg, bool shared) > if (r) { > pr_err("Resv shared slot allocation failed\n"); > dma_resv_unlock(&resv); > - goto err_free; > + goto err_resv; > } > > dma_resv_add_shared_fence(&resv, f); > @@ -336,6 +336,7 @@ static int test_get_fences(void *arg, bool shared) > while (i--) > dma_fence_put(fences[i]); > kfree(fences); > +err_resv: > dma_resv_fini(&resv); > dma_fence_put(f); > return r;

4 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [RFC 06/13] soc: mediatek: apu: Add apu core driver

by Randy Dunlap

Hi, On 10/23/21 4:14 AM, Flora Fu wrote: > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index d9bac2710494..074b0cf24c44 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -24,6 +24,24 @@ config MTK_APU_PM > APU power domain shall be enabled before accessing the > internal sub modules. > > +config MTK_APU > + tristate "MediaTek APUSYS Support" > + select REGMAP > + select MTK_APU_PM > + select MTK_SCP > + help > + Say yes here to add support for the APU tinysys. The tinsys is tinysys runs on > + running on a micro processor in APU. a microprocessor in the APU. > + Its firmware is load and boot from Kernel side. Kernel and tinysys use is loaded and booted > + IPI to tx/rx messages. to send/receive messages. > + > +config MTK_APU_DEBUG > + tristate "MediaTek APUSYS debug functions" > + depends on MTK_APU > + help > + Say yes here to enalbe debug on APUSYS. enable > + Disable it if you don't need them. -- ~Randy

4 years, 1 month

1
0
0 0

[PATCH 0/2] dma-buf: Fix breakages from dma_resv_iter conversion.

by Maarten Lankhorst

Urgent fixes! dma_resv_test_signaled is completely broken, dma_resv_wait_timeout kind of broken. Maarten Lankhorst (2): dma-buf: Fix dma_resv_wait_timeout handling of timeout = 0. dma-buf: Fix dma_resv_test_signaled. drivers/dma-buf/dma-resv.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) -- 2.33.0

4 years, 1 month

4
7
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: add attachments empty check for dma_buf_release

by Daniel Vetter

On Tue, Oct 19, 2021 at 08:23:45PM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Since there is no mandatory inspection for attachments in dma_buf_release. > There will be a case that dma_buf already released but attachment is still > in use, which can points to the dmabuf, and it maybe cause > some unexpected issues. > > With IOMMU, when this cases occurs, there will have IOMMU address > translation fault(s) followed by this warning, > I think it's useful for dma devices to debug issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> This feels a lot like hand-rolling kobject debugging. If you want to do this then I think adding kobject debug support to dma_buf/dma_buf_attachment would be better than hand-rolling something bespoke here. Also on the patch itself: You don't need the trylock. For correctly working code non one else can get at the dma-buf, so no locking needed to iterate through the attachment list. For incorrect code the kernel will be on fire pretty soon anyway, trying to do locking won't help :-) And without the trylock we can catch more bugs (e.g. if you also forgot to unlock and not just forgot to detach). -Daniel > --- > drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..672404857d6a 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -74,6 +74,29 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + /* attachment check */ > + if (dma_resv_trylock(dmabuf->resv) && WARN(!list_empty(&dmabuf->attachments), > + "%s err, inode:%08lu size:%08zu name:%s exp_name:%s flags:0x%08x mode:0x%08x, %s\n", > + __func__, file_inode(dmabuf->file)->i_ino, dmabuf->size, > + dmabuf->name, dmabuf->exp_name, > + dmabuf->file->f_flags, dmabuf->file->f_mode, > + "Release dmabuf before detach all attachments, dump attach:\n")) { > + int attach_cnt = 0; > + dma_addr_t dma_addr; > + struct dma_buf_attachment *attach_obj; > + /* dump all attachment info */ > + list_for_each_entry(attach_obj, &dmabuf->attachments, node) { > + dma_addr = (dma_addr_t)0; > + if (attach_obj->sgt) > + dma_addr = sg_dma_address(attach_obj->sgt->sgl); > + pr_err("attach[%d]: dev:%s dma_addr:0x%-12lx\n", > + attach_cnt, dev_name(attach_obj->dev), dma_addr); > + attach_cnt++; > + } > + pr_err("Total %d devices attached\n\n", attach_cnt); > + dma_resv_unlock(dmabuf->resv); > + } > + > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 1 month

2
2
0 0

Re: [Linaro-mm-sig] [PATCH 16/28] drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2

by Christian König

Thanks for the notice. Going to take a deeper look into this tomorrow. Basically looks like we messed up the fence ref count somehow. Thanks, Christian. Am 17.10.21 um 16:40 schrieb Nicolas Frattaroli: > On Dienstag, 5. Oktober 2021 13:37:30 CEST Christian König wrote: >> Simplifying the code a bit. >> >> v2: use dma_resv_for_each_fence >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >> --- >> drivers/gpu/drm/scheduler/sched_main.c | 26 ++++++-------------------- >> 1 file changed, 6 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/scheduler/sched_main.c >> b/drivers/gpu/drm/scheduler/sched_main.c index 042c16b5d54a..5bc5f775abe1 >> 100644 >> --- a/drivers/gpu/drm/scheduler/sched_main.c >> +++ b/drivers/gpu/drm/scheduler/sched_main.c >> @@ -699,30 +699,16 @@ int drm_sched_job_add_implicit_dependencies(struct >> drm_sched_job *job, struct drm_gem_object *obj, >> bool write) >> { >> + struct dma_resv_iter cursor; >> + struct dma_fence *fence; >> 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_sched_job_add_dependency(job, 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_sched_job_add_dependency(job, fences[i]); >> + dma_resv_for_each_fence(&cursor, obj->resv, write, fence) { >> + ret = drm_sched_job_add_dependency(job, fence); >> if (ret) >> - break; >> + return ret; >> } >> - >> - for (; i < fence_count; i++) >> - dma_fence_put(fences[i]); >> - kfree(fences); >> - return ret; >> + return 0; >> } >> EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies); > Hi Christian, > > unfortunately, this breaks lima on the rk3328 quite badly. Running glmark2- > es2-drm just locks up the device with the following traces: > > [ 39.624100] ------------[ cut here ]------------ > [ 39.624555] refcount_t: addition on 0; use-after-free. > [ 39.625058] WARNING: CPU: 0 PID: 123 at lib/refcount.c:25 > refcount_warn_saturate+0xa4/0x150 > [ 39.625825] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.626753] CPU: 0 PID: 123 Comm: pp Tainted: G C 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.627614] Hardware name: Pine64 Rock64 (DT) > [ 39.628004] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.628628] pc : refcount_warn_saturate+0xa4/0x150 > [ 39.629062] lr : refcount_warn_saturate+0xa4/0x150 > [ 39.629495] sp : ffffffc0124d3d90 > [ 39.629794] x29: ffffffc0124d3d90 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.630441] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff8001ad73f8 > [ 39.631087] x23: ffffffc0107fc3e0 x22: ffffffc0117fe000 x21: > ffffff8010660000 > [ 39.631731] x20: ffffff8001ad73c0 x19: ffffff807db094c8 x18: > ffffffffffffffff > [ 39.632377] x17: 0000000000000001 x16: 0000000000000001 x15: > 0765076507720766 > [ 39.633022] x14: 072d077207650774 x13: 0765076507720766 x12: > 072d077207650774 > [ 39.633668] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.634314] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.634960] x5 : ffffff807fb4d980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.635605] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8000e1f000 > [ 39.636250] Call trace: > [ 39.636475] refcount_warn_saturate+0xa4/0x150 > [ 39.636879] drm_sched_entity_pop_job+0x414/0x4a0 > [ 39.637307] drm_sched_main+0xe4/0x450 > [ 39.637651] kthread+0x12c/0x140 > [ 39.637949] ret_from_fork+0x10/0x20 > [ 39.638279] ---[ end trace 47528e09b2512330 ]--- > [ 39.638783] ------------[ cut here ]------------ > [ 39.639214] refcount_t: underflow; use-after-free. > [ 39.639687] WARNING: CPU: 0 PID: 123 at lib/refcount.c:28 > refcount_warn_saturate+0xf8/0x150 > [ 39.640447] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.641373] CPU: 0 PID: 123 Comm: pp Tainted: G WC 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.642237] Hardware name: Pine64 Rock64 (DT) > [ 39.642632] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.643257] pc : refcount_warn_saturate+0xf8/0x150 > [ 39.643693] lr : refcount_warn_saturate+0xf8/0x150 > [ 39.644128] sp : ffffffc0124d3d90 > [ 39.644430] x29: ffffffc0124d3d90 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.645077] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff8001ad73f8 > [ 39.645724] x23: ffffffc0107fc3e0 x22: ffffffc0117fe000 x21: > ffffff8010660000 > [ 39.646372] x20: ffffff8001ad73c0 x19: ffffff807db094c8 x18: > ffffffffffffffff > [ 39.647020] x17: 0000000000000001 x16: 0000000000000001 x15: > 072007200720072e > [ 39.647666] x14: 0765076507720766 x13: 072007200720072e x12: > 0765076507720766 > [ 39.648312] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.648957] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.649602] x5 : ffffff807fb4d980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.650247] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8000e1f000 > [ 39.650894] Call trace: > [ 39.651119] refcount_warn_saturate+0xf8/0x150 > [ 39.651526] drm_sched_entity_pop_job+0x420/0x4a0 > [ 39.651953] drm_sched_main+0xe4/0x450 > [ 39.652296] kthread+0x12c/0x140 > [ 39.652595] ret_from_fork+0x10/0x20 > [ 39.652924] ---[ end trace 47528e09b2512331 ]--- > [ 39.717053] ------------[ cut here ]------------ > [ 39.717543] refcount_t: saturated; leaking memory. > [ 39.718030] WARNING: CPU: 1 PID: 375 at lib/refcount.c:22 > refcount_warn_saturate+0x78/0x150 > [ 39.718800] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.719744] CPU: 1 PID: 375 Comm: glmark2-es2-drm Tainted: G WC > 5.15.0-rc1fratti-00251-g9c2ba265352a #158 > [ 39.720712] Hardware name: Pine64 Rock64 (DT) > [ 39.721109] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.721739] pc : refcount_warn_saturate+0x78/0x150 > [ 39.722178] lr : refcount_warn_saturate+0x78/0x150 > [ 39.722617] sp : ffffffc012913a90 > [ 39.722921] x29: ffffffc012913a90 x28: ffffff8010630000 x27: > ffffff8005219e00 > [ 39.723576] x26: ffffff80103da500 x25: 0000000000000000 x24: > ffffff8000cb24c0 > [ 39.724230] x23: ffffff800ac045b0 x22: ffffff8005212100 x21: > 0000000000000000 > [ 39.724884] x20: ffffff8000cb24c0 x19: 0000000000000000 x18: > ffffffffffffffff > [ 39.725538] x17: 0000000000000000 x16: 0000000000000000 x15: > 072007200720072e > [ 39.726192] x14: 07790772076f076d x13: 072007200720072e x12: > 07790772076f076d > [ 39.726846] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.727501] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.728155] x5 : ffffff807fb68980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.728808] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8004d7b800 > [ 39.729464] Call trace: > [ 39.729691] refcount_warn_saturate+0x78/0x150 > [ 39.730101] dma_resv_add_shared_fence+0x1ac/0x1cc > [ 39.730543] lima_gem_submit+0x300/0x580 > [ 39.730909] lima_ioctl_gem_submit+0x284/0x340 > [ 39.731318] drm_ioctl_kernel+0xd0/0x180 > [ 39.731685] drm_ioctl+0x220/0x450 > [ 39.732005] __arm64_sys_ioctl+0x568/0xe9c > [ 39.732386] invoke_syscall.constprop.0+0x58/0xf0 > [ 39.732824] do_el0_svc+0x138/0x170 > [ 39.733152] el0_svc+0x28/0xc0 > [ 39.733441] el0t_64_sync_handler+0xa8/0x130 > [ 39.733837] el0t_64_sync+0x1a0/0x1a4 > [ 39.734178] ---[ end trace 47528e09b2512332 ]--- > [ 39.734926] Unable to handle kernel write to read-only memory at virtual > address ffffffc0107fbc70 > [ 39.735763] Mem abort info: > [ 39.736029] ESR = 0x9600004e > [ 39.736313] EC = 0x25: DABT (current EL), IL = 32 bits > [ 39.736796] SET = 0, FnV = 0 > [ 39.737080] EA = 0, S1PTW = 0 > [ 39.737368] FSC = 0x0e: level 2 permission fault > [ 39.737804] Data abort info: > [ 39.738068] ISV = 0, ISS = 0x0000004e > [ 39.738419] CM = 0, WnR = 1 > [ 39.738693] swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000003893000 > [ 39.739297] [ffffffc0107fbc70] pgd=100000007ffff003, p4d=100000007ffff003, > pud=100000007ffff003, pmd=0040000002800781 > [ 39.740270] Internal error: Oops: 9600004e [#1] SMP > [ 39.740719] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.741665] CPU: 0 PID: 123 Comm: pp Tainted: G WC 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.742537] Hardware name: Pine64 Rock64 (DT) > [ 39.742934] pstate: 000000c5 (nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.743570] pc : dma_fence_add_callback+0xb0/0xf0 > [ 39.744017] lr : dma_fence_add_callback+0x5c/0xf0 > [ 39.744457] sp : ffffffc0124d3d60 > [ 39.744764] x29: ffffffc0124d3d60 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.745423] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff800536b6e0 > [ 39.746080] x23: 0000000000000000 x22: 0000000000000000 x21: > ffffffc0107fc3e0 > [ 39.746736] x20: ffffff807db09528 x19: ffffff8000cb24c0 x18: > 0000000000000001 > [ 39.747390] x17: 000000040044ffff x16: 000000000000000c x15: > 000000000000000d > [ 39.748044] x14: 0000000000000000 x13: 000000000000072b x12: > 071c71c71c71c71c > [ 39.748697] x11: 000000000000072b x10: 0000000000000002 x9 : > ffffffc01087d5ac > [ 39.749350] x8 : 0000000000000238 x7 : 0000000000000000 x6 : > 0000000000000000 > [ 39.750002] x5 : 0000000000000000 x4 : 0000000000000000 x3 : > ffffff8000cb24f0 > [ 39.750654] x2 : 0000000000000000 x1 : ffffffc0107fbc70 x0 : > ffffff8000cb24d0 > [ 39.751309] Call trace: > [ 39.751539] dma_fence_add_callback+0xb0/0xf0 > [ 39.751944] drm_sched_entity_pop_job+0xac/0x4a0 > [ 39.752371] drm_sched_main+0xe4/0x450 > [ 39.752720] kthread+0x12c/0x140 > [ 39.753024] ret_from_fork+0x10/0x20 > [ 39.753367] Code: 91004260 f9400e61 f9000e74 a9000680 (f9000034) > [ 39.753920] ---[ end trace 47528e09b2512333 ]--- > [ 40.253374] [drm:lima_sched_timedout_job] *ERROR* lima job timeout > > I've bisected the problem to this commit, and confirmed that reverting it gets > glmark2's 3d horse back to spinning. > > It's possible this patch just uncovers a bug in lima, so I've added the lima > list as a recipient to this reply as well. > > Since I doubt AMD has many Rockchip SoCs laying about, I'll gladly test any > prospective fixes for this. > > Regards, > Nicolas Frattaroli > >

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Update obsoluted comments on dma_buf_vmap/vunmap()

by Christian König

Am 08.10.21 um 13:20 schrieb Shunsuke Mie: > A comment for the dma_buf_vmap/vunmap() is not catching up a > corresponding implementation. > > Signed-off-by: Shunsuke Mie <mie(a)igel.co.jp> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-buf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index beb504a92d60..7b619998f03a 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1052,8 +1052,8 @@ EXPORT_SYMBOL_GPL(dma_buf_move_notify); > * > * Interfaces:: > * > - * void \*dma_buf_vmap(struct dma_buf \*dmabuf) > - * void dma_buf_vunmap(struct dma_buf \*dmabuf, void \*vaddr) > + * void \*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) > * > * The vmap call can fail if there is no vmap support in the exporter, or if > * it runs out of vmalloc space. Note that the dma-buf layer keeps a reference

4 years, 2 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Sumit Semwal

Hello Guangming, Christian, On Tue, 12 Oct 2021, 14:09 , <guangming.cao(a)mediatek.com> wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > > Am 09.10.21 um 07:55 schrieb guangming.cao(a)mediatek.com: > > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > > > > > If dma-buf don't want userspace users to touch the dmabuf buffer, > > > it seems we should add this restriction into dma_buf_ops.mmap, > > > not in this IOCTL:DMA_BUF_SET_NAME. > > > > > > With this restriction, we can only know the kernel users of the dmabuf > > > by attachments. > > > However, for many userspace users, such as userpsace users of dma_heap, > > > they also need to mark the usage of dma-buf, and they don't care about > > > who attached to this dmabuf, and seems it's no meaning to be waiting > for > > > IOCTL:DMA_BUF_SET_NAME rather than mmap. > > > > Sounds valid to me, but I have no idea why this restriction was added in > > the first place. > > > > Can you double check the git history and maybe identify when that was > > added? Mentioning this change in the commit message then might make > > things a bit easier to understand. > > > > Thanks, > > Christian. > It was add in this patch: https://patchwork.freedesktop.org/patch/310349/. > However, there is no illustration about it. > I guess it wants users to set_name when no attachments on the dmabuf, > for case with attachments, we can find owner by device in attachments. > But just I said in commit message, this is might not a good idea. > > Do you have any idea? > For the original series, the idea was that allowing name change mid-use could confuse the users about the dma-buf. However, the rest of the series also makes sure each dma-buf have a unique inode, and any accounting should probably use that, without relying on the name as much. So I don't have an objection to this change. Best, Sumit. > > > > > > > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > > > --- > > > drivers/dma-buf/dma-buf.c | 14 ++------------ > > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > index 511fe0d217a0..db2f4efdec32 100644 > > > --- a/drivers/dma-buf/dma-buf.c > > > +++ b/drivers/dma-buf/dma-buf.c > > > @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, > poll_table *poll) > > > > > > /** > > > * dma_buf_set_name - Set a name to a specific dma_buf to track the > usage. > > > - * The name of the dma-buf buffer can only be set when the dma-buf is > not > > > - * attached to any devices. It could theoritically support changing > the > > > - * name of the dma-buf if the same piece of memory is used for > multiple > > > - * purpose between different devices. > > > + * It could theoretically support changing the name of the dma-buf if > the same > > > + * piece of memory is used for multiple purpose between different > devices. > > > * > > > * @dmabuf: [in] dmabuf buffer that will be renamed. > > > * @buf: [in] A piece of userspace memory that contains the > name of > > > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf > *dmabuf, const char __user *buf) > > > if (IS_ERR(name)) > > > return PTR_ERR(name); > > > > > > - dma_resv_lock(dmabuf->resv, NULL); > > > - if (!list_empty(&dmabuf->attachments)) { > > > - ret = -EBUSY; > > > - 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); > > > return ret; > > > } > > > >

4 years, 2 months

2
1
0 0

Re: [Linaro-mm-sig] [RFC PATCH v2 2/2] RDMA/rxe: Add dma-buf support

by Daniel Vetter

On Fri, Oct 01, 2021 at 12:56:48PM +0900, Shunsuke Mie wrote: > 2021年9月30日(木) 23:41 Daniel Vetter <daniel(a)ffwll.ch>: > > > > On Wed, Sep 29, 2021 at 01:19:05PM +0900, Shunsuke Mie wrote: > > > Implement a ib device operation ‘reg_user_mr_dmabuf’. Generate a > > > rxe_map from the memory space linked the passed dma-buf. > > > > > > Signed-off-by: Shunsuke Mie <mie(a)igel.co.jp> > > > --- > > > drivers/infiniband/sw/rxe/rxe_loc.h | 2 + > > > drivers/infiniband/sw/rxe/rxe_mr.c | 118 ++++++++++++++++++++++++++ > > > drivers/infiniband/sw/rxe/rxe_verbs.c | 34 ++++++++ > > > drivers/infiniband/sw/rxe/rxe_verbs.h | 2 + > > > 4 files changed, 156 insertions(+) > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h > > > index 1ca43b859d80..8bc19ea1a376 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_loc.h > > > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h > > > @@ -75,6 +75,8 @@ u8 rxe_get_next_key(u32 last_key); > > > void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr); > > > int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > > > int access, struct rxe_mr *mr); > > > +int rxe_mr_dmabuf_init_user(struct rxe_pd *pd, int fd, u64 start, u64 length, > > > + u64 iova, int access, struct rxe_mr *mr); > > > int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr); > > > int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length, > > > enum rxe_mr_copy_dir dir); > > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > > > index 53271df10e47..af6ef671c3a5 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > > > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > > > @@ -4,6 +4,7 @@ > > > * Copyright (c) 2015 System Fabric Works, Inc. All rights reserved. > > > */ > > > > > > +#include <linux/dma-buf.h> > > > #include "rxe.h" > > > #include "rxe_loc.h" > > > > > > @@ -245,6 +246,120 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > > > return err; > > > } > > > > > > +static int rxe_map_dmabuf_mr(struct rxe_mr *mr, > > > + struct ib_umem_dmabuf *umem_dmabuf) > > > +{ > > > + struct rxe_map_set *set; > > > + struct rxe_phys_buf *buf = NULL; > > > + struct rxe_map **map; > > > + void *vaddr, *vaddr_end; > > > + int num_buf = 0; > > > + int err; > > > + size_t remain; > > > + > > > + mr->dmabuf_map = kzalloc(sizeof &mr->dmabuf_map, GFP_KERNEL); > > > > dmabuf_maps are just tagged pointers (and we could shrink them to actually > > just a tagged pointer if anyone cares about the overhead of the separate > > bool), allocating them seperately is overkill. > > I agree with you. However, I think it is needed to unmap by > dma_buf_vunmap(). If there is another simple way to unmap it. It is not > needed I think. What do you think about it? dma_buf_vunmap does not kfree the dma_buf_map argument, so that's no reason to allocate it separately. Or I'm confused. Also apologies, I'm way behind on mails. -Daniel > > > > + if (!mr->dmabuf_map) { > > > + err = -ENOMEM; > > > + goto err_out; > > > + } > > > + > > > + err = dma_buf_vmap(umem_dmabuf->dmabuf, mr->dmabuf_map); > > > + if (err) > > > + goto err_free_dmabuf_map; > > > + > > > + set = mr->cur_map_set; > > > + set->page_shift = PAGE_SHIFT; > > > + set->page_mask = PAGE_SIZE - 1; > > > + > > > + map = set->map; > > > + buf = map[0]->buf; > > > + > > > + vaddr = mr->dmabuf_map->vaddr; > > > > dma_buf_map can be an __iomem too, you shouldn't dig around in this, but > > use the dma-buf-map.h helpers instead. On x86 (and I think also on most > > arm) it doesn't matter, but it's kinda not very nice in a pure software > > driver. > > > > If anything is missing in dma-buf-map.h wrappers just add more. > > > > Or alternatively you need to fail the import if you can't handle __iomem. > > > > Aside from these I think the dma-buf side here for cpu access looks > > reasonable now. > > -Daniel > I'll see the dma-buf-map.h and consider the error handling that you suggested. > I appreciate your support. > > Thanks a lot, > Shunsuke. > > > > + vaddr_end = vaddr + umem_dmabuf->dmabuf->size; > > > + remain = umem_dmabuf->dmabuf->size; > > > + > > > + for (; remain; vaddr += PAGE_SIZE) { > > > + if (num_buf >= RXE_BUF_PER_MAP) { > > > + map++; > > > + buf = map[0]->buf; > > > + num_buf = 0; > > > + } > > > + > > > + buf->addr = (uintptr_t)vaddr; > > > + if (remain >= PAGE_SIZE) > > > + buf->size = PAGE_SIZE; > > > + else > > > + buf->size = remain; > > > + remain -= buf->size; > > > + > > > + num_buf++; > > > + buf++; > > > + } > > > + > > > + return 0; > > > + > > > +err_free_dmabuf_map: > > > + kfree(mr->dmabuf_map); > > > +err_out: > > > + return err; > > > +} > > > + > > > +static void rxe_unmap_dmabuf_mr(struct rxe_mr *mr) > > > +{ > > > + struct ib_umem_dmabuf *umem_dmabuf = to_ib_umem_dmabuf(mr->umem); > > > + > > > + dma_buf_vunmap(umem_dmabuf->dmabuf, mr->dmabuf_map); > > > + kfree(mr->dmabuf_map); > > > +} > > > + > > > +int rxe_mr_dmabuf_init_user(struct rxe_pd *pd, int fd, u64 start, u64 length, > > > + u64 iova, int access, struct rxe_mr *mr) > > > +{ > > > + struct ib_umem_dmabuf *umem_dmabuf; > > > + struct rxe_map_set *set; > > > + int err; > > > + > > > + umem_dmabuf = ib_umem_dmabuf_get(pd->ibpd.device, start, length, fd, > > > + access, NULL); > > > + if (IS_ERR(umem_dmabuf)) { > > > + err = PTR_ERR(umem_dmabuf); > > > + goto err_out; > > > + } > > > + > > > + rxe_mr_init(access, mr); > > > + > > > + err = rxe_mr_alloc(mr, ib_umem_num_pages(&umem_dmabuf->umem), 0); > > > + if (err) { > > > + pr_warn("%s: Unable to allocate memory for map\n", __func__); > > > + goto err_release_umem; > > > + } > > > + > > > + mr->ibmr.pd = &pd->ibpd; > > > + mr->umem = &umem_dmabuf->umem; > > > + mr->access = access; > > > + mr->state = RXE_MR_STATE_VALID; > > > + mr->type = IB_MR_TYPE_USER; > > > + > > > + set = mr->cur_map_set; > > > + set->length = length; > > > + set->iova = iova; > > > + set->va = start; > > > + set->offset = ib_umem_offset(mr->umem); > > > + > > > + err = rxe_map_dmabuf_mr(mr, umem_dmabuf); > > > + if (err) > > > + goto err_free_map_set; > > > + > > > + return 0; > > > + > > > +err_free_map_set: > > > + rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); > > > +err_release_umem: > > > + ib_umem_release(&umem_dmabuf->umem); > > > +err_out: > > > + return err; > > > +} > > > + > > > int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr) > > > { > > > int err; > > > @@ -703,6 +818,9 @@ void rxe_mr_cleanup(struct rxe_pool_entry *arg) > > > { > > > struct rxe_mr *mr = container_of(arg, typeof(*mr), pelem); > > > > > > + if (mr->umem && mr->umem->is_dmabuf) > > > + rxe_unmap_dmabuf_mr(mr); > > > + > > > ib_umem_release(mr->umem); > > > > > > if (mr->cur_map_set) > > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c > > > index 9d0bb9aa7514..6191bb4f434d 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c > > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c > > > @@ -916,6 +916,39 @@ static struct ib_mr *rxe_reg_user_mr(struct ib_pd *ibpd, > > > return ERR_PTR(err); > > > } > > > > > > +static struct ib_mr *rxe_reg_user_mr_dmabuf(struct ib_pd *ibpd, u64 start, > > > + u64 length, u64 iova, int fd, > > > + int access, struct ib_udata *udata) > > > +{ > > > + int err; > > > + struct rxe_dev *rxe = to_rdev(ibpd->device); > > > + struct rxe_pd *pd = to_rpd(ibpd); > > > + struct rxe_mr *mr; > > > + > > > + mr = rxe_alloc(&rxe->mr_pool); > > > + if (!mr) { > > > + err = -ENOMEM; > > > + goto err2; > > > + } > > > + > > > + rxe_add_index(mr); > > > + > > > + rxe_add_ref(pd); > > > + > > > + err = rxe_mr_dmabuf_init_user(pd, fd, start, length, iova, access, mr); > > > + if (err) > > > + goto err3; > > > + > > > + return &mr->ibmr; > > > + > > > +err3: > > > + rxe_drop_ref(pd); > > > + rxe_drop_index(mr); > > > + rxe_drop_ref(mr); > > > +err2: > > > + return ERR_PTR(err); > > > +} > > > + > > > static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type, > > > u32 max_num_sg) > > > { > > > @@ -1081,6 +1114,7 @@ static const struct ib_device_ops rxe_dev_ops = { > > > .query_qp = rxe_query_qp, > > > .query_srq = rxe_query_srq, > > > .reg_user_mr = rxe_reg_user_mr, > > > + .reg_user_mr_dmabuf = rxe_reg_user_mr_dmabuf, > > > .req_notify_cq = rxe_req_notify_cq, > > > .resize_cq = rxe_resize_cq, > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h > > > index c807639435eb..0aa95ab06b6e 100644 > > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.h > > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h > > > @@ -334,6 +334,8 @@ struct rxe_mr { > > > > > > struct rxe_map_set *cur_map_set; > > > struct rxe_map_set *next_map_set; > > > + > > > + struct dma_buf_map *dmabuf_map; > > > }; > > > > > > enum rxe_mw_state { > > > -- > > > 2.17.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 2 months

1
0
0 0

[PATCH] dma-resv: Fix dma_resv_get_fences and dma_resv_copy_fences after conversion

by Tvrtko Ursulin

From: Tvrtko Ursulin <tvrtko.ursulin(a)intel.com> Cache the count of shared fences in the iterator to avoid dereferencing the dma_resv_object outside the RCU protection. Otherwise iterator and its users can observe an incosistent state which makes it impossible to use safely. Such as: <6> [187.517041] [IGT] gem_sync: executing <7> [187.536343] i915 0000:00:02.0: [drm:i915_gem_context_create_ioctl [i915]] HW context 1 created <7> [187.536793] i915 0000:00:02.0: [drm:i915_gem_context_create_ioctl [i915]] HW context 1 created <6> [187.551235] [IGT] gem_sync: starting subtest basic-many-each <1> [188.935462] BUG: kernel NULL pointer dereference, address: 0000000000000010 <1> [188.935485] #PF: supervisor write access in kernel mode <1> [188.935495] #PF: error_code(0x0002) - not-present page <6> [188.935504] PGD 0 P4D 0 <4> [188.935512] Oops: 0002 [#1] PREEMPT SMP NOPTI <4> [188.935521] CPU: 2 PID: 1467 Comm: gem_sync Not tainted 5.15.0-rc4-CI-Patchwork_21264+ #1 <4> [188.935535] Hardware name: /NUC6CAYB, BIOS AYAPLCEL.86A.0049.2018.0508.1356 05/08/2018 <4> [188.935546] RIP: 0010:dma_resv_get_fences+0x116/0x2d0 <4> [188.935560] Code: 10 85 c0 7f c9 be 03 00 00 00 e8 15 8b df ff eb bd e8 8e c6 ff ff eb b6 41 8b 04 24 49 8b 55 00 48 89 e7 8d 48 01 41 89 0c 24 <4c> 89 34 c2 e8 41 f2 ff ff 49 89 c6 48 85 c0 75 8c 48 8b 44 24 10 <4> [188.935583] RSP: 0018:ffffc900011dbcc8 EFLAGS: 00010202 <4> [188.935593] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000000000001 <4> [188.935603] RDX: 0000000000000010 RSI: ffffffff822e343c RDI: ffffc900011dbcc8 <4> [188.935613] RBP: ffffc900011dbd48 R08: ffff88812d255bb8 R09: 00000000fffffffe <4> [188.935623] R10: 0000000000000001 R11: 0000000000000000 R12: ffffc900011dbd44 <4> [188.935633] R13: ffffc900011dbd50 R14: ffff888113d29cc0 R15: 0000000000000000 <4> [188.935643] FS: 00007f68d17e9700(0000) GS:ffff888277900000(0000) knlGS:0000000000000000 <4> [188.935655] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [188.935665] CR2: 0000000000000010 CR3: 000000012d0a4000 CR4: 00000000003506e0 <4> [188.935676] Call Trace: <4> [188.935685] i915_gem_object_wait+0x1ff/0x410 [i915] <4> [188.935988] i915_gem_wait_ioctl+0xf2/0x2a0 [i915] <4> [188.936272] ? i915_gem_object_wait+0x410/0x410 [i915] <4> [188.936533] drm_ioctl_kernel+0xae/0x140 <4> [188.936546] drm_ioctl+0x201/0x3d0 <4> [188.936555] ? i915_gem_object_wait+0x410/0x410 [i915] <4> [188.936820] ? __fget_files+0xc2/0x1c0 <4> [188.936830] ? __fget_files+0xda/0x1c0 <4> [188.936839] __x64_sys_ioctl+0x6d/0xa0 <4> [188.936848] do_syscall_64+0x3a/0xb0 <4> [188.936859] entry_SYSCALL_64_after_hwframe+0x44/0xae If the shared object has changed during the RCU unlocked period callers will correctly handle the restart on the next iteration. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)intel.com> Fixes: 96601e8a4755 ("dma-buf: use new iterator in dma_resv_copy_fences") Fixes: d3c80698c9f5 ("dma-buf: use new iterator in dma_resv_get_fences v3") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4274 Cc: Christian König <christian.koenig(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-resv.c | 18 ++++++++++-------- include/linux/dma-resv.h | 5 ++++- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a480af9581bd..7b6d881c8904 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -333,10 +333,14 @@ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) { cursor->seq = read_seqcount_begin(&cursor->obj->seq); cursor->index = -1; - if (cursor->all_fences) + cursor->shared_count = 0; + if (cursor->all_fences) { cursor->fences = dma_resv_shared_list(cursor->obj); - else + if (cursor->fences) + cursor->shared_count = cursor->fences->shared_count; + } else { cursor->fences = NULL; + } cursor->is_restarted = true; } @@ -363,7 +367,7 @@ static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) continue; } else if (!cursor->fences || - cursor->index >= cursor->fences->shared_count) { + cursor->index >= cursor->shared_count) { cursor->fence = NULL; break; @@ -448,10 +452,8 @@ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) dma_resv_list_free(list); dma_fence_put(excl); - if (cursor.fences) { - unsigned int cnt = cursor.fences->shared_count; - - list = dma_resv_list_alloc(cnt); + if (cursor.shared_count) { + list = dma_resv_list_alloc(cursor.shared_count); if (!list) { dma_resv_iter_end(&cursor); return -ENOMEM; @@ -522,7 +524,7 @@ int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **fence_excl, if (fence_excl) dma_fence_put(*fence_excl); - count = cursor.fences ? cursor.fences->shared_count : 0; + count = cursor.shared_count; count += fence_excl ? 0 : 1; /* Eventually re-allocate the array */ diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 8b6c20636a79..3e1bff147428 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -170,9 +170,12 @@ struct dma_resv_iter { /** @index: index into the shared fences */ unsigned int index; - /** @fences: the shared fences */ + /** @fences: the shared fences; private, *MUST* not dereference */ struct dma_resv_list *fences; + /** @shared_count: number of shared fences */ + unsigned int shared_count; + /** @is_restarted: true if this is the first returned fence */ bool is_restarted; }; -- 2.30.2

4 years, 2 months

3
5
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Christian König

Am 09.10.21 um 07:55 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > If dma-buf don't want userspace users to touch the dmabuf buffer, > it seems we should add this restriction into dma_buf_ops.mmap, > not in this IOCTL:DMA_BUF_SET_NAME. > > With this restriction, we can only know the kernel users of the dmabuf > by attachments. > However, for many userspace users, such as userpsace users of dma_heap, > they also need to mark the usage of dma-buf, and they don't care about > who attached to this dmabuf, and seems it's no meaning to be waiting for > IOCTL:DMA_BUF_SET_NAME rather than mmap. Sounds valid to me, but I have no idea why this restriction was added in the first place. Can you double check the git history and maybe identify when that was added? Mentioning this change in the commit message then might make things a bit easier to understand. Thanks, Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..db2f4efdec32 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > > /** > * dma_buf_set_name - Set a name to a specific dma_buf to track the usage. > - * The name of the dma-buf buffer can only be set when the dma-buf is not > - * attached to any devices. It could theoritically support changing the > - * name of the dma-buf if the same piece of memory is used for multiple > - * purpose between different devices. > + * It could theoretically support changing the name of the dma-buf if the same > + * piece of memory is used for multiple purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > * @buf: [in] A piece of userspace memory that contains the name of > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > if (IS_ERR(name)) > return PTR_ERR(name); > > - dma_resv_lock(dmabuf->resv, NULL); > - if (!list_empty(&dmabuf->attachments)) { > - ret = -EBUSY; > - 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); > return ret; > } >

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Randy Dunlap

On 10/8/21 7:47 PM, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > If dma-buf don't want userspace users to touch the dmabuf buffer, > it seems we should add this restriction into dma_buf_ops.mmap, > not in this IOCTL:DMA_BUF_SET_NAME. > > With this restriction, we can only know the kernel users of the dmabuf > by attachments. > However, for many userspace users, such as userpsace users of dma_heap, > they also need to mark the usage of dma-buf, and they don't care about > who attached to this dmabuf, and seems it's no meaning to waitting for to be waiting for > IOCTL:DMA_BUF_SET_NAME rather than mmap. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..afbd0a226639 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > > /** > * dma_buf_set_name - Set a name to a specific dma_buf to track the usage. > - * The name of the dma-buf buffer can only be set when the dma-buf is not > - * attached to any devices. It could theoritically support changing the > - * name of the dma-buf if the same piece of memory is used for multiple > - * purpose between different devices. > + * It could theoritically support changing the name of the dma-buf if the same theoretically (yes, it was incorrect before this change.) > + * piece of memory is used for multiple purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > * @buf: [in] A piece of userspace memory that contains the name of > @@ -346,19 +344,11 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > if (IS_ERR(name)) > return PTR_ERR(name); > > - dma_resv_lock(dmabuf->resv, NULL); > - if (!list_empty(&dmabuf->attachments)) { > - ret = -EBUSY; > - 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); > return ret; > } > > -- ~Randy

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 08.10.21 um 09:54 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Because dma-buf.name can be freed in func: "dma_buf_set_name", > so, we need to acquire lock first before we read/write dma_buf.name > to prevent Use After Free(UAF) issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Going to push that upstream if nobody else objects. Thanks, Christian. > --- > drivers/dma-buf/dma-buf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..a7f6fd13a635 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1372,6 +1372,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > if (ret) > goto error_unlock; > > + > + spin_lock(&buf_obj->name_lock); > seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > @@ -1379,6 +1381,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > buf_obj->exp_name, > file_inode(buf_obj->file)->i_ino, > buf_obj->name ?: ""); > + spin_unlock(&buf_obj->name_lock); > > robj = buf_obj->resv; > fence = dma_resv_excl_fence(robj);

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 08.10.21 um 08:29 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Because dma-buf.name can be freed in func: "dma_buf_set_name", > so, we need to acquire lock first before we read/write dma_buf.name > to prevent Use After Free(UAF) issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..aebb51b3ff52 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -80,7 +80,9 @@ static void dma_buf_release(struct dentry *dentry) > dma_resv_fini(dmabuf->resv); > > module_put(dmabuf->owner); > + spin_lock(&dmabuf->name_lock); > kfree(dmabuf->name); > + spin_unlock(&dmabuf->name_lock); That here is certainly a NAK. This is the release function if somebody is changing the name on a released DMA-buf we have much bigger problems. > kfree(dmabuf); > } > > @@ -1372,6 +1374,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > if (ret) > goto error_unlock; > > + > + spin_lock(&dmabuf->name_lock); > seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > @@ -1379,6 +1383,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > buf_obj->exp_name, > file_inode(buf_obj->file)->i_ino, > buf_obj->name ?: ""); > + spin_unlock(&dmabuf->name_lock); Yeah, that part looks like a good idea to me as well. Christian. > > robj = buf_obj->resv; > fence = dma_resv_excl_fence(robj);

4 years, 2 months

1
0
0 0

[PATCH 1/2] dma-buf: add dma_resv_for_each_fence v3

by Christian König

A simpler version of the iterator to be used when the dma_resv object is locked. v2: fix index check here as well v3: minor coding improvement, some documentation cleanup Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-resv.c | 51 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 20 +++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a480af9581bd..2f98caa68ae5 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -423,6 +423,57 @@ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) } EXPORT_SYMBOL(dma_resv_iter_next_unlocked); +/** + * dma_resv_iter_first - first fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the first fence in the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor) +{ + struct dma_fence *fence; + + dma_resv_assert_held(cursor->obj); + + cursor->index = 0; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + + fence = dma_resv_excl_fence(cursor->obj); + if (!fence) + fence = dma_resv_iter_next(cursor); + + cursor->is_restarted = true; + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_iter_first); + +/** + * dma_resv_iter_next - next fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the next fences from the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor) +{ + unsigned int idx; + + dma_resv_assert_held(cursor->obj); + + cursor->is_restarted = false; + if (!cursor->fences || cursor->index >= cursor->fences->shared_count) + return NULL; + + idx = cursor->index++; + return rcu_dereference_protected(cursor->fences->shared[idx], + dma_resv_held(cursor->obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_iter_next); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 764138ad8583..491359cea54c 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -179,6 +179,8 @@ struct dma_resv_iter { struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor); /** * dma_resv_iter_begin - initialize a dma_resv_iter object @@ -244,6 +246,24 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) for (fence = dma_resv_iter_first_unlocked(cursor); \ fence; fence = dma_resv_iter_next_unlocked(cursor)) +/** + * dma_resv_for_each_fence - fence iterator + * @cursor: a struct dma_resv_iter pointer + * @obj: a dma_resv object pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object while holding the + * &dma_resv.lock. @all_fences controls if the shared fences are returned as + * well. The cursor initialisation is part of the iterator and the fence stays + * valid as long as the lock is held and so no extra reference to the fence is + * taken. + */ +#define dma_resv_for_each_fence(cursor, obj, all_fences, fence) \ + for (dma_resv_iter_begin(cursor, obj, all_fences), \ + fence = dma_resv_iter_first(cursor); fence; \ + fence = dma_resv_iter_next(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -- 2.25.1

4 years, 2 months

2
3
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi guys, I've fixed up the lockdep splat in the new selftests found by the CI systems and added another path for dma_resv_poll. I know you guys are flooded, but can we get at least the first few patches committed? The patches to change the individual drivers could also be pushed later on I think. Thanks, Christian.

4 years, 2 months

3
47
0 0

Re: [Linaro-mm-sig] [PATCH net-next 2/5] dt-bindings: net: brcm, unimac-mdio: Add asp-v2.0

by Rob Herring

On Fri, 24 Sep 2021 14:44:48 -0700, Justin Chen wrote: > The ASP 2.0 Ethernet controller uses a brcm unimac. > > Signed-off-by: Justin Chen <justinpopo6(a)gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli(a)gmail.com> > --- > Documentation/devicetree/bindings/net/brcm,unimac-mdio.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh(a)kernel.org>

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v6 2/2] habanalabs: add support for dma-buf exporter

by Jason Gunthorpe

On Thu, Sep 30, 2021 at 03:46:35PM +0300, Oded Gabbay wrote: > After reading the kernel iommu code, I think this is not relevant > here, and I'll add a comment appropriately but I'll also write it > here, and please correct me if my understanding is wrong. > > The memory behind this specific dma-buf has *always* resided on the > device itself, i.e. it lives only in the 'device' domain (after all, > it maps a PCI bar address which points to the device memory). > Therefore, it was never in the 'CPU' domain and hence, there is no > need to perform a sync of the memory to the CPU's cache, as it was > never inside that cache to begin with. > > This is not the same case as with regular memory which is dma-mapped > and then copied into the device using a dma engine. In that case, > the memory started in the 'CPU' domain and moved to the 'device' > domain. When it is unmapped it will indeed be recycled to be used > for another purpose and therefore we need to sync the CPU cache. > > Is my understanding correct ? It makes sense to me Jason

4 years, 2 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v6 2/2] habanalabs: add support for dma-buf exporter

by Jason Gunthorpe

On Wed, Sep 29, 2021 at 12:17:35AM +0300, Oded Gabbay wrote: > On Tue, Sep 28, 2021 at 8:36 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Sun, Sep 12, 2021 at 07:53:09PM +0300, Oded Gabbay wrote: > > > From: Tomer Tayar <ttayar(a)habana.ai> > > > > > > Implement the calls to the dma-buf kernel api to create a dma-buf > > > object backed by FD. > > > > > > We block the option to mmap the DMA-BUF object because we don't support > > > DIRECT_IO and implicit P2P. > > > > This statement doesn't make sense, you can mmap your dmabuf if you > > like. All dmabuf mmaps are supposed to set the special bit/etc to > > exclude them from get_user_pages() anyhow - and since this is BAR > > memory not struct page memory this driver would be doing it anyhow. > > > But we block mmap the dmabuf fd from user-space. > If you try to do it, you will get MAP_FAILED. You do, I'm saying the above paragraph explaining *why* that was done is not correct. > > > We check the p2p distance using pci_p2pdma_distance_many() and refusing > > > to map dmabuf in case the distance doesn't allow p2p. > > > > Does this actually allow the p2p transfer for your intended use cases? > > It depends on the system. If we are working bare-metal, then yes, it allows. > If inside a VM, then no. The virtualized root complex is not > white-listed and the kernel can't know the distance. > But I remember you asked me to add this check, in v3 of the review IIRC. > I don't mind removing this check if you don't object. Yes, i tis the right code, I was curious how far along things have gotten > > Don't write to the kernel log from user space triggered actions > at all ? At all. > It's the first time I hear about this limitation... Oh? It is a security issue, we don't want to allow userspace to DOS the kerne logging. > How do you tell the user it has done something wrong ? dev_dbg is the usual way and then users doing debugging can opt in to the logging. > > Why doesn't this return a sg_table * and an ERR_PTR? > Basically I modeled this function after amdgpu_vram_mgr_alloc_sgt() > And in that function they also return int and pass the sg_table as ** > > If it's critical I can change. Please follow the normal kernel style Jason

4 years, 2 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig