Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
3689 discussions

[PATCH v3 0/3] dma-buf: dmabuf->file release-time cleanup

by Matt Evans

Hi all, A fix for DMABUF's file-release path, to set dmabuf->file = NULL when the struct file itself is being freed and avoid a stale pointer lingering. Previously it could be assumed that dmabuf->file was always valid until .release was called, but commit 4ab59c3c638c6 ("dma-buf: Move dma_buf_release() from fops to dentry_ops") separated dentry release (calls .release) from file release (can happen before .release). Setting the pointer to NULL clearly delimits the period in which it points to a valid struct file. Raw dmabuf->file access is bad news unless the file is known to have a reference held (for example, during export). A single-threaded problem scenario is a dentry held open (e.g. using O_PATH) but the file closed; .release is not called, so the exporter was unaware that dmabuf->file became stale. Exporter activities using dmabuf->file then led to a UAF. Or, threads racing concurrent access with a file closing. VFIO uses get_file_active(&dmabuf->file), which is a UAF when the pointer goes stale. Alex Mastro has created a VFIO-centric reproducer [1], which shows up with KASAN. To fix, dmabuf->file is either valid or NULL. get_file_active() is then safe and a recommended way to access dmabuf->file without an existing reference. The only possible outcomes are for it to get a reference to the file, to observe a valid pointer but the file in the DEAD state (being closed, fail), or NULL (fail). Patch [3/3] sets the pointer NULL when the file closes, which get_file_active() handles correctly. vmwgfx was identified as doing a problematic dmabuf->file dereference. Although already a UAF, making dmabuf->file = NULL turns it into an oops. So, patch [1/3] correspondingly converts vmwgfx to use get_file_active() too. i915 GVT unsafely dereferences dmabuf->file for a trivial debug printout after the file reference moves to the dmabuf's fd, so patch [2/3] dereferences in the safe window. Added comments to highlight that dmabuf->file could go away before .release, and to use get_file_active() if a reference isn't otherwise already held. Thanks, Matt [1]: https://github.com/opsound/vfio-dmabuf-lab/blob/main/tests/vfio_dmabuf_opat… Changes: v3: - Better comment for struct dma_buf's file (refer to synchronisation against ->release). - Added trivial i915 patch to remove unsafe debug dmabuf->file accesss. v2: https://lore.kernel.org/all/20261009123601.94079-1-matt@ozlabs.org/ "v1" was the single "dma-buf: Annul dmabuf->file on file release" patch: https://lore.kernel.org/all/f8efaabd-c06e-4f04-8cfa-489c148e37ba@ozlabs.org/ Matt Evans (3): drm/vmwgfx: Use get_file_active() for dmabuf->file drm/i915/gvt: Read dmabuf->file only while valid dma-buf: Annul dmabuf->file on file release drivers/dma-buf/dma-buf.c | 21 ++++++++++++++++++--- drivers/gpu/drm/i915/gvt/dmabuf.c | 5 +++-- drivers/gpu/drm/vmwgfx/ttm_object.c | 2 +- include/linux/dma-buf.h | 5 ++++- 4 files changed, 26 insertions(+), 7 deletions(-) -- 2.47.3

2 hours, 5 minutes

[PATCH v3 0/8] memcg: dma-buf cgroup accounting and charge transfer

by Albert Esteve

