Linaro-mm-sig April 2024

linaro-mm-sig@lists.linaro.org

18 participants
41 discussions

[RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()

by Thomas Hellström

Condsider the following call sequence: /* Upper layer */ dma_fence_begin_signalling(); lock(tainted_shared_lock); /* Driver callback */ dma_fence_begin_signalling(); ... The driver might here use a utility that is annotated as intended for the dma-fence signalling critical path. Now if the upper layer isn't correctly annotated yet for whatever reason, resulting in /* Upper layer */ lock(tainted_shared_lock); /* Driver callback */ dma_fence_begin_signalling(); We will receive a false lockdep locking order violation notification from dma_fence_begin_signalling(). However entering a dma-fence signalling critical section itself doesn't block and could not cause a deadlock. So use a successful read_trylock() annotation instead for dma_fence_begin_signalling(). That will make sure that the locking order is correctly registered in the first case, and doesn't register any locking order in the second case. The alternative is of course to make sure that the "Upper layer" is always correctly annotated. But experience shows that's not easily achievable in all cases. Signed-off-by: Thomas Hellström <thomas.hellstrom(a)linux.intel.com> --- drivers/dma-buf/dma-fence.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index f177c56269bb..17f632768ef9 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void) if (in_atomic()) return true; - /* ... and non-recursive readlock */ - lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_); + /* ... and non-recursive successful read_trylock */ + lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_); return false; } @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void) lock_map_acquire(&dma_fence_lockdep_map); lock_map_release(&dma_fence_lockdep_map); if (tmp) - lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_); + lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _THIS_IP_); } #endif -- 2.39.2

3 months

[PATCH v4 0/7] dma-buf: heaps: Add restricted heap

by Yong Wu

The purpose of this patchset is for MediaTek secure video playback, and also to enable other potential uses of this in the future. The 'restricted dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/unmappable by the non-secure (i.e. kernel/userspace) world. That memory will be used by the secure/ trusted world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two MediaTek restricted heaps and they will be used in v4l2[1] and drm[2]. 1) restricted_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) restricted_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos. Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20231206081538.17056-1-yunfei.dong@m… [2] https://lore.kernel.org/all/20231223182932.27683-1-jason-jh.lin@mediatek.co… Change note: v4: 1) Rename the heap name from "secure" to "restricted". suggested from Simon/Pekka. There are still several "secure" string in MTK file since we use ARM platform in which we call this "secure world"/ "secure command". v3: https://lore.kernel.org/linux-mediatek/20231212024607.3681-1-yong.wu@mediat… 1) Separate the secure heap to a common file(secure_heap.c) and mtk special file (secure_heap_mtk.c), and put all the tee related code into our special file. 2) About dt-binding, Add "mediatek," prefix since this is Mediatek TEE firmware definition. 3) Remove the normal CMA heap which is a draft for qcom. Rebase on v6.7-rc1. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediat… 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@m… 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@media… Base on v6.6-rc1. Yong Wu (7): dt-bindings: reserved-memory: Add mediatek,dynamic-restricted-region dma-buf: heaps: Initialize a restricted heap dma-buf: heaps: restricted_heap: Add private heap ops dma-buf: heaps: restricted_heap: Add dma_ops dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init dma-buf: heaps: restricted_heap_mtk: Add TEE memory service call dma_buf: heaps: restricted_heap_mtk: Add a new CMA heap .../mediatek,dynamic-restricted-region.yaml | 43 +++ drivers/dma-buf/heaps/Kconfig | 16 + drivers/dma-buf/heaps/Makefile | 4 +- drivers/dma-buf/heaps/restricted_heap.c | 237 +++++++++++++ drivers/dma-buf/heaps/restricted_heap.h | 43 +++ drivers/dma-buf/heaps/restricted_heap_mtk.c | 322 ++++++++++++++++++ 6 files changed, 664 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml create mode 100644 drivers/dma-buf/heaps/restricted_heap.c create mode 100644 drivers/dma-buf/heaps/restricted_heap.h create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c -- 2.18.0

7 months, 1 week

Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue

by Christian König

Am 18.04.24 um 03:33 schrieb zhiguojiang: > 在 2024/4/15 19:57, Christian König 写道: >> [Some people who received this message don't often get email from >> christian.koenig(a)amd.com. Learn why this is important at >> https://aka.ms/LearnAboutSenderIdentification ] >> >> Am 15.04.24 um 12:35 schrieb zhiguojiang: >>> 在 2024/4/12 14:39, Christian König 写道: >>>> [Some people who received this message don't often get email from >>>> christian.koenig(a)amd.com. Learn why this is important at >>>> https://aka.ms/LearnAboutSenderIdentification ] >>>> >>>> Am 12.04.24 um 08:19 schrieb zhiguojiang: >>>>> [SNIP] >>>>> -> Here task 2220 do epoll again where internally it will get/put >>>>> then >>>>> start to free twice and lead to final crash. >>>>> >>>>> Here is the basic flow: >>>>> >>>>> 1. Thread A install the dma_buf_fd via dma_buf_export, the fd >>>>> refcount >>>>> is 1 >>>>> >>>>> 2. Thread A add the fd to epoll list via epoll_ctl(EPOLL_CTL_ADD) >>>>> >>>>> 3. After use the dma buf, Thread A close the fd, then here fd >>>>> refcount >>>>> is 0, >>>>> and it will run __fput finally to release the file >>>> >>>> Stop, that isn't correct. >>>> >>>> The fs layer which calls dma_buf_poll() should make sure that the file >>>> can't go away until the function returns. >>>> >>>> Then inside dma_buf_poll() we add another reference to the file while >>>> installing the callback: >>>> >>>> /* Paired with fput in dma_buf_poll_cb */ >>>> get_file(dmabuf->file); >>> Hi, >>> >>> The problem may just occurred here. >>> >>> Is it possible file reference count already decreased to 0 and fput >>> already being in progressing just before calling >>> get_file(dmabuf->file) in dma_buf_poll()? >> >> No, exactly that isn't possible. >> >> If a function gets a dma_buf pointer or even more general any reference >> counted pointer which has already decreased to 0 then that is a major >> bug in the caller of that function. >> >> BTW: It's completely illegal to work around such issues by using >> file_count() or RCU functions. So when you suggest stuff like that it >> will immediately face rejection. >> >> Regards, >> Christian. > Hi, > > Thanks for your kind comment. > > 'If a function gets a dma_buf pointer or even more general any reference > > counted pointer which has already decreased to 0 then that is a major > > bug in the caller of that function.' > > According to the issue log, we can see the dma buf file close and > epoll operation running in parallel. > > Apparently at the moment caller calls epoll, although another task > caller already called close on the same fd, but this fd was still in > progress to close, so fd is still valid, thus no EBADF returned to > caller. No, exactly that can't happen either. As far as I can see the EPOLL holds a reference to the files it contains. So it is perfectly valid to add the file descriptor to EPOLL and then close it. In this case the file won't be closed, but be kept alive by it's reference in the EPOLL file descriptor. > > Then the two task contexts operate on same dma buf fd(one is close, > another is epoll) in kernel space. > > Please kindly help to comment whether above scenario is possible. > > If there is some bug in the caller logic, Can u help to point it out? :) Well what could be is that the EPOLL implementation is broken somehow, but I have really doubts on that. As I said before the most likely explanation is that you have a broken device driver which messes up the DMA-buf file reference count somehow. Regards, Christian. > > Regards, > Zhiguo >> >>> >>>> >>>> This reference is only dropped after the callback is completed in >>>> dma_buf_poll_cb(): >>>> >>>> /* Paired with get_file in dma_buf_poll */ >>>> fput(dmabuf->file); >>>> >>>> So your explanation for the issue just seems to be incorrect. >>>> >>>>> >>>>> 4. Here Thread A not do epoll_ctl(EPOLL_CTL_DEL) manunally, so it >>>>> still resides in one epoll_list. >>>>> Although __fput will call eventpoll_release to remove the file from >>>>> binded epoll list, >>>>> but it has small time window where Thread B jumps in. >>>> >>>> Well if that is really the case then that would then be a bug in >>>> epoll_ctl(). >>>> >>>>> >>>>> 5. During the small window, Thread B do the poll action for >>>>> dma_buf_fd, where it will fget/fput for the file, >>>>> this means the fd refcount will be 0 -> 1 -> 0, and it will call >>>>> __fput again. >>>>> This will lead to __fput twice for the same file. >>>>> >>>>> 6. So the potenial fix is use get_file_rcu which to check if file >>>>> refcount already zero which means under free. >>>>> If so, we just return and no need to do the dma_buf_poll. >>>> >>>> Well to say it bluntly as far as I can see this suggestion is >>>> completely >>>> nonsense. >>>> >>>> When the reference to the file goes away while dma_buf_poll() is >>>> executed then that's a massive bug in the caller of that function. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> Here is the race condition: >>>>> >>>>> Thread A Thread B >>>>> dma_buf_export >>>>> fd_refcount is 1 >>>>> epoll_ctl(EPOLL_ADD) >>>>> add dma_buf_fd to epoll list >>>>> close(dma_buf_fd) >>>>> fd_refcount is 0 >>>>> __fput >>>>> dma_buf_poll >>>>> fget >>>>> fput >>>>> fd_refcount is zero again >>>>> >>>>> Thanks >>>>> >>>> >>> >> >

7 months, 2 weeks

Re: [PATCH v4 05/16] SoC: mediatek: mt8365: support audio clock control

by Mark Brown

On Fri, Apr 26, 2024 at 07:22:34PM +0200, Alexandre Mergnat wrote: > Add audio clock wrapper and audio tuner control. Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.

7 months, 3 weeks

Re: [PATCH v4 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 26/04/2024 19:22, Alexandre Mergnat wrote: > regulators: > type: object > $ref: /schemas/regulator/mediatek,mt6357-regulator.yaml > @@ -83,6 +111,12 @@ examples: > interrupt-controller; > #interrupt-cells = <2>; > > + audio-codec { > + mediatek,micbias0-microvolt = <1700000>; > + mediatek,micbias1-microvolt = <1700000>; > + vaud28-supply = <&mt6357_vaud28_reg>; And now you should see how odd it looks. Supplies are part of entire chip, not subblock, even if they supply dedicated domain within that chip. That's why I asked to put it in the parent node. Best regards, Krzysztof

7 months, 3 weeks

Re: [PATCH v4 02/16] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

by Krzysztof Kozlowski

On 26/04/2024 19:22, Alexandre Mergnat wrote: > + link-name: > + description: Indicates dai-link name and PCM stream name > + enum: > + - I2S_IN_BE > + - I2S_OUT_BE > + - PCM1_BE > + - PDM1_BE > + - PDM2_BE > + - PDM3_BE > + - PDM4_BE > + - SPDIF_IN_BE > + - SPDIF_OUT_BE > + - TDM_IN_BE > + - TDM_OUT_BE Feels like BE is redundant. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> Best regards, Krzysztof

7 months, 3 weeks

Re: [PATCH v4 01/16] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

by Krzysztof Kozlowski

On 26/04/2024 19:22, Alexandre Mergnat wrote: > Add MT8365 audio front-end bindings > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> Best regards, Krzysztof

7 months, 3 weeks

Re: [PATCH v3 03/18] ASoC: dt-bindings: mt6357: Add audio codec document

by Krzysztof Kozlowski

On 23/04/2024 19:07, Alexandre Mergnat wrote: > > > On 09/04/2024 17:55, Krzysztof Kozlowski wrote: >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + codec { >>> + mediatek,micbias0-microvolt = <1900000>; >>> + mediatek,micbias1-microvolt = <1700000>; >>> + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; >> Sorry, this does not work. Change voltage to 1111111 and check the results. > > Actually it's worst ! I've removed the required property (vaud28-supply) but the dt check pass. > Same behavior for some other docs like mt6359.yaml Yeah, the schema is not applied. There is nothing selecting it, so this is no-op schema. I don't know what exactly you want to describe, but usually either you miss compatible or this should be just part of parent node. > > The at24.yaml doc works as expected, then I tried compare an find the issue, without success... > > I've replaced "codec" by "audio-codec", according to [1]. > I've tried multiple manner to implement the example code, without success. I'm wondering if what I > try to do is the correct way or parse-able by the dt_check. > > If I drop this file and implement all these new properties into the MFD PMIC documentation directly, > I've the expected dt_check result (function to good or wrong parameters) Yes. Best regards, Krzysztof

8 months

Re: [PATCH] drm/panfrost: Fix dma_resv deadlock at drm object pin time

by Dmitry Osipenko

On 4/21/24 19:39, Adrián Larumbe wrote: > When Panfrost must pin an object that is being prepared a dma-buf > attachment for on behalf of another driver, the core drm gem object pinning > code already takes a lock on the object's dma reservation. > > However, Panfrost GEM object's pinning callback would eventually try taking > the lock on the same dma reservation when delegating pinning of the object > onto the shmem subsystem, which led to a deadlock. > > This can be shown by enabling CONFIG_DEBUG_WW_MUTEX_SLOWPATH, which throws > the following recursive locking situation: > > weston/3440 is trying to acquire lock: > ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_shmem_pin+0x34/0xb8 [drm_shmem_helper] > but task is already holding lock: > ffff000000e235a0 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_pin+0x2c/0x80 [drm] > > Fix it by assuming the object's reservation had already been locked by the > time we reach panfrost_gem_pin. > > Cc: Thomas Zimmermann <tzimmermann(a)suse.de> > Cc: Dmitry Osipenko <dmitry.osipenko(a)collabora.com> > Cc: Boris Brezillon <boris.brezillon(a)collabora.com> > Cc: Steven Price <steven.price(a)arm.com> > Fixes: a78027847226 ("drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}()") > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_gem.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index d47b40b82b0b..6c26652d425d 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -192,7 +192,12 @@ static int panfrost_gem_pin(struct drm_gem_object *obj) > if (bo->is_heap) > return -EINVAL; > > - return drm_gem_shmem_pin(&bo->base); > + /* > + * Pinning can only happen in response to a prime attachment request from > + * another driver, but that's already being handled by drm_gem_pin > + */ > + drm_WARN_ON(obj->dev, obj->import_attach); > + return drm_gem_shmem_pin_locked(&bo->base); > } Will be better to use drm_gem_shmem_object_pin() to avoid such problem in future Please also fix the Lima driver -- Best regards, Dmitry

8 months

Re: [PATCH v3 2/2] misc: sram: Add DMA-BUF Heap exporting of SRAM areas

by Greg Kroah-Hartman

On Tue, Apr 09, 2024 at 02:06:05PM +0200, Pascal FONTAIN wrote: > From: Andrew Davis <afd(a)ti.com> > > This new export type exposes to userspace the SRAM area as a DMA-BUF > Heap, > this allows for allocations of DMA-BUFs that can be consumed by various > DMA-BUF supporting devices. > > Signed-off-by: Andrew Davis <afd(a)ti.com> > Tested-by: Pascal Fontain <pascal.fontain(a)bachmann.info> When sending on a patch from someone else, you too must sign off on it as per our documenation. Please read it and understand what you are agreeing to when you do that for a new version please. > --- > drivers/misc/Kconfig | 7 + > drivers/misc/Makefile | 1 + > drivers/misc/sram-dma-heap.c | 246 +++++++++++++++++++++++++++++++++++ > drivers/misc/sram.c | 6 + > drivers/misc/sram.h | 16 +++ > 5 files changed, 276 insertions(+) > create mode 100644 drivers/misc/sram-dma-heap.c > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 9e4ad4d61f06..e6674e913168 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -448,6 +448,13 @@ config SRAM > config SRAM_EXEC > bool > > +config SRAM_DMA_HEAP > + bool "Export on-chip SRAM pools using DMA-Heaps" > + depends on DMABUF_HEAPS && SRAM > + help > + This driver allows the export of on-chip SRAM marked as both pool > + and exportable to userspace using the DMA-Heaps interface. What will use this in userspace? thanks, greg k-h

8 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig April 2024