Linaro-mm-sig April 2024

linaro-mm-sig@lists.linaro.org

18 participants
41 discussions

Re: [PATCH v2] doc: dma-buf: fix grammar typo

by Christian König

Well checkpatch.pl still complained: ERROR: trailing whitespace #157: FILE: Documentation/driver-api/dma-buf.rst:80: + If llseek on dma-buf FDs isn't supported the kernel will report -ESPIPE for all^M$ That actually looks like you used a Windows line ending. I fixed it up and pushed the patch, but please take a bit more care next time. Thanks, Christian. Am 17.04.24 um 06:59 schrieb Baruch Siach: > Use 'supported' instead of 'support'. 'support' makes no sense in this > context. > > Signed-off-by: Baruch Siach <baruch(a)tkos.co.il> > --- > v2: Add commit log message (Christian König) > --- > Documentation/driver-api/dma-buf.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst > index 0c153d79ccc4..29abf1eebf9f 100644 > --- a/Documentation/driver-api/dma-buf.rst > +++ b/Documentation/driver-api/dma-buf.rst > @@ -77,7 +77,7 @@ consider though: > the usual size discover pattern size = SEEK_END(0); SEEK_SET(0). Every other > llseek operation will report -EINVAL. > > - If llseek on dma-buf FDs isn't support the kernel will report -ESPIPE for all > + If llseek on dma-buf FDs isn't supported the kernel will report -ESPIPE for all > cases. Userspace can use this to detect support for discovering the dma-buf > size using llseek. >

8 months, 1 week

Re: [PATCH v5 1/9] drm/mediatek/uapi: Add DRM_MTK_GEM_CREATE_ENCRYPTED flag

by Nicolas Dufresne

Hi, Le mercredi 03 avril 2024 à 18:26 +0800, Shawn Sung a écrit : > From: "Jason-JH.Lin" <jason-jh.lin(a)mediatek.com> > > Add DRM_MTK_GEM_CREATE_ENCRYPTED flag to allow user to allocate Is "ENCRYPTED" a proper naming ? My expectation is that this would hold data in a PROTECTED memory region but that no cryptographic algorithm will be involved. Nicolas > a secure buffer to support secure video path feature. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin(a)mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.com> > --- > include/uapi/drm/mediatek_drm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/drm/mediatek_drm.h b/include/uapi/drm/mediatek_drm.h > index b0dea00bacbc4..e9125de3a24ad 100644 > --- a/include/uapi/drm/mediatek_drm.h > +++ b/include/uapi/drm/mediatek_drm.h > @@ -54,6 +54,7 @@ struct drm_mtk_gem_map_off { > > #define DRM_MTK_GEM_CREATE 0x00 > #define DRM_MTK_GEM_MAP_OFFSET 0x01 > +#define DRM_MTK_GEM_CREATE_ENCRYPTED 0x02 > > #define DRM_IOCTL_MTK_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + \ > DRM_MTK_GEM_CREATE, struct drm_mtk_gem_create)

8 months, 1 week

Re: [PATCH] dma-buf: Do not build debugfs related code when !CONFIG_DEBUG_FS

by Maíra Canal

Hi Tvrtko, On 4/1/24 10:21, Tvrtko Ursulin wrote: > > On 01/04/2024 13:45, Christian König wrote: >> Am 01.04.24 um 14:39 schrieb Tvrtko Ursulin: >>> >>> On 29/03/2024 00:00, T.J. Mercier wrote: >>>> On Thu, Mar 28, 2024 at 7:53 AM Tvrtko Ursulin <tursulin(a)igalia.com> >>>> wrote: >>>>> >>>>> From: Tvrtko Ursulin <tursulin(a)ursulin.net> >>>>> >>>>> There is no point in compiling in the list and mutex operations >>>>> which are >>>>> only used from the dma-buf debugfs code, if debugfs is not compiled >>>>> in. >>>>> >>>>> Put the code in questions behind some kconfig guards and so save >>>>> some text >>>>> and maybe even a pointer per object at runtime when not enabled. >>>>> >>>>> Signed-off-by: Tvrtko Ursulin <tursulin(a)ursulin.net> >>>> >>>> Reviewed-by: T.J. Mercier <tjmercier(a)google.com> >>> >>> Thanks! >>> >>> How would patches to dma-buf be typically landed? Via what tree I >>> mean? drm-misc-next? >> >> That should go through drm-misc-next. >> >> And feel free to add Reviewed-by: Christian König >> <christian.koenig(a)amd.com> as well. > > Thanks! > > Maarten if I got it right you are handling the next drm-misc-next pull - > could you merge this one please? Applied to drm-misc/drm-misc-next! Best Regards, - Maíra > > Regards, > > Tvrtko

8 months, 1 week

Re: [PATCH] doc: dma-buf: fix grammar typo

by Christian König

Am 14.04.24 um 15:07 schrieb Baruch Siach: > Signed-off-by: Baruch Siach <baruch(a)tkos.co.il> > --- > Documentation/driver-api/dma-buf.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst > index 0c153d79ccc4..29abf1eebf9f 100644 > --- a/Documentation/driver-api/dma-buf.rst > +++ b/Documentation/driver-api/dma-buf.rst > @@ -77,7 +77,7 @@ consider though: > the usual size discover pattern size = SEEK_END(0); SEEK_SET(0). Every other > llseek operation will report -EINVAL. > > - If llseek on dma-buf FDs isn't support the kernel will report -ESPIPE for all > + If llseek on dma-buf FDs isn't supported the kernel will report -ESPIPE for all Looks valid of hand, but checkpatch.pl complains about 2 errors (missing commit message for example) and a warning. Please fix and resend. Thanks, Christian. > cases. Userspace can use this to detect support for discovering the dma-buf > size using llseek. >

8 months, 1 week

Re: [PATCH net-next v9 0/3] Add minimal XDP support to TI AM65 CPSW Ethernet driver

by patchwork-bot+netdevbpf＠kernel.org

Hello: This series was applied to netdev/net-next.git (main) by David S. Miller <davem(a)davemloft.net>: On Fri, 12 Apr 2024 17:38:31 +0200 you wrote: > This patch adds XDP support to TI AM65 CPSW Ethernet driver. > > The following features are implemented: NETDEV_XDP_ACT_BASIC, > NETDEV_XDP_ACT_REDIRECT, and NETDEV_XDP_ACT_NDO_XMIT. > > Zero-copy and non-linear XDP buffer supports are NOT implemented. > > [...] Here is the summary with links: - [net-next,v9,1/3] net: ethernet: ti: Add accessors for struct k3_cppi_desc_pool members https://git.kernel.org/netdev/net-next/c/cd8ff81f747f - [net-next,v9,2/3] net: ethernet: ti: Add desc_infos member to struct k3_cppi_desc_pool https://git.kernel.org/netdev/net-next/c/84d767a3c0b5 - [net-next,v9,3/3] net: ethernet: ti: am65-cpsw: Add minimal XDP support https://git.kernel.org/netdev/net-next/c/8acacc40f733 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html

8 months, 1 week

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

by Christian König

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. > >> >> 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 >>> >> >

8 months, 1 week

Re: [PATCH v5 2/9] drm/mediatek: Add secure buffer control flow to mtk_drm_gem

by Maxime Ripard

On Wed, Apr 03, 2024 at 06:26:54PM +0800, Shawn Sung wrote: > From: "Jason-JH.Lin" <jason-jh.lin(a)mediatek.com> > > Add secure buffer control flow to mtk_drm_gem. > > When user space takes DRM_MTK_GEM_CREATE_ENCRYPTED flag and size > to create a mtk_drm_gem object, mtk_drm_gem will find a matched size > dma buffer from secure dma-heap and bind it to mtk_drm_gem object. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin(a)mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_gem.c | 85 +++++++++++++++++++++++++++++- > drivers/gpu/drm/mediatek/mtk_gem.h | 4 ++ > 2 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_gem.c b/drivers/gpu/drm/mediatek/mtk_gem.c > index e59e0727717b7..ec34d02c14377 100644 > --- a/drivers/gpu/drm/mediatek/mtk_gem.c > +++ b/drivers/gpu/drm/mediatek/mtk_gem.c > @@ -4,6 +4,8 @@ > */ > > #include <linux/dma-buf.h> > +#include <linux/dma-heap.h> > +#include <uapi/linux/dma-heap.h> > #include <drm/mediatek_drm.h> > > #include <drm/drm.h> > @@ -102,6 +104,81 @@ struct mtk_gem_obj *mtk_gem_create(struct drm_device *dev, > return ERR_PTR(ret); > } > > +struct mtk_gem_obj *mtk_gem_create_from_heap(struct drm_device *dev, > + const char *heap, size_t size) > +{ > + struct mtk_drm_private *priv = dev->dev_private; > + struct mtk_gem_obj *mtk_gem; > + struct drm_gem_object *obj; > + struct dma_heap *dma_heap; > + struct dma_buf *dma_buf; > + struct dma_buf_attachment *attach; > + struct sg_table *sgt; > + struct iosys_map map = {}; > + int ret; > + > + mtk_gem = mtk_gem_init(dev, size); > + if (IS_ERR(mtk_gem)) > + return ERR_CAST(mtk_gem); > + > + obj = &mtk_gem->base; > + > + dma_heap = dma_heap_find(heap); > + if (!dma_heap) { > + DRM_ERROR("heap find fail\n"); > + goto err_gem_free; > + } > + dma_buf = dma_heap_buffer_alloc(dma_heap, size, > + O_RDWR | O_CLOEXEC, DMA_HEAP_VALID_HEAP_FLAGS); > + if (IS_ERR(dma_buf)) { > + DRM_ERROR("buffer alloc fail\n"); > + dma_heap_put(dma_heap); > + goto err_gem_free; > + } > + dma_heap_put(dma_heap); > + > + attach = dma_buf_attach(dma_buf, priv->dma_dev); > + if (IS_ERR(attach)) { > + DRM_ERROR("attach fail, return\n"); > + dma_buf_put(dma_buf); > + goto err_gem_free; > + } > + > + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); > + if (IS_ERR(sgt)) { > + DRM_ERROR("map failed, detach and return\n"); > + dma_buf_detach(dma_buf, attach); > + dma_buf_put(dma_buf); > + goto err_gem_free; > + } > + obj->import_attach = attach; > + mtk_gem->dma_addr = sg_dma_address(sgt->sgl); > + mtk_gem->sg = sgt; > + mtk_gem->size = dma_buf->size; > + > + if (!strcmp(heap, "mtk_svp") || !strcmp(heap, "mtk_svp_cma")) { > + /* secure buffer can not be mapped */ > + mtk_gem->secure = true; > + } else { > + ret = dma_buf_vmap(dma_buf, &map); > + mtk_gem->kvaddr = map.vaddr; > + if (ret) { > + DRM_ERROR("map failed, ret=%d\n", ret); > + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); > + dma_buf_detach(dma_buf, attach); > + dma_buf_put(dma_buf); > + mtk_gem->kvaddr = NULL; > + } > + } > + > + return mtk_gem; > + > +err_gem_free: > + drm_gem_object_release(obj); > + kfree(mtk_gem); > + return ERR_PTR(-ENOMEM); > +} > + > void mtk_gem_free_object(struct drm_gem_object *obj) > { > struct mtk_gem_obj *mtk_gem = to_mtk_gem_obj(obj); > @@ -229,7 +306,9 @@ struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev, > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = !sg_page(sg->sgl); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > + mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > @@ -304,7 +383,11 @@ int mtk_gem_create_ioctl(struct drm_device *dev, void *data, > struct drm_mtk_gem_create *args = data; > int ret; > > - mtk_gem = mtk_gem_create(dev, args->size, false); > + if (args->flags & DRM_MTK_GEM_CREATE_ENCRYPTED) > + mtk_gem = mtk_gem_create_from_heap(dev, "mtk_svp_cma", args->size); That heap doesn't exist upstream either. Also, I'm wondering if it's the right solution there. From what I can tell, you want to allow to create encrypted buffers from the TEE. Why do we need this as a DRM ioctl at all? A heap seems like the perfect solution to do so, and then you just have to import it into DRM. I'm also not entirely sure that not having a SG list is enough to consider the buffer secure. Wouldn't a buffer allocated without a kernel mapping also be in that situation? Maxime

8 months, 1 week

Re: [PATCH v5 1/9] drm/mediatek/uapi: Add DRM_MTK_GEM_CREATE_ENCRYPTED flag

by Maxime Ripard

Hi, On Wed, Apr 03, 2024 at 06:26:53PM +0800, Shawn Sung wrote: > From: "Jason-JH.Lin" <jason-jh.lin(a)mediatek.com> > > Add DRM_MTK_GEM_CREATE_ENCRYPTED flag to allow user to allocate > a secure buffer to support secure video path feature. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin(a)mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.com> > --- > include/uapi/drm/mediatek_drm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/drm/mediatek_drm.h b/include/uapi/drm/mediatek_drm.h > index b0dea00bacbc4..e9125de3a24ad 100644 > --- a/include/uapi/drm/mediatek_drm.h > +++ b/include/uapi/drm/mediatek_drm.h > @@ -54,6 +54,7 @@ struct drm_mtk_gem_map_off { > > #define DRM_MTK_GEM_CREATE 0x00 > #define DRM_MTK_GEM_MAP_OFFSET 0x01 > +#define DRM_MTK_GEM_CREATE_ENCRYPTED 0x02 > > #define DRM_IOCTL_MTK_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + \ > DRM_MTK_GEM_CREATE, struct drm_mtk_gem_create) That flag doesn't exist in drm-misc-next, which tree is this based on? Maxime

8 months, 1 week

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

by Krzysztof Kozlowski

On 10/04/2024 11:29, Alexandre Mergnat wrote: > > > On 09/04/2024 17:46, Krzysztof Kozlowski wrote: >>> + soc { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + afe@11220000 { >> Did you implement the comment or decided to keep afe? >> > > Though it was clear according to [1]: > " > Audio Front End, this is the same name used for other MTK SoC, to be > consistent. > > Cook a new patch serie to change "afe" by "audio-controller" for all MTK > SoC would be great. > " > > I want to keep it and fix it later with ALL other MTK SoC. > You didn't answer after that, I though it was ok for you... Then no, I don't agree. If you add code, which you already plan to fix, it means the code is not correct somehow. Then just add correct code in the beginning. Best regards, Krzysztof

8 months, 2 weeks

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

by Christian König

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); 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 >

8 months, 2 weeks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig April 2024