Shared-memory allocator services (e.g., Android's gralloc) allocate dma-bufs on behalf of client processes in different cgroups. Today those allocations are charged to the allocator's own cgroup, leaving client cgroups with no visibility into their actual memory consumption and defeating resource limits. This series adds memory cgroup accounting for dma-buf allocations, building on T.J. Mercier's earlier series [1] and incorporating feedback from previous versions and LPC discussions [2]. Patch 1 introduces the MEMCG_DMABUF stat and reserve/commit/cancel and uncharge helpers. Rather than charging at the page level via __GFP_ACCOUNT (which would require every dma-buf exporter to implement charging, and makes cross-cgroup transfer impractical without access to backing pages), we track charges externally on struct dma_buf via a MEMCG_DMABUF stat counter and route all accounting through a single path, removing the __GFP_ACCOUNT flag from system-heap allocations to avoid double counting. Patch 2 adds a charge_fd field to DMA_HEAP_IOCTL_ALLOC, accepting either a pidfd or a cgroupfd to identify the target cgroup at allocation time. If the target cgroup cannot accommodate the charge, the allocation returns -ENOMEM. Patches 3-6 add access control for cross-cgroup charging. Letting userspace decide who to charge needs to be a privileged operation because it has material impact on OOM decission and resource limits. We gate it via LSM hooks for the two charge_fd flavours: security_dma_buf_charge_task() gates the pidfd path (target identified by task credentials) and security_dma_buf_charge_cgroup() gates the cgroupfd path (target identified by cgroup directory label), with SELinux implementations under a dma_buf object class. Patch 7 adds DMA_BUF_IOCTL_XFER_CHARGE, a general ioctl on any dma-buf fd to transfer charges post-allocation. This covers the double-hop allocator case (SurfaceFlinger -> gralloc -> kernel) and works for buffers from any exporter (DRM, V4L2, etc.). A new memcg_account flag is added for dma_buf_export() to ensure that transfer/charge is only applied for buffers backed by reclaimable system memory. If the receiving cgroup lacks capacity for the charge, the ioctl returns -ENOMEM and the original charge is preserved. Patch 8 adds selftests. Known limitations: - Single-owner model: a dma-buf carries one memcg charge regardless of how many processes share it. - Only memcg accounting. CMA heaps would additionally need dmem controller integration. - If a transfer of an uncharged buffer fails, it remains uncharged. The allocator (userspace) is expected to handle this (retry, free, ...). - The LSM hooks are intentionally narrow. Broader cgroup access controls (e.g. for the memfd/kexec handover case [2]) could be a separate effort reusing the same pattern. - Only system heap sets memcg_account today. Other exporters (DRM, V4L2, etc.) can opt in by setting the flag, but are not covered by this series. [1] https://lore.kernel.org/cgroups/20230109213809.418135-1-tjmercier@google.co… [2] https://lpc.events/event/20/contributions/2416/ Signed-off-by: Albert Esteve <aesteve(a)redhat.com> --- Changes in v3: - Split mem_cgroup_charge_dmabuf() into reserve/commit/cancel three-phase API - Extract mem_cgroup_get_from_fd() helper shared by allocate and xfer paths - Rename __padding to flags in struct dma_buf_transfer_charge - Fix UAF race: move mod_memcg_state() before cmpxchg in transfer - Add memcg_account opt-in flag; xfer returns -EOPNOTSUPP if unset - Wrap dma_buf_charge_cgroup hook in #ifdef CONFIG_CGROUPS - Fix TOCTOU in SELinux charge_cgroup xattr length handling - Link to v2: https://lore.kernel.org/r/20261021-v2_20230123_tjmercier_google_com-v2-0-ef… Changes in v2: - Move charge_pid_fd to charge_fd, accepting cgroupfd in addition to pidfd per Christian König. - New DMA_BUF_IOCTL_XFER_CHARGE ioctl for post-allocation charge transfer on any dma-buf, not just dma-heap buffers per Christian König and T.J. Mercier. - Moved __GFP_ACCOUNT removal to patch 1 per Barry Song. - dma-buf charge/uncharge helpers take struct dma_buf * directly to avoid the need for caller to manage size calculations. - Added test_xfer_charge_cgroupfd_self to dmabuf-heap selftests. - Added LSM and SELinux hooks for cgroupfd-based charging. - Rebased to v7.3-rc7. - Link to v1: https://lore.kernel.org/r/20260512-v2_20230123_tjmercier_google_com-v1-0-63… --- Albert Esteve (7): dma-heap: charge dma-buf memory via explicit memcg security: dma-buf: Add dma_buf_charge_task LSM hook security: dma-buf: Add dma_buf_charge_cgroup LSM hook selinux: Restrict cross-cgroup dma-buf charging selinux: Restrict dma-buf direct cgroup charging dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE selftests/dmabuf-heaps: Add dma-buf memcg accounting tests T.J. Mercier (1): memcg: Add MEMCG_DMABUF accounting infrastructure Documentation/admin-guide/cgroup-v2.rst | 5 + drivers/dma-buf/dma-buf.c | 46 ++++ drivers/dma-buf/dma-heap.c | 28 +- drivers/dma-buf/heaps/cma_heap.c | 3 +- drivers/dma-buf/heaps/system_heap.c | 22 +- drivers/tee/tee_heap.c | 3 +- include/linux/dma-buf.h | 9 + include/linux/dma-heap.h | 3 +- include/linux/lsm_hook_defs.h | 4 + include/linux/memcontrol.h | 52 ++++ include/linux/security.h | 18 ++ include/uapi/linux/dma-buf.h | 26 ++ include/uapi/linux/dma-heap.h | 7 + mm/memcontrol.c | 192 ++++++++++++++ security/security.c | 34 +++ security/selinux/hooks.c | 44 ++++ security/selinux/include/classmap.h | 1 + tools/testing/selftests/cgroup/Makefile | 2 +- tools/testing/selftests/cgroup/test_memcontrol.c | 290 ++++++++++++++++++++- tools/testing/selftests/dmabuf-heaps/config | 1 + tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 227 +++++++++++++++- tools/testing/selftests/dmabuf-heaps/vmtest.sh | 226 ++++++++++++++++ 22 files changed, 1228 insertions(+), 15 deletions(-) --- base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907 change-id: 20260508-v2_20230123_tjmercier_google_com-f44fcfb16530 Best regards, -- Albert Esteve <aesteve(a)redhat.com>

3 hours, 16 minutes

Re: [PATCH] dma-buf: system_heap: Reject private mappings in system_heap_mmap()

by T.J. Mercier

On Fri, Oct 30, 2026 at 1:44 PM Chris Wiseman <chriswiseman(a)google.com> wrote: > > When commit 381295758792 ("dma-buf: system_heap: Rework system heap to > use sgtables instead of pagelists") stopped using heap_helpers.c and > added a custom system_heap_mmap(), the check rejecting private mappings > was dropped. > > Other exporters and buffer allocators reject private or copy-on-write > mappings in their mmap handlers: cma_heap_mmap() and mmap_udmabuf() > check that VM_SHARED or VM_MAYSHARE is set, and drm_gem_shmem_mmap() > and ttm_bo_mmap_obj() reject vma_is_cow_mapping(vma). > > However, system_heap_mmap() omits this check and calls remap_pfn_range() > even when userspace requests MAP_PRIVATE with PROT_WRITE. As a result, > subsequent CPU writes to a MAP_PRIVATE mapping of a system heap buffer > fault into do_wp_page() -> wp_page_copy(), silently allocating private > anonymous copy-on-write pages whose writes never reach the underlying > system heap buffer pages. > > Reject non-shared mappings in system_heap_mmap() with -EINVAL to match > cma_heap_mmap() and mmap_udmabuf(). > > Fixes: 381295758792 ("dma-buf: system_heap: Rework system heap to use sgtables instead of pagelists") > Cc: stable(a)vger.kernel.org > Signed-off-by: Chris Wiseman <chriswiseman(a)google.com> Reviewed-by: T.J. Mercier <tjmercier(a)google.com> I wonder if we should have this in dma_buf_mmap_internal() though. Any thoughts Sumit or Christian?

6 hours, 9 minutes

Re: [PATCH v4 00/25] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 02/11/2026 11:08, Ekansh Gupta wrote: > On 02-11-2026 14:23, Krzysztof Kozlowski wrote: >> On 01/11/2026 23:21, Ekansh Gupta wrote: >>> drivers/accel/qda/qda_soc.c | 66 ++ >>> drivers/accel/qda/qda_soc.h | 61 ++ >>> drivers/misc/Kconfig | 1 + >>> include/uapi/drm/qda_accel.h | 282 +++++++ >>> 33 files changed, 5877 insertions(+), 1 deletion(-) >>> --- >>> base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c >>> change-id: 20261102-v4-e8feb321263f >> >> What is happening here? How are we supposed to work with this? >> >> v1: 20260223-qda-firstpost-4ab05249e2cc >> v2: 20260519-qda-series-78c2bf0ed78b >> v2 again: 20260817-qda-v2-78e2d1f10529 >> >> And now completely different one? >> >> This makes the change id completely unusable. b4 diff obviously does not >> work. Any tracking of this does not work. >> >> NAK, really, I already complained internally months ago about such >> practice, so apparently not enough. >> >> Best regards, >> Krzysztof > > Sorry about this. I lost the original b4 branch and re-enrolled the You lost original branch four times. It's not a coincidence, it's not random data loss because then after first loss you would solve the problem. It's a broken workflow. Best regards, Krzysztof

7 hours, 7 minutes

Re: [PATCH v4 00/25] accel/qda: Qualcomm DSP Accelerator driver

by Krzysztof Kozlowski

On 01/11/2026 23:21, Ekansh Gupta wrote: > drivers/accel/qda/qda_soc.c | 66 ++ > drivers/accel/qda/qda_soc.h | 61 ++ > drivers/misc/Kconfig | 1 + > include/uapi/drm/qda_accel.h | 282 +++++++ > 33 files changed, 5877 insertions(+), 1 deletion(-) > --- > base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c > change-id: 20261102-v4-e8feb321263f What is happening here? How are we supposed to work with this? v1: 20260223-qda-firstpost-4ab05249e2cc v2: 20260519-qda-series-78c2bf0ed78b v2 again: 20260817-qda-v2-78e2d1f10529 And now completely different one? This makes the change id completely unusable. b4 diff obviously does not work. Any tracking of this does not work. NAK, really, I already complained internally months ago about such practice, so apparently not enough. Best regards, Krzysztof

8 hours, 26 minutes

Re: [PATCH v4 21/25] accel/qda: Add Audio protection domain attach and its remote heap

by Krzysztof Kozlowski

On 01/11/2026 23:21, Ekansh Gupta wrote: > > +/* > + * The Audio PD keeps the remote heap across daemon restarts, so only the first > + * attach on a device hands it to the DSP. The lock is held across the attach > + * so that a concurrent one cannot skip the heap while the first is still in > + * flight, and the flag is set only once the DSP has accepted it. > + */ > +static int qda_attach_audio_pd(struct qda_session *session, struct drm_qda_remote_attach *args) > +{ > + struct qda_dev *qdev = session->qda_dev; > + struct qda_static_pd_args sargs = {}; > + char *name; > + int err; > + > + if (strcmp(qdev->dsp_name, "adsp") || !qdev->remote_heap_size) That's value from DTB, so where did you document the ABI? I see such undocumented ABI in multiple places. Usually requirement would be to document it, but not label. Why label 'ADSP' is not correct? Why "Audio DSP" would not be correct? Drop all this label dependency, completely (and do not introduce other names, like node name). Best regards, Krzysztof

8 hours, 28 minutes

Re: [PATCH v4 16/25] accel/qda: Add DSP process creation and release to the legacy shim

by Krzysztof Kozlowski

On 01/11/2026 23:21, Ekansh Gupta wrote: > snode->sdev = sdev; > + snode->secure = secure; > snode->miscdev.minor = MISC_DYNAMIC_MINOR; > snode->miscdev.name = snode->name; > snode->miscdev.fops = &qda_shim_fops; > @@ -293,8 +346,6 @@ static int qda_shim_probe(struct auxiliary_device *auxdev, > const struct auxiliary_device_id *id) > { > struct qda_dev *qdev = dev_get_platdata(&auxdev->dev); > - bool secure_channel = !of_property_read_bool(qdev->dev->of_node, > - "qcom,non-secure-domain"); You just added these lines two patches earlier. You are adding COMPLETELY new driver, so adding completely new code and then removing it is confusing. It means you added something which was not needed. Best regards, Krzysztof

8 hours, 31 minutes

Re: [PATCH v4 23/25] accel/qda: Add DSP capability query

by Arnd Bergmann

On Sun, Nov 1, 2026, at 23:21, Ekansh Gupta wrote: > * > * This structure is used with DRM_IOCTL_QDA_QUERY to query device attributes > - * based on @query_type. > + * based on @query_type. @dsp_name and @cap share storage, as only one query > + * is made per call. > */ > struct drm_qda_query { > __u32 query_type; > - __u8 dsp_name[16]; > + union { > + __u8 dsp_name[16]; > + struct { > + __u32 attribute_id; > + __u32 capability; > + } cap; > + }; > }; I think it would be better to make this separate ioctl commands instead of a multiplexer for each thing you query. ioctl numbers are cheap, but complexity is not. Arnd

8 hours, 32 minutes

Re: [PATCH v4 04/25] accel/qda: Assign a context bank to each session

by Krzysztof Kozlowski

On 01/11/2026 23:21, Ekansh Gupta wrote: > + > +static int qda_count_cb_nodes(struct device_node *parent_node) > +{ > + int count = 0; > + > + for_each_available_child_of_node_scoped(parent_node, child) I don't see a need for scoped loop. > + if (of_device_is_compatible(child, "qcom,fastrpc-compute-cb")) > + count++; > + > + return count; > } Best regards, Krzysztof

8 hours, 33 minutes

Re: [PATCH v4 03/25] accel/qda: Add compute context bank devices

by Krzysztof Kozlowski

On 01/11/2026 23:21, Ekansh Gupta wrote: > diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c > index 6bd977e8d68a..78b5f31974c1 100644 > --- a/drivers/accel/qda/qda_rpmsg.c > +++ b/drivers/accel/qda/qda_rpmsg.c > @@ -5,18 +5,18 @@ > #include <linux/rpmsg.h> > #include <drm/drm_print.h> > > +#include "qda_cb.h" > #include "qda_drv.h" > +#include "qda_soc.h" > > /* > * Both compatibles the binding defines are matched here. The glymur, hawi, > * maili and nord nodes name "qcom,kaanapali-fastrpc" as their fallback > - * compatible and so are matched by that entry. Per-SoC DSP parameters are > - * attached as match data by a later patch, once there is something that > - * consumes them. You just added this in previous commit. Why adding invalid documentation, which is immediately removed? > + * compatible and so are matched by that entry. Best regards, Krzysztof

8 hours, 35 minutes

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig