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

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

[Albert Esteve]

This series adds memory cgroup accounting for dma-buf allocations,
building on T.J. Mercier's earlier series [1] and incorporating
feedback from the v1 and LPC discussions.

Patch 1 introduces the MEMCG_DMABUF stat and charge/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-4 add 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).

Patches 5-6 implement the SELinux side under a dma_buf object class
with charge_task and charge_cgroup permissions. For charge_cgroup,
the target SID is derived from the cgroup directory's SELinux label.

Patch 7 adds DMA_BUF_IOCTL_XFER_CHARGE, a general ioctl on any
dma-buf fd to transfer charges (or charge) post-allocation. This
covers the double-hop allocator case (SurfaceFlinger -> gralloc) and
works for buffers from any exporter (DRM, V4L2, etc.). 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.
- Charging CMA-backed heaps to memcg charges unreclaimable memory:
  when a cgroup hits its limit, try_charge_memcg() may trigger a
  reclaim pass that cannot free CMA pages. A future version could
  move charging into per-heap callbacks.
- If a transfer of an uncharged buffer fails, the buffer remains
  uncharged. The allocator (userspace) is expected to handle this
  (e.g., retry, free, ...).
- The LSM hook is intentionally narrow. Broader cgroup access controls
  could be a separate effort.

[1] https://lore.kernel.org/cgroups/20230109213809.418135-1-tjmercier@google.com/

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
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-6326701c3691@redhat.com

---
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                          |  80 ++++++++
 drivers/dma-buf/dma-heap.c                         |  72 ++++++-
 drivers/dma-buf/heaps/system_heap.c                |   2 -
 include/linux/dma-buf.h                            |   4 +
 include/linux/lsm_hook_defs.h                      |   2 +
 include/linux/memcontrol.h                         |  29 +++
 include/linux/security.h                           |  14 ++
 include/uapi/linux/dma-buf.h                       |  26 +++
 include/uapi/linux/dma-heap.h                      |   7 +
 mm/memcontrol.c                                    |  94 +++++++++
 security/security.c                                |  32 +++
 security/selinux/hooks.c                           |  40 ++++
 security/selinux/include/classmap.h                |   1 +
 tools/testing/selftests/cgroup/Makefile            |   2 +-
 tools/testing/selftests/cgroup/test_memcontrol.c   | 145 +++++++++++++-
 tools/testing/selftests/dmabuf-heaps/config        |   1 +
 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 222 ++++++++++++++++++++-
 tools/testing/selftests/dmabuf-heaps/vmtest.sh     | 205 +++++++++++++++++++
 19 files changed, 974 insertions(+), 9 deletions(-)
---
base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
change-id: 20260508-v2_20230123_tjmercier_google_com-f44fcfb16530

Best regards,
-- 
Albert Esteve <aesteve@redhat.com>

[PATCH RFC v2 1/8] memcg: Add MEMCG_DMABUF accounting infrastructure

[Albert Esteve]

From: "T.J. Mercier" <tjmercier@google.com>

Introduce a MEMCG_DMABUF stat counter and helpers for charging,
and uncharging dma-buf memory against a memory cgroup:

The charge is tracked on struct dma_buf via a new memcg pointer. The
page count is derived internally from PAGE_ALIGN(dmabuf->size), and
the memcg reference is stored into (or cleared from) dmabuf->memcg by
the helpers themselves, removing the need for callers to manage those
details. The release path in dma_buf_release() is reduced to a single
mem_cgroup_uncharge_dmabuf() call.

This decouples accounting from the backing page allocator, avoiding the
need for every exporter to pass __GFP_ACCOUNT and enabling cross-cgroup
charge transfer, which is impractical with page-level accounting.

Remove __GFP_ACCOUNT from system-heap page allocations (added by
commit 3c227be90659 ("dma-buf: system_heap: account for system heap
allocation in memcg")) to avoid double counting once callers adopt
this path.

The next patch wires up the charging policy in the dma-heap ioctl.

Signed-off-by: T.J. Mercier <tjmercier@google.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 Documentation/admin-guide/cgroup-v2.rst |  4 +++
 drivers/dma-buf/dma-buf.c               |  3 ++
 drivers/dma-buf/heaps/system_heap.c     |  2 --
 include/linux/dma-buf.h                 |  4 +++
 include/linux/memcontrol.h              | 20 ++++++++++++
 mm/memcontrol.c                         | 54 +++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 86a2a0099178..a3265a4a953e 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1662,6 +1662,10 @@ The following nested keys are defined.
 		Amount of memory used for storing in-kernel data
 		structures.
 
+	  dmabuf (npn)
+		Amount of memory used for exported DMA buffers allocated by the cgroup.
+		Stays with the allocating cgroup regardless of how the buffer is shared.
+
 	  workingset_refault_anon
 		Number of refaults of previously evicted anonymous pages.
 
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 4c9add51f9ef..f31f49ebd0af 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -14,6 +14,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/dma-buf.h>
+#include <linux/memcontrol.h>
 #include <linux/dma-fence.h>
 #include <linux/dma-fence-unwrap.h>
 #include <linux/anon_inodes.h>
@@ -180,6 +181,8 @@ static void dma_buf_release(struct dentry *dentry)
 	 */
 	BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
 
+	mem_cgroup_uncharge_dmabuf(dmabuf);
+
 	dmabuf->ops->release(dmabuf);
 
 	if (dmabuf->resv == (struct dma_resv *)&dmabuf[1])
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index c8959eadc71d..0a99ac0183a8 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -388,8 +388,6 @@ static struct page *alloc_largest_available(unsigned long size,
 		if (max_order < orders[i])
 			continue;
 		flags = order_flags[i];
-		if (mem_accounting)
-			flags |= __GFP_ACCOUNT;
 		page = alloc_pages(flags, orders[i]);
 		if (!page)
 			continue;
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index d15b2b31d3c9..23ec0ff23fba 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -27,6 +27,7 @@
 struct device;
 struct dma_buf;
 struct dma_buf_attachment;
+struct mem_cgroup;
 
 /**
  * struct dma_buf_ops - operations possible on struct dma_buf
@@ -429,6 +430,9 @@ struct dma_buf {
 
 		__poll_t active;
 	} cb_in, cb_out;
+
+	/** @memcg: the cgroup to which this buffer is currently attributed */
+	struct mem_cgroup *memcg;
 };
 
 /**
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 7d1c0ce189a8..aa08fbb0c26c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -10,6 +10,9 @@
 
 #ifndef _LINUX_MEMCONTROL_H
 #define _LINUX_MEMCONTROL_H
+
+struct dma_buf;
+
 #include <linux/cgroup.h>
 #include <linux/vm_event_item.h>
 #include <linux/hardirq.h>
@@ -40,6 +43,7 @@ enum memcg_stat_item {
 	MEMCG_ZSWAP_B,
 	MEMCG_ZSWAPPED,
 	MEMCG_ZSWAP_INCOMP,
+	MEMCG_DMABUF,
 	MEMCG_NR_STAT,
 };
 
@@ -667,6 +671,9 @@ int mem_cgroup_charge_hugetlb(struct folio* folio, gfp_t gfp);
 int mem_cgroup_swapin_charge_folio(struct folio *folio, unsigned short id,
 				   struct mm_struct *mm, gfp_t gfp);
 
+bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
+			      gfp_t gfp_mask);
+
 void __mem_cgroup_uncharge(struct folio *folio);
 
 /**
@@ -682,6 +689,8 @@ static inline void mem_cgroup_uncharge(struct folio *folio)
 	__mem_cgroup_uncharge(folio);
 }
 
+void mem_cgroup_uncharge_dmabuf(struct dma_buf *dmabuf);
+
 void __mem_cgroup_uncharge_folios(struct folio_batch *folios);
 static inline void mem_cgroup_uncharge_folios(struct folio_batch *folios)
 {
@@ -1167,10 +1176,21 @@ static inline int mem_cgroup_swapin_charge_folio(struct folio *folio,
 	return 0;
 }
 
+static inline bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf,
+					    struct mem_cgroup *memcg,
+					    gfp_t gfp_mask)
+{
+	return true;
+}
+
 static inline void mem_cgroup_uncharge(struct folio *folio)
 {
 }
 
+static inline void mem_cgroup_uncharge_dmabuf(struct dma_buf *dmabuf)
+{
+}
+
 static inline void mem_cgroup_uncharge_folios(struct folio_batch *folios)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 856a7d07586c..e94222237d31 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -69,6 +69,7 @@
 #include <net/ip.h>
 #include "slab.h"
 #include "memcontrol-v1.h"
+#include <linux/dma-buf.h>
 
 #include <linux/uaccess.h>
 
@@ -442,6 +443,7 @@ static const unsigned int memcg_stat_items[] = {
 	MEMCG_ZSWAP_B,
 	MEMCG_ZSWAPPED,
 	MEMCG_ZSWAP_INCOMP,
+	MEMCG_DMABUF,
 };
 
 #define NR_MEMCG_NODE_STAT_ITEMS ARRAY_SIZE(memcg_node_stat_items)
@@ -1625,6 +1627,7 @@ static const struct memory_stat memory_stats[] = {
 #ifdef CONFIG_HUGETLB_PAGE
 	{ "hugetlb",			NR_HUGETLB			},
 #endif
+	{ "dmabuf",			MEMCG_DMABUF			},
 
 	/* The memory events */
 	{ "workingset_refault_anon",	WORKINGSET_REFAULT_ANON		},
@@ -5658,6 +5661,57 @@ void mem_cgroup_flush_workqueue(void)
 	flush_workqueue(memcg_wq);
 }
 
+/**
+ * mem_cgroup_charge_dmabuf - charge a dma-buf to a memcg
+ * @dmabuf: buffer to charge
+ * @memcg: memcg to charge; caller must hold a reference
+ * @gfp_mask: reclaim mode
+ *
+ * Charges the memory backing @dmabuf to @memcg and, on success, stores
+ * @memcg in @dmabuf->memcg (transferring the caller's reference).
+ * Returns %true on success, %false if the charge would exceed the limit.
+ * On failure the caller retains its reference to @memcg.
+ */
+bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
+			      gfp_t gfp_mask)
+{
+	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
+
+	if (mem_cgroup_disabled())
+		return true;
+
+	if (try_charge(memcg, gfp_mask, nr_pages))
+		return false;
+
+	mod_memcg_state(memcg, MEMCG_DMABUF, nr_pages);
+	dmabuf->memcg = memcg;
+	return true;
+}
+
+/**
+ * mem_cgroup_uncharge_dmabuf - uncharge a dma-buf from its memcg
+ * @dmabuf: buffer to uncharge
+ *
+ * Reverses a successful mem_cgroup_charge_dmabuf(). Drops the memcg
+ * reference stored in @dmabuf->memcg and clears the pointer.
+ * Safe to call when @dmabuf->memcg is %NULL.
+ */
+void mem_cgroup_uncharge_dmabuf(struct dma_buf *dmabuf)
+{
+	unsigned int nr_pages;
+	struct mem_cgroup *memcg = dmabuf->memcg;
+
+	if (!memcg)
+		return;
+
+	nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
+	mod_memcg_state(memcg, MEMCG_DMABUF, -nr_pages);
+	if (!mem_cgroup_is_root(memcg))
+		refill_stock(memcg, nr_pages);
+	mem_cgroup_put(memcg);
+	dmabuf->memcg = NULL;
+}
+
 static int __init cgroup_memory(char *s)
 {
 	char *token;

-- 
2.55.0

Re: [PATCH RFC v2 1/8] memcg: Add MEMCG_DMABUF accounting infrastructure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dma-buf: system_heap: uncharged page allocation in system_heap_allocate()
--

commit c1f3a1d4f988a0cc1048e5d03e0a2f1959fbf0ee
Author: T.J. Mercier <tjmercier@google.com>

memcg: Add MEMCG_DMABUF accounting infrastructure

This commit introduces a MEMCG_DMABUF stat counter along with
mem_cgroup_charge_dmabuf() and mem_cgroup_uncharge_dmabuf() helpers to
track dma-buf memory charges on struct dma_buf. It also removes
__GFP_ACCOUNT from system-heap page allocations in preparation for wiring
up charging in the dma-heap ioctl.

[ ... ]

> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index c8959eadc71d1..0a99ac0183a81 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -388,8 +388,6 @@ static struct page *alloc_largest_available(unsigned long size,
>  		if (max_order < orders[i])
>  			continue;
>  		flags = order_flags[i];
> -		if (mem_accounting)
> -			flags |= __GFP_ACCOUNT;
>  		page = alloc_pages(flags, orders[i]);
>  		if (!page)
>  			continue;

[Severity: High]
Does removing __GFP_ACCOUNT from alloc_largest_available() in
drivers/dma-buf/heaps/system_heap.c introduce a regression in memory
cgroup limit enforcement and bisectability?

First, this commit removes __GFP_ACCOUNT before any caller of
mem_cgroup_charge_dmabuf() exists, which leaves dma_heap.mem_accounting=1
non-functional at this commit until the subsequent patch ("dma-heap:
charge dma-buf memory via explicit memcg") wires up the call in
dma_heap_ioctl_allocate().

Second, even after dma_heap_ioctl_allocate() calls
mem_cgroup_charge_dmabuf(), mem_cgroup_charge_dmabuf() in mm/memcontrol.c
operates on an already-exported struct dma_buf:

mm/memcontrol.c:mem_cgroup_charge_dmabuf() {
	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;

	if (mem_cgroup_disabled())
		return true;

	if (try_charge(memcg, gfp_mask, nr_pages))
		return false;
    ...
}

When userspace invokes ioctl(DMA_HEAP_IOCTL_ALLOC) on /dev/dma_heap/system,
the call chain is:

  dma_heap_ioctl()
    -> dma_heap_ioctl_allocate()
      -> system_heap_allocate()
        -> alloc_largest_available()

Inside drivers/dma-buf/heaps/system_heap.c:system_heap_allocate(), the
allocation loop allocates and zeroes (via GFP_HIGHUSER | __GFP_ZERO) all
backing pages for the entire requested buffer before exporting the
dma_buf:

drivers/dma-buf/heaps/system_heap.c:system_heap_allocate() {
    ...
	while (size_remaining > 0) {
		/*
		 * Avoid trying to allocate memory if the process
		 * has been killed by SIGKILL
		 */
		if (fatal_signal_pending(current)) {
			ret = -EINTR;
			goto free_buffer;
		}

		page = alloc_largest_available(size_remaining, max_order);
		if (!page)
			goto free_buffer;

		list_add_tail(&page->lru, &pages);
		size_remaining -= page_size(page);
		max_order = compound_order(page);
		i++;
	}
    ...
}

system_heap_allocate() then allocates the scatter-gather table via
sg_alloc_table(), decrypts pages when cc_shared_buffer(buffer) is true,
and exports the struct dma_buf via dma_buf_export() before returning to
dma_heap_ioctl_allocate() where mem_cgroup_charge_dmabuf() is finally
called.

Previously, passing __GFP_ACCOUNT to alloc_pages() in
alloc_largest_available() charged each page to the calling task's memcg
during allocation, aborting the loop in system_heap_allocate() as soon as
the cgroup's memory.max limit was reached.

With charging deferred until after system_heap_allocate() completes, can a
task inside a memory-limited cgroup request a buffer much larger than its
memory.max limit and force the global page allocator to allocate and zero
arbitrary amounts of physical memory, triggering host-wide memory reclaim
or the global out-of-memory killer before mem_cgroup_charge_dmabuf() ever
checks the cgroup limit?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=1

[PATCH RFC v2 2/8] dma-heap: charge dma-buf memory via explicit memcg

[Albert Esteve]

On embedded platforms a central process often allocates dma-buf
memory on behalf of client applications. Without a way to
attribute the charge to the requesting client's cgroup, the
cost lands on the allocator, making per-cgroup memory limits
ineffective for the actual consumers.

Add charge_fd to struct dma_heap_allocation_data. When set to
a valid pidfd or cgroupfd, DMA_HEAP_IOCTL_ALLOC resolves the target
task's or cgroup's memcg and charges the buffer there via
mem_cgroup_charge_dmabuf() inside dma_heap_ioctl_allocate().
Without charge_fd, and with the mem_accounting module parameter enabled,
the buffer is charged to the allocator's own cgroup.

Usage examples:

  1. Central allocator charging to a client at allocation time.
     The allocator knows the client's PID (e.g., from binder's
     sender_pid) and uses pidfd to attribute the charge:

       pid_t client_pid = txn->sender_pid;
       int pidfd = pidfd_open(client_pid, 0);

       struct dma_heap_allocation_data alloc = {
           .len         = buffer_size,
           .fd_flags    = O_RDWR | O_CLOEXEC,
           .charge_fd   = pidfd,
       };
       ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &alloc);
       close(pidfd);
       /* alloc.fd is now charged to client's cgroup */

  2. Default allocation (no pidfd, mem_accounting=1).
     When charge_fd is not set and the mem_accounting module
     parameter is enabled, the buffer is charged to the allocator's
     own cgroup:

       struct dma_heap_allocation_data alloc = {
           .len      = buffer_size,
           .fd_flags = O_RDWR | O_CLOEXEC,
       };
       ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &alloc);
       /* charged to current process's cgroup */

Current limitations:

 - Single-owner model: a dma-buf carries one memcg charge regardless of
   how many processes share it. Means only the first owner (and exporter)
   of the shared buffer bears the charge.
 - Only memcg accounting supported. While this makes sense for system
   heap buffers, other heaps (e.g., CMA heaps) will require selectively
   charging also for the dmem controller.
 - Charging CMA-backed heaps to memcg charges unreclaimable memory:
   when a cgroup hits its limit, try_charge_memcg() may trigger a
   reclaim pass that cannot free CMA pages. This is consistent with
   other unreclaimable memcg charges. A refinement for the next version
   could move charging into per-heap callbacks and skip the futile
   reclaim.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 Documentation/admin-guide/cgroup-v2.rst |  5 +--
 drivers/dma-buf/dma-heap.c              | 54 +++++++++++++++++++++++++++++++--
 include/uapi/linux/dma-heap.h           |  7 +++++
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index a3265a4a953e..66ddba678784 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1663,8 +1663,9 @@ The following nested keys are defined.
 		structures.
 
 	  dmabuf (npn)
-		Amount of memory used for exported DMA buffers allocated by the cgroup.
-		Stays with the allocating cgroup regardless of how the buffer is shared.
+		Amount of memory used for exported DMA buffers allocated by or on
+		behalf of the cgroup. Stays with the allocating cgroup regardless
+		of how the buffer is shared.
 
 	  workingset_refault_anon
 		Number of refaults of previously evicted anonymous pages.
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8a6c2b6e0cc6..be77a4c168ab 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -7,14 +7,18 @@
  */
 
 #include <linux/cdev.h>
+#include <linux/cgroup.h>
 #include <linux/device.h>
 #include <linux/dma-buf.h>
 #include <linux/dma-heap.h>
+#include <linux/memcontrol.h>
+#include <linux/sched/mm.h>
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/minmax.h>
 #include <linux/nospec.h>
+#include <linux/pidfd.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/xarray.h>
@@ -75,9 +79,14 @@ static int dma_heap_open(struct inode *inode, struct file *file)
 
 static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
 {
+	struct cgroup *cgrp;
+	struct cgroup_subsys_state *css;
 	struct dma_heap_allocation_data *heap_allocation = data;
 	struct dma_heap *heap = file->private_data;
 	struct dma_buf *dmabuf;
+	struct mem_cgroup *memcg = NULL;
+	struct task_struct *task;
+	unsigned int pidfd_flags;
 	int fd;
 	size_t len;
 
@@ -90,15 +99,56 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
 	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
 		return ERR_PTR(-EINVAL);
 
+	if (heap_allocation->charge_fd) {
+		cgrp = cgroup_get_from_fd(heap_allocation->charge_fd);
+		if (!IS_ERR(cgrp)) {
+			/* charge_fd is a cgroupfd */
+			rcu_read_lock();
+			css = cgroup_css(cgrp, &memory_cgrp_subsys);
+			if (!css || !css_tryget_online(css)) {
+				rcu_read_unlock();
+				cgroup_put(cgrp);
+				return ERR_PTR(-ENOENT);
+			}
+
+			memcg = mem_cgroup_from_css(css);
+			rcu_read_unlock();
+			cgroup_put(cgrp);
+		} else {
+			/* charge_fd is a pidfd  */
+			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);
+			if (IS_ERR(task))
+				return ERR_CAST(task);
+
+			memcg = get_mem_cgroup_from_mm(task->mm);
+			put_task_struct(task);
+		}
+	}
+
 	len = PAGE_ALIGN(heap_allocation->len);
-	if (!len)
+	if (!len) {
+		mem_cgroup_put(memcg);
 		return ERR_PTR(-EINVAL);
+	}
 
 	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
 				     heap_allocation->heap_flags);
 
-	if (IS_ERR(dmabuf))
+	if (IS_ERR(dmabuf)) {
+		mem_cgroup_put(memcg);
 		return dmabuf;
+	}
+
+	if (!memcg && mem_accounting)
+		memcg = get_mem_cgroup_from_mm(current->mm);
+
+	if (memcg) {
+		if (!mem_cgroup_charge_dmabuf(dmabuf, memcg, GFP_KERNEL)) {
+			mem_cgroup_put(memcg);
+			dma_buf_put(dmabuf);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
 
 	fd = get_unused_fd_flags(heap_allocation->fd_flags);
 	if (fd < 0) {
diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
index a4cf716a49fa..bfcd1c25bc7e 100644
--- a/include/uapi/linux/dma-heap.h
+++ b/include/uapi/linux/dma-heap.h
@@ -29,6 +29,11 @@
  *			handle to the allocated dma-buf
  * @fd_flags:		file descriptor flags used when allocating
  * @heap_flags:		flags passed to heap
+ * @charge_fd:		optional pidfd or cgroupfd whose cgroup should be
+ *			charged for this allocation; 0 means charge the calling
+ *			process's cgroup when the mem_accounting module parameter
+ *			is enabled, or skip charging otherwise
+ * @__padding:		reserved, must be zero
  *
  * Provided by userspace as an argument to the ioctl
  */
@@ -37,6 +42,8 @@ struct dma_heap_allocation_data {
 	__u32 fd;
 	__u32 fd_flags;
 	__u64 heap_flags;
+	__u32 charge_fd;
+	__u32 __padding;
 };
 
 #define DMA_HEAP_IOC_MAGIC		'H'

-- 
2.55.0

Re: [PATCH RFC v2 2/8] dma-heap: charge dma-buf memory via explicit memcg

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] dma-heap: UAF and wrong memcg fallback on task->mm in dma_heap_ioctl_allocate()
- [High] dma-heap: missing permission check on charge_fd in dma_heap_ioctl_allocate()
- [Medium] dma-heap: missing __padding validation in dma_heap_ioctl_allocate()
- [Medium] dma-heap: physical pages allocated before memcg charge in dma_heap_ioctl_allocate()
- [Low] dma-heap: build failure in dma_heap_ioctl_allocate() when CONFIG_MEMCG=n
--

commit 662cb8c6d2fd1fbf166b582f9ede716df0713b4a
Author: Albert Esteve <aesteve@redhat.com>

dma-heap: charge dma-buf memory via explicit memcg

This commit adds a charge_fd field to struct dma_heap_allocation_data so
DMA_HEAP_IOCTL_ALLOC can resolve a target cgroup from a cgroupfd or pidfd
and charge the allocated dma-buf to that memcg via
mem_cgroup_charge_dmabuf(). When charge_fd is unset and mem_accounting is
enabled, the allocation is charged to the calling process's memcg.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 8a6c2b6e0cc60..be77a4c168ab0 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -7,14 +7,18 @@
>   */
>  
>  #include <linux/cdev.h>
> +#include <linux/cgroup.h>
>  #include <linux/device.h>
>  #include <linux/dma-buf.h>
>  #include <linux/dma-heap.h>
> +#include <linux/memcontrol.h>
> +#include <linux/sched/mm.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/minmax.h>
>  #include <linux/nospec.h>
> +#include <linux/pidfd.h>
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
>  #include <linux/xarray.h>
> @@ -75,9 +79,14 @@ static int dma_heap_open(struct inode *inode, struct file *file)
>  
>  static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  {
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
>  	struct dma_heap_allocation_data *heap_allocation = data;
>  	struct dma_heap *heap = file->private_data;
>  	struct dma_buf *dmabuf;
> +	struct mem_cgroup *memcg = NULL;
> +	struct task_struct *task;
> +	unsigned int pidfd_flags;
>  	int fd;
>  	size_t len;
>  
> @@ -90,15 +99,56 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (heap_allocation->charge_fd) {
> +		cgrp = cgroup_get_from_fd(heap_allocation->charge_fd);
> +		if (!IS_ERR(cgrp)) {
> +			/* charge_fd is a cgroupfd */
> +			rcu_read_lock();
> +			css = cgroup_css(cgrp, &memory_cgrp_subsys);
> +			if (!css || !css_tryget_online(css)) {
> +				rcu_read_unlock();
> +				cgroup_put(cgrp);
> +				return ERR_PTR(-ENOENT);
> +			}
> +
> +			memcg = mem_cgroup_from_css(css);
> +			rcu_read_unlock();
> +			cgroup_put(cgrp);

[Severity: Low]
Will this build when CONFIG_DMABUF_HEAPS=y and CONFIG_MEMCG=n (or
CONFIG_CGROUPS=n)?

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS:

drivers/dma-buf/Kconfig:
    menuconfig DMABUF_HEAPS
        bool "DMA-BUF Userland Memory Heaps"
        select DMA_SHARED_BUFFER

In include/linux/cgroup_subsys.h, SUBSYS(memory) is guarded by
#if IS_ENABLED(CONFIG_MEMCG), so memory_cgrp_subsys is only declared when
both CONFIG_CGROUPS and CONFIG_MEMCG are enabled:

include/linux/cgroup_subsys.h:
    #if IS_ENABLED(CONFIG_MEMCG)
    SUBSYS(memory)
    #endif

Additionally, when CONFIG_CGROUPS=n, include/linux/cgroup.h does not provide
declarations or stubs for cgroup_get_from_fd(), cgroup_css(),
css_tryget_online(), or cgroup_put(). Referencing memory_cgrp_subsys and
those helpers unconditionally in drivers/dma-buf/dma-heap.c:
dma_heap_ioctl_allocate() breaks the build when CONFIG_MEMCG=n. Should this
resolution block be guarded by IS_ENABLED(CONFIG_MEMCG) or moved into a
memcontrol helper with a !CONFIG_MEMCG stub?

> +		} else {
> +			/* charge_fd is a pidfd  */
> +			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);
> +			if (IS_ERR(task))
> +				return ERR_CAST(task);
> +
> +			memcg = get_mem_cgroup_from_mm(task->mm);
> +			put_task_struct(task);
> +		}
> +	}

[Severity: Critical]
Can passing task->mm directly to get_mem_cgroup_from_mm(task->mm) in
drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() trigger a
use-after-free on struct mm_struct or silently charge the wrong memcg?

pidfd_get_task() only increments task->usage and does not hold
task_lock(task) or pin task->mm via get_task_mm(task). If the target task
concurrently exits in kernel/exit.c:exit_mm() or execs in
fs/exec.c:exec_mmap() -> exec_mm_put_old(), it clears or replaces task->mm
under task_lock() and drops its reference via mmput(mm) -> __mmdrop(mm) ->
free_mm(mm):

kernel/exit.c:exit_mm() {
    ...
    task_lock(current);
    smp_mb__after_spinlock();
    local_irq_disable();
    current->mm = NULL;
    membarrier_update_current_mm(NULL);
    enter_lazy_tlb(mm, current);
    local_irq_enable();
    task_unlock(current);
    mmap_read_unlock(mm);
    mm_update_next_owner(mm);
    mmput(mm);
}

Because mm_cachep is not SLAB_TYPESAFE_BY_RCU, free_mm() frees the mm_struct
immediately while mm/memcontrol.c:get_mem_cgroup_from_mm() dereferences
rcu_dereference(mm->owner):

mm/memcontrol.c:get_mem_cgroup_from_mm() {
    if (unlikely(!mm)) {
        memcg = active_memcg();
        if (unlikely(memcg)) {
            /* remote memcg must hold a ref */
            css_get(&memcg->css);
            return memcg;
        }
        mm = current->mm;
        if (unlikely(!mm))
            return root_mem_cgroup;
    }

    rcu_read_lock();
    do {
        memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
    ...
}

In addition, if task->mm is already NULL when charge_fd is resolved (for
instance, a zombie task after exit_mm() whose pidfd is still valid, a
thread-group leader that exited via pthread_exit() while other threads
remain alive, or a kernel thread), get_mem_cgroup_from_mm(NULL) falls back
to current->mm and silently charges the central allocator's own cgroup
(even when mem_accounting is disabled) instead of failing with an error
such as -ESRCH. Should this use get_task_mm(task) / mmput(mm) and return an
error if mm is NULL, or resolve the memcg directly from task under RCU?

[Severity: High]
What prevents an unprivileged process with access to /dev/dma_heap/* from
passing an arbitrary pidfd or read-only cgroupfd in charge_fd to escape its
own memory.max limit or trigger OOM kills in another cgroup?

In drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate(), charge_fd is
resolved via cgroup_get_from_fd() or pidfd_get_task() and charged via
mem_cgroup_charge_dmabuf() without checking whether the caller has
permission to charge the target cgroup or task:

1. sys_pidfd_open() in kernel/pid.c allows opening a pidfd for any task
   visible in the caller's PID namespace (such as PID 1 in the root cgroup
   or a task in a peer cgroup) without permission checks.
2. cgroup_get_from_fd() in kernel/cgroup/cgroup.c only checks that the file
   descriptor refers to a cgroup v2 directory and does not check write
   permission (unlike cgroup_css_set_fork(), which checks
   cgroup_may_write()), so any world-readable /sys/fs/cgroup directory
   opened with O_RDONLY | O_DIRECTORY or O_PATH can be charged.

While later commits in this series ("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", and "selinux: Restrict dma-buf direct cgroup charging") add LSM
hooks and SELinux policy checks, there is no baseline DAC,
cgroup_may_write(), ptrace_may_access(), or capability check (such as
CAP_SYS_RESOURCE). On systems without SELinux enabled (or with
CONFIG_SECURITY=n or other LSMs), or on SELinux systems where cgroup
directories lack explicit security.selinux xattrs (-ENODATA in
selinux_dma_buf_charge_cgroup()) or tasks share a domain, unprivileged
cross-cgroup charging remains unrestricted. Should dma_heap_ioctl_allocate()
enforce baseline permission checks on the target cgroup and task in addition
to the LSM hooks?

> +
>  	len = PAGE_ALIGN(heap_allocation->len);
> -	if (!len)
> +	if (!len) {
> +		mem_cgroup_put(memcg);
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
>  				     heap_allocation->heap_flags);
>  
> -	if (IS_ERR(dmabuf))
> +	if (IS_ERR(dmabuf)) {
> +		mem_cgroup_put(memcg);
>  		return dmabuf;
> +	}
> +
> +	if (!memcg && mem_accounting)
> +		memcg = get_mem_cgroup_from_mm(current->mm);
> +
> +	if (memcg) {
> +		if (!mem_cgroup_charge_dmabuf(dmabuf, memcg, GFP_KERNEL)) {
> +			mem_cgroup_put(memcg);
> +			dma_buf_put(dmabuf);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}

[Severity: Medium]
Could calling heap->ops->allocate() before mem_cgroup_charge_dmabuf() in
drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() allow a process in a
memory-limited cgroup to allocate and pin physical memory far beyond its
memory.max limit before the charge check happens?

Prior to commit "memcg: Add MEMCG_DMABUF accounting infrastructure" in this
series, system_heap_allocate() passed __GFP_ACCOUNT to alloc_pages() when
mem_accounting was enabled, charging the memcg incrementally as each page
was allocated and aborting as soon as memory.max was reached.

With __GFP_ACCOUNT removed from system_heap_allocate(),
dma_heap_ioctl_allocate() now allocates and zeroes the entire requested len
of physical backing pages inside heap->ops->allocate() before
mem_cgroup_charge_dmabuf() calls try_charge(). A task confined to a small
memory.max limit can request a multi-gigabyte buffer and force the kernel to
allocate all physical pages (potentially triggering global reclaim or global
OOM) before try_charge() rejects the charge. In addition, if the allocation
would only fit after reclaiming from the target cgroup, allocating the
physical pages before try_charge() prevents cgroup reclaim from freeing the
target cgroup's pages prior to the physical allocation. Could the memcg be
charged before invoking heap->ops->allocate() and uncharged if the
allocation fails?

[ ... ]

> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
> index a4cf716a49fa6..bfcd1c25bc7ea 100644
> --- a/include/uapi/linux/dma-heap.h
> +++ b/include/uapi/linux/dma-heap.h
> @@ -29,6 +29,11 @@
>   *			handle to the allocated dma-buf
>   * @fd_flags:		file descriptor flags used when allocating
>   * @heap_flags:		flags passed to heap
> + * @charge_fd:		optional pidfd or cgroupfd whose cgroup should be
> + *			charged for this allocation; 0 means charge the calling
> + *			process's cgroup when the mem_accounting module parameter
> + *			is enabled, or skip charging otherwise
> + * @__padding:		reserved, must be zero
>   *
>   * Provided by userspace as an argument to the ioctl
>   */
> @@ -37,6 +42,8 @@ struct dma_heap_allocation_data {
>  	__u32 fd;
>  	__u32 fd_flags;
>  	__u64 heap_flags;
> +	__u32 charge_fd;
> +	__u32 __padding;
>  };

[Severity: Medium]
Should drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() reject non-zero
values in heap_allocation->__padding with -EINVAL?

include/uapi/linux/dma-heap.h documents @__padding as "reserved, must be
zero", and dma_heap_ioctl_allocate() validates heap_allocation->fd,
heap_allocation->fd_flags, and heap_allocation->heap_flags, but never checks
heap_allocation->__padding:

drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() {
    if (heap_allocation->fd)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->charge_fd) {
    ...
}

Whereas a later commit in this series validates arg.__padding != 0 in
dma_buf_xfer_charge(), dma_heap_ioctl_allocate() leaves __padding unchecked
throughout the series. Also, using charge_fd == 0 as the unset sentinel
without a flag in heap_flags makes it impossible to pass valid file
descriptor 0 as charge_fd and causes older kernels to silently ignore
charge_fd. Note that extending struct dma_heap_allocation_data here also
causes tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c:
dmabuf_heap_alloc_newer() (which places .garbage1 = 0xffffffff at this
offset) to fail with -EBADF at this commit until the selftest struct layout
is updated in commit 0809a03b2fbc81e6f2b637ef5c0744b2c8ae99f1
("selftests/dmabuf-heaps: Add dma-buf memcg accounting tests").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=2

[PATCH RFC v2 3/8] security: dma-buf: Add dma_buf_charge_task LSM hook

[Albert Esteve]

DMA_HEAP_IOCTL_ALLOC accepts a charge_fd field that,
when set, causes the allocation to be charged to an arbitrary
process's cgroup rather than the caller's.

Without an access-control point, any process that holds a handle
to a dma-heap device node can charge unlimited memory to any other
process's cgroup, potentially exhausting that cgroup's limit and
triggering OOM kills independent of the victim's own activity or
privileges.

Add security_dma_buf_charge_task(), called in dma_heap_ioctl_allocate()
when charge_fd refers to another process. The hook receives
the credentials of the allocating process (from) and the credentials
of the process whose cgroup will be charged (to), giving security
modules a controlled enforcement point for cross-cgroup dma-buf
attribution policy.

When CONFIG_SECURITY is not set the hook compiles to an inline
returning 0, adding no overhead to the fast path.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 drivers/dma-buf/dma-heap.c    | 12 +++++++++++-
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  7 +++++++
 security/security.c           | 16 ++++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index be77a4c168ab..c31573917886 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -19,6 +19,7 @@
 #include <linux/minmax.h>
 #include <linux/nospec.h>
 #include <linux/pidfd.h>
+#include <linux/security.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/xarray.h>
@@ -81,13 +82,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
 {
 	struct cgroup *cgrp;
 	struct cgroup_subsys_state *css;
+	const struct cred *tcred;
 	struct dma_heap_allocation_data *heap_allocation = data;
 	struct dma_heap *heap = file->private_data;
 	struct dma_buf *dmabuf;
 	struct mem_cgroup *memcg = NULL;
 	struct task_struct *task;
 	unsigned int pidfd_flags;
-	int fd;
+	int fd, ret;
 	size_t len;
 
 	if (heap_allocation->fd)
@@ -120,6 +122,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
 			if (IS_ERR(task))
 				return ERR_CAST(task);
 
+			tcred = get_task_cred(task);
+			ret = security_dma_buf_charge_task(current_cred(), tcred);
+			put_cred(tcred);
+			if (ret) {
+				put_task_struct(task);
+				return ERR_PTR(ret);
+			}
+
 			memcg = get_mem_cgroup_from_mm(task->mm);
 			put_task_struct(task);
 		}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 65c9609ec207..0f1138e16dbe 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -43,6 +43,7 @@ LSM_HOOK(int, 0, capset, struct cred *new, const struct cred *old,
 	 const kernel_cap_t *permitted)
 LSM_HOOK(int, 0, capable, const struct cred *cred, struct user_namespace *ns,
 	 int cap, unsigned int opts)
+LSM_HOOK(int, 0, dma_buf_charge_task, const struct cred *from, const struct cred *to)
 LSM_HOOK(int, 0, quotactl, int cmds, int type, int id, const struct super_block *sb)
 LSM_HOOK(int, 0, quota_on, struct dentry *dentry)
 LSM_HOOK(int, 0, syslog, int type)
diff --git a/include/linux/security.h b/include/linux/security.h
index 153e9043058f..89a35eb07b8c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -350,6 +350,7 @@ int security_capable(const struct cred *cred,
 		       struct user_namespace *ns,
 		       int cap,
 		       unsigned int opts);
+int security_dma_buf_charge_task(const struct cred *from, const struct cred *to);
 int security_quotactl(int cmds, int type, int id, const struct super_block *sb);
 int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
@@ -701,6 +702,12 @@ static inline int security_capable(const struct cred *cred,
 	return cap_capable(cred, ns, cap, opts);
 }
 
+static inline int security_dma_buf_charge_task(const struct cred *from,
+					       const struct cred *to)
+{
+	return 0;
+}
+
 static inline int security_quotactl(int cmds, int type, int id,
 				     const struct super_block *sb)
 {
diff --git a/security/security.c b/security/security.c
index 2ee276ab15c5..4a16e4076aab 100644
--- a/security/security.c
+++ b/security/security.c
@@ -660,6 +660,22 @@ int security_capable(const struct cred *cred,
 	return call_int_hook(capable, cred, ns, cap, opts);
 }
 
+/**
+ * security_dma_buf_charge_task() - Check if cross-cgroup dma-heap charging is allowed
+ * @from: credentials of the allocating process
+ * @to: credentials of the process to charge
+ *
+ * Check whether the process with credentials @from is allowed to allocate
+ * dma-heap memory and charge it to the cgroup of the process with credentials
+ * @to.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_dma_buf_charge_task(const struct cred *from, const struct cred *to)
+{
+	return call_int_hook(dma_buf_charge_task, from, to);
+}
+
 /**
  * security_quotactl() - Check if a quotactl() syscall is allowed for this fs
  * @cmds: commands

-- 
2.55.0

Re: [PATCH RFC v2 3/8] security: dma-buf: Add dma_buf_charge_task LSM hook

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] dma-buf: heaps: UAF on remote task->mm in dma_heap_ioctl_allocate()
- [High] dma-buf: allocate() runs before memcg charge in dma_heap_ioctl_allocate()
- [Medium] dma-buf: heaps: unguarded cgroup symbols in dma_heap_ioctl_allocate()
--

commit 4f97d3e850a29378e70a6ab0cb15c71860e1fcea
Author: Albert Esteve <aesteve@redhat.com>

security: dma-buf: Add dma_buf_charge_task LSM hook

This commit adds the security_dma_buf_charge_task() LSM hook and calls it
in dma_heap_ioctl_allocate() when charge_fd refers to a process via pidfd,
allowing security modules to control cross-cgroup dma-buf charging.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index be77a4c168ab0..c315739178867 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c

[ ... ]

> @@ -81,13 +82,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  {
>  	struct cgroup *cgrp;
>  	struct cgroup_subsys_state *css;
> +	const struct cred *tcred;
>  	struct dma_heap_allocation_data *heap_allocation = data;
>  	struct dma_heap *heap = file->private_data;
>  	struct dma_buf *dmabuf;
>  	struct mem_cgroup *memcg = NULL;
>  	struct task_struct *task;
>  	unsigned int pidfd_flags;
> -	int fd;
> +	int fd, ret;
>  	size_t len;
>  
>  	if (heap_allocation->fd)

[Severity: Medium]
Will this cause a build regression in dma_heap_ioctl_allocate() when
CONFIG_DMABUF_HEAPS=y is built with CONFIG_MEMCG=n or CONFIG_CGROUPS=n?

In drivers/dma-buf/dma-heap.c, dma_heap_ioctl_allocate() unconditionally
references cgroup_get_from_fd(), cgroup_css(), memory_cgrp_subsys,
css_tryget_online(), and cgroup_put() in the charge_fd resolution block
(introduced earlier in this series by commit "dma-heap: charge dma-buf
memory via explicit memcg" and updated here):

	if (heap_allocation->charge_fd) {
		cgrp = cgroup_get_from_fd(heap_allocation->charge_fd);
		if (!IS_ERR(cgrp)) {
			/* charge_fd is a cgroupfd */
			rcu_read_lock();
			css = cgroup_css(cgrp, &memory_cgrp_subsys);
			if (!css || !css_tryget_online(css)) {
				rcu_read_unlock();
				cgroup_put(cgrp);
				return ERR_PTR(-ENOENT);
			}

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS. However, in
include/linux/cgroup_subsys.h, SUBSYS(memory) (which declares
memory_cgrp_subsys) is guarded by:

#if IS_ENABLED(CONFIG_MEMCG)
SUBSYS(memory)
#endif

Furthermore, in include/linux/cgroup.h, the SUBSYS() declarations along
with cgroup_get_from_fd(), cgroup_css(), css_tryget_online(), and
cgroup_put() are only declared inside #ifdef CONFIG_CGROUPS without stub
definitions when CONFIG_CGROUPS is disabled. Should this path be guarded by
IS_ENABLED(CONFIG_MEMCG), or should Kconfig dependencies/stubs be added?

> @@ -120,6 +122,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  			if (IS_ERR(task))
>  				return ERR_CAST(task);
>  
> +			tcred = get_task_cred(task);
> +			ret = security_dma_buf_charge_task(current_cred(), tcred);
> +			put_cred(tcred);
> +			if (ret) {
> +				put_task_struct(task);
> +				return ERR_PTR(ret);
> +			}
> +
>  			memcg = get_mem_cgroup_from_mm(task->mm);

[Severity: Critical]
Can passing task->mm directly to get_mem_cgroup_from_mm(task->mm) in
dma_heap_ioctl_allocate() trigger a use-after-free on struct mm_struct, or
silently charge the wrong memcg when task->mm is NULL?

When resolving heap_allocation->charge_fd via pidfd_get_task(), the caller
only holds a reference on task->usage, which does not pin task->mm. After
security_dma_buf_charge_task() returns 0, dma_heap_ioctl_allocate() reads
task->mm and passes it to get_mem_cgroup_from_mm(task->mm) without holding
task_lock(task) or acquiring an mm reference via get_task_mm(task) (a
pattern introduced in commit "dma-heap: charge dma-buf memory via explicit
memcg", modified here, and duplicated in commit "dma-buf: transfer memcg
charges via DMA_BUF_IOCTL_XFER_CHARGE").

If the target task concurrently exits via do_exit() -> exit_mm() in
kernel/exit.c (or replaces its mm during exec via exec_mmap() ->
exec_mm_put_old() in fs/exec.c), it clears task->mm under task_lock() and
drops the last reference via mmput() -> __mmdrop() -> free_mm():

	local_irq_disable();
	current->mm = NULL;
	membarrier_update_current_mm(NULL);
	enter_lazy_tlb(mm, current);
	local_irq_enable();
	task_unlock(current);
	mmap_read_unlock(mm);
	mm_update_next_owner(mm);
	mmput(mm);

Because mm_cachep is not SLAB_TYPESAFE_BY_RCU and __mmdrop() frees the
mm_struct immediately without an RCU grace period, get_mem_cgroup_from_mm()
in mm/memcontrol.c can dereference freed memory at mm->owner:

	rcu_read_lock();
	do {
		memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));

In addition, if the target task is already a zombie (having completed
exit_mm() while still reachable via pidfd_get_task()) or a kernel thread,
task->mm is NULL. Even though get_task_cred(task) and
security_dma_buf_charge_task() authorized charging the target task,
get_mem_cgroup_from_mm(NULL) in mm/memcontrol.c takes the !mm fallback:

	if (unlikely(!mm)) {
		memcg = active_memcg();
		if (unlikely(memcg)) {
			/* remote memcg must hold a ref */
			css_get(&memcg->css);
			return memcg;
		}
		mm = current->mm;
		if (unlikely(!mm))
			return root_mem_cgroup;
	}

which silently charges current->mm (the allocating caller's own memcg) or
root_mem_cgroup instead of charging the target task's cgroup or returning
an error. Should this pin the mm with get_task_mm(task) / mmput(mm) (or
resolve the memcg from the task under rcu_read_lock() via
mem_cgroup_from_task())?

>  			put_task_struct(task);
>  		}

[Severity: High]
Should the memcg charge happen before heap->ops->allocate() allocates the
physical backing pages in dma_heap_ioctl_allocate()?

Immediately following the charge_fd resolution in drivers/dma-buf/dma-heap.c,
dma_heap_ioctl_allocate() invokes heap->ops->allocate() for the full
requested len before charging memcg via mem_cgroup_charge_dmabuf():

	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
				     heap_allocation->heap_flags);

	if (IS_ERR(dmabuf)) {
		mem_cgroup_put(memcg);
		return dmabuf;
	}

	if (!memcg && mem_accounting)
		memcg = get_mem_cgroup_from_mm(current->mm);

	if (memcg) {
		if (!mem_cgroup_charge_dmabuf(dmabuf, memcg, GFP_KERNEL)) {
			mem_cgroup_put(memcg);
			dma_buf_put(dmabuf);
			return ERR_PTR(-ENOMEM);
		}
	}

Because commit "memcg: Add MEMCG_DMABUF accounting infrastructure" removed
__GFP_ACCOUNT from alloc_largest_available() in
drivers/dma-buf/heaps/system_heap.c:

		flags = order_flags[i];
		page = alloc_pages(flags, orders[i]);

system_heap_allocate() allocates and zeroes the entire buffer of size len
using uncharged alloc_pages() calls before try_charge() is evaluated in
mem_cgroup_charge_dmabuf(). Can a process inside a memory-limited cgroup
(or targeting one via charge_fd in DMA_HEAP_IOCTL_ALLOC) use this window to
exhaust host physical memory and trigger the global OOM killer before the
cgroup limit check runs?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=3

[PATCH RFC v2 4/8] security: dma-buf: Add dma_buf_charge_cgroup LSM hook

[Albert Esteve]

Similar to how security_dma_buf_charge_task adds an
access-control point for pidfd for the charge_fd of a
DMA_HEAP_IOCTL_ALLOC ioctl, we need a new access-control
point for the case where charge_fd represents a cgroupfd.

Add security_dma_buf_charge_cgroup(), called in
dma_heap_ioctl_allocate() when charge_fd refers to a cgroup.
The hook receives the credentials of the requesting process (cred)
and the target cgroup to charge (cgrp), giving security modules
a controlled enforcement point for cross-cgroup dma-buf attribution
policy.

When CONFIG_SECURITY is not set the hook compiles to an inline
returning 0, adding no overhead to the fast path.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 drivers/dma-buf/dma-heap.c    |  6 ++++++
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  7 +++++++
 security/security.c           | 16 ++++++++++++++++
 4 files changed, 30 insertions(+)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index c31573917886..e06131f4655d 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -115,7 +115,13 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
 
 			memcg = mem_cgroup_from_css(css);
 			rcu_read_unlock();
+
+			ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
 			cgroup_put(cgrp);
+			if (ret) {
+				mem_cgroup_put(memcg);
+				return ERR_PTR(ret);
+			}
 		} else {
 			/* charge_fd is a pidfd  */
 			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 0f1138e16dbe..9a719e0e9ced 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -44,6 +44,7 @@ LSM_HOOK(int, 0, capset, struct cred *new, const struct cred *old,
 LSM_HOOK(int, 0, capable, const struct cred *cred, struct user_namespace *ns,
 	 int cap, unsigned int opts)
 LSM_HOOK(int, 0, dma_buf_charge_task, const struct cred *from, const struct cred *to)
+LSM_HOOK(int, 0, dma_buf_charge_cgroup, const struct cred *cred, struct cgroup *cgrp)
 LSM_HOOK(int, 0, quotactl, int cmds, int type, int id, const struct super_block *sb)
 LSM_HOOK(int, 0, quota_on, struct dentry *dentry)
 LSM_HOOK(int, 0, syslog, int type)
diff --git a/include/linux/security.h b/include/linux/security.h
index 89a35eb07b8c..4626c047a0c2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -351,6 +351,7 @@ int security_capable(const struct cred *cred,
 		       int cap,
 		       unsigned int opts);
 int security_dma_buf_charge_task(const struct cred *from, const struct cred *to);
+int security_dma_buf_charge_cgroup(const struct cred *cred, struct cgroup *cgrp);
 int security_quotactl(int cmds, int type, int id, const struct super_block *sb);
 int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
@@ -708,6 +709,12 @@ static inline int security_dma_buf_charge_task(const struct cred *from,
 	return 0;
 }
 
+static inline int security_dma_buf_charge_cgroup(const struct cred *cred,
+						 struct cgroup *cgrp)
+{
+	return 0;
+}
+
 static inline int security_quotactl(int cmds, int type, int id,
 				     const struct super_block *sb)
 {
diff --git a/security/security.c b/security/security.c
index 4a16e4076aab..eb83e8bf3dbf 100644
--- a/security/security.c
+++ b/security/security.c
@@ -676,6 +676,22 @@ int security_dma_buf_charge_task(const struct cred *from, const struct cred *to)
 	return call_int_hook(dma_buf_charge_task, from, to);
 }
 
+/**
+ * security_dma_buf_charge_cgroup() - Check if charging dma-buf memory to a cgroup is allowed
+ * @cred: credentials of the process requesting the charge
+ * @cgrp: target cgroup to charge
+ *
+ * Check whether the process with credentials @cred is allowed to charge
+ * dma-buf memory to @cgrp. Called when userspace passes a cgroupfd to
+ * identify the charge target instead of a pidfd.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_dma_buf_charge_cgroup(const struct cred *cred, struct cgroup *cgrp)
+{
+	return call_int_hook(dma_buf_charge_cgroup, cred, cgrp);
+}
+
 /**
  * security_quotactl() - Check if a quotactl() syscall is allowed for this fs
  * @cmds: commands

-- 
2.55.0

Re: [PATCH RFC v2 4/8] security: dma-buf: Add dma_buf_charge_cgroup LSM hook

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dma-buf: heaps: build failure in dma_heap_ioctl_allocate() when CONFIG_MEMCG=n
- [Medium] dma-heap: missing __padding zero check in dma_heap_ioctl_allocate()
--

commit c7d9f1c9769a263b2541e4535f61a1da114ca74b
Author: Albert Esteve <aesteve@redhat.com>

security: dma-buf: Add dma_buf_charge_cgroup LSM hook

This patch adds the security_dma_buf_charge_cgroup() LSM hook and calls it
in dma_heap_ioctl_allocate() when charge_fd refers to a cgroupfd. When
CONFIG_SECURITY is disabled, the hook compiles to a no-op inline returning 0.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index c315739178867..e06131f4655d8 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -115,7 +115,13 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  
>  			memcg = mem_cgroup_from_css(css);
>  			rcu_read_unlock();
> +
> +			ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
>  			cgroup_put(cgrp);
> +			if (ret) {
> +				mem_cgroup_put(memcg);
> +				return ERR_PTR(ret);
> +			}

[Severity: Medium]
Will drivers/dma-buf/dma-heap.c fail to build when CONFIG_DMABUF_HEAPS=y and
CONFIG_MEMCG=n (or CONFIG_CGROUPS=n)?

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS:

    menuconfig DMABUF_HEAPS
    	bool "DMA-BUF Userland Memory Heaps"
    	select DMA_SHARED_BUFFER

In dma_heap_ioctl_allocate(), the cgroupfd path introduced earlier in this
series ("dma-heap: charge dma-buf memory via explicit memcg") and updated
here unconditionally references cgroup_get_from_fd(), cgroup_css(),
memory_cgrp_subsys, css_tryget_online(), and cgroup_put():

		cgrp = cgroup_get_from_fd(heap_allocation->charge_fd);
		if (!IS_ERR(cgrp)) {
			/* charge_fd is a cgroupfd */
			rcu_read_lock();
			css = cgroup_css(cgrp, &memory_cgrp_subsys);
			if (!css || !css_tryget_online(css)) {
				rcu_read_unlock();
				cgroup_put(cgrp);
				return ERR_PTR(-ENOENT);
			}

In include/linux/cgroup_subsys.h, SUBSYS(memory) (which declares
memory_cgrp_subsys in include/linux/cgroup.h) is only emitted when
CONFIG_MEMCG is enabled:

    #if IS_ENABLED(CONFIG_MEMCG)
    SUBSYS(memory)
    #endif

Additionally, in include/linux/cgroup.h, cgroup_get_from_fd(),
css_tryget_online(), cgroup_css(), and cgroup_put() are only declared inside
#ifdef CONFIG_CGROUPS and have no stubs when CONFIG_CGROUPS=n, unlike the
helpers in include/linux/memcontrol.h.

Should the cgroupfd resolution block in dma_heap_ioctl_allocate() be guarded
with #ifdef CONFIG_MEMCG?

>  		} else {
>  			/* charge_fd is a pidfd  */
>  			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);

[Severity: Medium]
Also in dma_heap_ioctl_allocate(), should non-zero values in
heap_allocation->__padding be rejected with ERR_PTR(-EINVAL)?

Earlier in this series ("dma-heap: charge dma-buf memory via explicit
memcg"), struct dma_heap_allocation_data in include/uapi/linux/dma-heap.h was
extended with charge_fd and __padding:

     * @__padding:		reserved, must be zero
     *
     * Provided by userspace as an argument to the ioctl
     */
    struct dma_heap_allocation_data {
    	__u64 len;
    	__u32 fd;
    	__u32 fd_flags;
    	__u64 heap_flags;
    	__u32 charge_fd;
    	__u32 __padding;
    };

When a userspace process issues ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &alloc)
with a 32-byte struct dma_heap_allocation_data where alloc.__padding != 0,
dma_heap_ioctl() copies all 32 bytes into kdata and calls
dma_heap_ioctl_allocate(file, kdata). However, dma_heap_ioctl_allocate() only
checks fd, fd_flags, and heap_flags before processing charge_fd:

	if (heap_allocation->fd)
		return ERR_PTR(-EINVAL);

	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
		return ERR_PTR(-EINVAL);

	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
		return ERR_PTR(-EINVAL);

Because dma_heap_ioctl() zeroes trailing bytes when ksize > in_size:

	if (ksize > in_size)
		memset(kdata + in_size, 0, ksize - in_size);

legacy 24-byte ioctl callers already have __padding zeroed, whereas 32-byte
callers can currently pass arbitrary non-zero values in __padding without
receiving -EINVAL, preventing __padding from being safely repurposed in the
future.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=4

[PATCH RFC v2 5/8] selinux: Restrict cross-cgroup dma-buf charging

[Albert Esteve]

The security_dma_buf_charge_task() hook allows security modules
to control which processes may charge dma-buf allocations
to another process's cgroup via the charge_fd field of
DMA_HEAP_IOCTL_ALLOC. Without a policy implementation, the
hook is a no-op and the restriction is not enforced.

On SELinux-managed systems any domain with access to a
dma-heap device node can therefore exhaust another cgroup's
memory budget without restriction.

Implement selinux_dma_buf_charge_task() using avc_has_perm() with
a new dma_buf object class and a charge_task permission. Policy
authors can then grant cross-cgroup charging selectively,
for example:

  allow allocator_app_t client_app_t:dma_buf charge_task;

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 security/selinux/hooks.c            | 7 +++++++
 security/selinux/include/classmap.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 30776b0107f2..8f39d91eabc9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2204,6 +2204,12 @@ static int selinux_capable(const struct cred *cred, struct user_namespace *ns,
 	return cred_has_capability(cred, cap, opts, ns == &init_user_ns);
 }
 
+static int selinux_dma_buf_charge_task(const struct cred *from, const struct cred *to)
+{
+	return avc_has_perm(cred_sid(from), cred_sid(to),
+			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_TASK, NULL);
+}
+
 static int selinux_quotactl(int cmds, int type, int id, const struct super_block *sb)
 {
 	const struct cred *cred = current_cred();
@@ -7674,6 +7680,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(capget, selinux_capget),
 	LSM_HOOK_INIT(capset, selinux_capset),
 	LSM_HOOK_INIT(capable, selinux_capable),
+	LSM_HOOK_INIT(dma_buf_charge_task, selinux_dma_buf_charge_task),
 	LSM_HOOK_INIT(quotactl, selinux_quotactl),
 	LSM_HOOK_INIT(quota_on, selinux_quota_on),
 	LSM_HOOK_INIT(syslog, selinux_syslog),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 90cb61b16425..b4ebedc6e424 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -181,6 +181,7 @@ const struct security_class_mapping secclass_map[] = {
 	{ "user_namespace", { "create", NULL } },
 	{ "memfd_file",
 	  { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } },
+	{ "dma_buf", { "charge_task", NULL } },
 	/* last one */ { NULL, {} }
 };
 

-- 
2.55.0

Re: [PATCH RFC v2 5/8] selinux: Restrict cross-cgroup dma-buf charging

[sashiko-bot]

> The security_dma_buf_charge_task() hook allows security modules
> to control which processes may charge dma-buf allocations
> to another process's cgroup via the charge_fd field of
> DMA_HEAP_IOCTL_ALLOC. Without a policy implementation, the
> hook is a no-op and the restriction is not enforced.
> 
> On SELinux-managed systems any domain with access to a
> dma-heap device node can therefore exhaust another cgroup's
> memory budget without restriction.
> 
> Implement selinux_dma_buf_charge_task() using avc_has_perm() with
> a new dma_buf object class and a charge_task permission. Policy
> authors can then grant cross-cgroup charging selectively,
> for example:
> 
>   allow allocator_app_t client_app_t:dma_buf charge_task;
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=5

[PATCH RFC v2 6/8] selinux: Restrict dma-buf direct cgroup charging

[Albert Esteve]

The security_dma_buf_charge_cgroup() hook allows security modules
to control which processes may charge dma-buf to an arbitrary
cgroup via cgroupfd passed in charge_fd. Without a policy
implementation, the hook is a no-op and any process that can open a
cgroup directory (effectively any process) can charge memory to it.

Implement selinux_dma_buf_charge_cgroup() using avc_has_perm() with
the dma_buf object class and a charge_cgroup permission. The target
SID is derived from the SELinux label on the cgroup's kernfs
directory, so access control follow the cgroup filesystem labeling.
Policy authors can then grant cross-cgroup charging selectively,
for example:

  allow allocator_app_t cgroup_t:dma_buf charge_cgroup;

The hook matches the fail-open semantics of
selinux_kernfs_init_security(). This means unlabeled cgroups are
unrestricted by default.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 security/selinux/hooks.c            | 33 +++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |  2 +-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8f39d91eabc9..8de5e16951de 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2210,6 +2210,38 @@ static int selinux_dma_buf_charge_task(const struct cred *from, const struct cre
 			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_TASK, NULL);
 }
 
+static int selinux_dma_buf_charge_cgroup(const struct cred *cred, struct cgroup *cgrp)
+{
+	u32 cgrp_sid, clen;
+	int rc;
+	char *context;
+
+	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0);
+	if (rc == -ENODATA)
+		return 0;
+	else if (rc < 0)
+		return rc;
+
+	clen = (u32)rc;
+	context = kmalloc(clen, GFP_KERNEL);
+	if (!context)
+		return -ENOMEM;
+
+	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, context, clen);
+	if (rc < 0) {
+		kfree(context);
+		return rc;
+	}
+
+	rc = security_context_to_sid(context, clen, &cgrp_sid, GFP_KERNEL);
+	kfree(context);
+	if (rc)
+		return rc;
+
+	return avc_has_perm(cred_sid(cred), cgrp_sid,
+			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_CGROUP, NULL);
+}
+
 static int selinux_quotactl(int cmds, int type, int id, const struct super_block *sb)
 {
 	const struct cred *cred = current_cred();
@@ -7681,6 +7713,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(capset, selinux_capset),
 	LSM_HOOK_INIT(capable, selinux_capable),
 	LSM_HOOK_INIT(dma_buf_charge_task, selinux_dma_buf_charge_task),
+	LSM_HOOK_INIT(dma_buf_charge_cgroup, selinux_dma_buf_charge_cgroup),
 	LSM_HOOK_INIT(quotactl, selinux_quotactl),
 	LSM_HOOK_INIT(quota_on, selinux_quota_on),
 	LSM_HOOK_INIT(syslog, selinux_syslog),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index b4ebedc6e424..90d1e04c3d27 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -181,7 +181,7 @@ const struct security_class_mapping secclass_map[] = {
 	{ "user_namespace", { "create", NULL } },
 	{ "memfd_file",
 	  { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } },
-	{ "dma_buf", { "charge_task", NULL } },
+	{ "dma_buf", { "charge_task", "charge_cgroup", NULL } },
 	/* last one */ { NULL, {} }
 };
 

-- 
2.55.0

Re: [PATCH RFC v2 6/8] selinux: Restrict dma-buf direct cgroup charging

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] selinux: selinux_dma_buf_charge_cgroup() bypasses check for genfscon cgroups
- [Medium] selinux: stale xattr length in selinux_dma_buf_charge_cgroup() on race
- [Low] selinux: build failure in selinux_dma_buf_charge_cgroup() if !CONFIG_CGROUPS
--

commit 8979483cce99c621c0be48bae510af01c958c235
Author: Albert Esteve <aesteve@redhat.com>

selinux: Restrict dma-buf direct cgroup charging

This commit implements the selinux_dma_buf_charge_cgroup() hook and adds the
charge_cgroup permission to the dma_buf security class to control cross-cgroup
dma-buf charging. It derives the target security identifier from the
security.selinux extended attribute on the cgroup kernfs node and checks
access via avc_has_perm().

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8f39d91eabc97..8de5e16951dec 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2210,6 +2210,38 @@ static int selinux_dma_buf_charge_task(const struct cred *from, const struct cre
>  			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_TASK, NULL);
>  }
>  
> +static int selinux_dma_buf_charge_cgroup(const struct cred *cred, struct cgroup *cgrp)
> +{
> +	u32 cgrp_sid, clen;
> +	int rc;
> +	char *context;
> +
> +	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0);

[Severity: Low]
Will this fail to build when CONFIG_SECURITY_SELINUX is enabled and
CONFIG_CGROUPS is disabled?

In security/selinux/hooks.c, selinux_dma_buf_charge_cgroup() is compiled
unconditionally and dereferences cgrp->kn. However, the full definition of
struct cgroup in include/linux/cgroup-defs.h is guarded by CONFIG_CGROUPS:

include/linux/cgroup-defs.h:
#ifdef CONFIG_CGROUPS

struct cgroup;

When CONFIG_CGROUPS is disabled, include/linux/cgroup.h only forward-declares
struct cgroup as an incomplete type:

include/linux/cgroup.h:
#else /* !CONFIG_CGROUPS */

struct cgroup_subsys_state;
struct cgroup;

Since CONFIG_SECURITY_SELINUX does not depend on CONFIG_CGROUPS, dereferencing
cgrp->kn without a CONFIG_CGROUPS guard will reference an incomplete type.

> +	if (rc == -ENODATA)
> +		return 0;

[Severity: High]
Does returning 0 on -ENODATA in selinux_dma_buf_charge_cgroup() bypass the
permission check for all cgroups that use default genfscon labeling (such as
cgroup_t)?

When a process calls dma_heap_ioctl_allocate() or dma_buf_xfer_charge() ->
security_dma_buf_charge_cgroup() -> selinux_dma_buf_charge_cgroup(), this code
queries XATTR_NAME_SELINUX directly on cgrp->kn.

However, cgroup and cgroup2 filesystems are configured with SE_SBGENFS |
SE_SBGENFS_XATTR. When a cgroup inode is initialized in
security/selinux/hooks.c:inode_doinit_with_dentry(), its default label from
genfscon (for example, cgroup_t) is resolved via selinux_genfs_get_sid() and
passed as def_sid to inode_doinit_use_xattr():

security/selinux/hooks.c:inode_doinit_with_dentry() {
    ...
			rc = selinux_genfs_get_sid(dentry, sclass,
						   sbsec->flags, &sid);
			if (rc) {
				dput(dentry);
				goto out;
			}

			if ((sbsec->flags & SE_SBGENFS_XATTR) &&
			    (inode->i_opflags & IOP_XATTR)) {
				rc = inode_doinit_use_xattr(inode, dentry,
							    sid, &sid);
    ...
}

In security/selinux/hooks.c:inode_doinit_use_xattr(), when no explicit
security.selinux xattr has been set on the kernfs node, getxattr returns
-ENODATA and the inode falls back to def_sid in memory without writing
XATTR_NAME_SELINUX to cgrp->kn:

security/selinux/hooks.c:inode_doinit_use_xattr() {
    ...
	if (rc < 0) {
		kfree(context);
		if (rc != -ENODATA) {
			pr_warn("SELinux: %s:  getxattr returned %d for dev=%s ino=%llu\n",
				__func__, -rc, inode->i_sb->s_id, inode->i_ino);
			return rc;
		}
		*sid = def_sid;
		return 0;
	}
    ...
}

Similarly, selinux_kernfs_init_security() sees -ENODATA on the parent node and
returns 0 without setting XATTR_NAME_SELINUX on child cgroup nodes so that
they also fall back to genfscon.

As a result, unless a cgroup directory has been explicitly relabeled via
setxattr(), kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0) returns
-ENODATA and selinux_dma_buf_charge_cgroup() returns 0 without calling
avc_has_perm(), leaving default-labeled cgroup_t cgroups unrestricted.

> +	else if (rc < 0)
> +		return rc;
> +
> +	clen = (u32)rc;
> +	context = kmalloc(clen, GFP_KERNEL);
> +	if (!context)
> +		return -ENOMEM;
> +
> +	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, context, clen);
> +	if (rc < 0) {
> +		kfree(context);
> +		return rc;
> +	}
> +
> +	rc = security_context_to_sid(context, clen, &cgrp_sid, GFP_KERNEL);

[Severity: Medium]
Could passing clen instead of rc to security_context_to_sid() read
uninitialized heap memory if the security.selinux xattr is concurrently
shortened via setxattr()?

In selinux_dma_buf_charge_cgroup(), context is allocated with
kmalloc(clen, GFP_KERNEL) using the length from the first kernfs_xattr_get()
call. In fs/xattr.c:simple_xattr_get(), the lookup is only protected by a
local rcu read lock per call:

fs/xattr.c:simple_xattr_get() {
    ...
	guard(rcu)();
	xattr = rhashtable_lookup(ht, &key, simple_xattr_params);
	if (xattr) {
		ret = xattr->size;
		if (buffer) {
			if (size < xattr->size)
				ret = -ERANGE;
			else
				memcpy(buffer, xattr->value, xattr->size);
		}
	}
	return ret;
}

If a concurrent setxattr() replaces security.selinux on the cgroup with a
shorter, non-nul-terminated context string between the two kernfs_xattr_get()
calls, the second kernfs_xattr_get() copies only the new rc bytes into context
and returns rc < clen, leaving context[rc .. clen - 1] uninitialized.

Passing the stale clen to security_context_to_sid() then causes
security/selinux/ss/services.c:security_context_to_sid_core() to copy all clen
bytes and place the terminator at index clen:

security/selinux/ss/services.c:security_context_to_sid_core() {
    ...
	/* Copy the string to allow changes and ensure a NUL terminator */
	scontext2 = kmemdup_nul(scontext, scontext_len, gfp_flags);
	if (!scontext2)
		return -ENOMEM;
    ...
}

Should security_context_to_sid() be passed rc from the second
kernfs_xattr_get() call instead of clen?

> +	kfree(context);
> +	if (rc)
> +		return rc;
> +
> +	return avc_has_perm(cred_sid(cred), cgrp_sid,
> +			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_CGROUP, NULL);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=6

[PATCH RFC v2 7/8] dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE

[Albert Esteve]

dma-heap already supports charging a specific cgroup at allocation
time via charge_fd in DMA_HEAP_IOCTL_ALLOC. However, dma-bufs can
originate from any exporter (DRM, V4L2, etc.), and the allocator may
not know the final owner at allocation time.

Add DMA_BUF_IOCTL_XFER_CHARGE to transfer a dma-buf's memcg charge
to a different cgroup after allocation. The target is identified by
charge_fd, which may be a cgroupfd or a pidfd.

The transfer charges the new cgroup before uncharging the old one,
preserving the original charge on failure. An atomic cmpxchg on
dmabuf->memcg prevents races with concurrent transfers.

Usage example:
  int cg_fd = open("/sys/fs/cgroup/clients/app1", O_RDONLY | O_DIRECTORY);

  struct dma_buf_transfer_charge xfer = {
      .charge_fd = cg_fd,
  };
  ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
  /* charge now on app1's cgroup */
  close(cg_fd);

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 drivers/dma-buf/dma-buf.c    | 77 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/memcontrol.h   |  9 ++++++
 include/uapi/linux/dma-buf.h | 26 +++++++++++++++
 mm/memcontrol.c              | 40 +++++++++++++++++++++++
 4 files changed, 152 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f31f49ebd0af..7fa607d9bf65 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -540,6 +540,80 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf,
 }
 #endif
 
+static long dma_buf_xfer_charge(struct dma_buf *dmabuf,
+				const void __user *user_data)
+{
+	struct cgroup *cgrp;
+	struct cgroup_subsys_state *css;
+	struct dma_buf_transfer_charge arg;
+	struct mem_cgroup *memcg;
+	struct task_struct *task;
+	const struct cred *tcred;
+	unsigned int pidfd_flags;
+	int ret = 0;
+
+	if (copy_from_user(&arg, user_data, sizeof(arg)))
+		return -EFAULT;
+
+	if (!IS_ENABLED(CONFIG_MEMCG))
+		return 0;
+
+	if (arg.__padding != 0)
+		return -EINVAL;
+
+	cgrp = cgroup_get_from_fd(arg.charge_fd);
+	if (!IS_ERR(cgrp)) {
+		/* charge_fd is a cgroupfd */
+		rcu_read_lock();
+		css = cgroup_css(cgrp, &memory_cgrp_subsys);
+		if (!css || !css_tryget_online(css)) {
+			rcu_read_unlock();
+			cgroup_put(cgrp);
+			return -ENOENT;
+		}
+
+		memcg = mem_cgroup_from_css(css);
+		rcu_read_unlock();
+
+		ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
+		cgroup_put(cgrp);
+		if (ret) {
+			mem_cgroup_put(memcg);
+			return ret;
+		}
+	} else {
+		/* charge_fd is a pidfd  */
+		task = pidfd_get_task(arg.charge_fd, &pidfd_flags);
+		if (IS_ERR(task))
+			return PTR_ERR(task);
+
+		tcred = get_task_cred(task);
+		ret = security_dma_buf_charge_task(current_cred(), tcred);
+		put_cred(tcred);
+		if (ret) {
+			put_task_struct(task);
+			return ret;
+		}
+
+		memcg = get_mem_cgroup_from_mm(task->mm);
+		put_task_struct(task);
+	}
+
+	if (dmabuf->memcg == memcg)
+		goto skip_transfer;
+
+	if (!mem_cgroup_transfer_dmabuf_charge(dmabuf, memcg)) {
+		ret = -ENOMEM;
+		goto skip_transfer;
+	}
+
+	return 0;
+
+skip_transfer:
+	mem_cgroup_put(memcg);
+	return ret;
+}
+
 static long dma_buf_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long arg)
 {
@@ -590,6 +664,9 @@ static long dma_buf_ioctl(struct file *file,
 		return dma_buf_import_sync_file(dmabuf, (const void __user *)arg);
 #endif
 
+	case DMA_BUF_IOCTL_XFER_CHARGE:
+		return dma_buf_xfer_charge(dmabuf, (const void __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index aa08fbb0c26c..3a6acfda1423 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -674,6 +674,9 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, unsigned short id,
 bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
 			      gfp_t gfp_mask);
 
+bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
+				       struct mem_cgroup *new_memcg);
+
 void __mem_cgroup_uncharge(struct folio *folio);
 
 /**
@@ -1183,6 +1186,12 @@ static inline bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf,
 	return true;
 }
 
+static inline bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
+						     struct mem_cgroup *new_memcg)
+{
+	return true;
+}
+
 static inline void mem_cgroup_uncharge(struct folio *folio)
 {
 }
diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
index e827c9d20c5d..4970a7b8e08e 100644
--- a/include/uapi/linux/dma-buf.h
+++ b/include/uapi/linux/dma-buf.h
@@ -168,6 +168,31 @@ struct dma_buf_import_sync_file {
 	__s32 fd;
 };
 
+/**
+ * struct dma_buf_transfer_charge - Transfer a dma-buf's memcg charge
+ *
+ * Any holder of a dma-buf file descriptor can perform a
+ * DMA_BUF_IOCTL_XFER_CHARGE to attribute the buffer's memory
+ * cost to a specific cgroup, or move it from one cgroup to
+ * another.  The ioctl charges the new cgroup before uncharging
+ * the old one atomically.  On failure the original charge is
+ * preserved.  If the buffer has no existing charge, a new
+ * charge is established.
+ */
+struct dma_buf_transfer_charge {
+	/**
+	 * @charge_fd: File descriptor identifying the target cgroup.
+	 *
+	 * May be either a pidfd, in which case the charge moves to the
+	 * memory cgroup of the referenced process, or a cgroupfd obtained
+	 * by opening a cgroup v2 directory, in which case the charge moves
+	 * directly to that cgroup's memory controller.
+	 */
+	__u32 charge_fd;
+	/** @__padding: Reserved, must be zero */
+	__u32 __padding;
+};
+
 #define DMA_BUF_BASE		'b'
 #define DMA_BUF_IOCTL_SYNC	_IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
 
@@ -179,5 +204,6 @@ struct dma_buf_import_sync_file {
 #define DMA_BUF_SET_NAME_B	_IOW(DMA_BUF_BASE, 1, __u64)
 #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE	_IOWR(DMA_BUF_BASE, 2, struct dma_buf_export_sync_file)
 #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE	_IOW(DMA_BUF_BASE, 3, struct dma_buf_import_sync_file)
+#define DMA_BUF_IOCTL_XFER_CHARGE	_IOW(DMA_BUF_BASE, 4, struct dma_buf_transfer_charge)
 
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e94222237d31..d994a4b2d35e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5688,6 +5688,46 @@ bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
 	return true;
 }
 
+/**
+ * mem_cgroup_transfer_dmabuf_charge - move a dma-buf charge
+ * @dmabuf: buffer to transfer charge
+ * @new_memcg: target memcg; caller must hold a reference
+ *
+ * Transfers the charge for @dmabuf to @new_memcg and, on
+ * success, stores @new_memcg in @dmabuf->memcg.
+ * Returns %true on success, %false if the charge against @new_memcg
+ * failed or a concurrent transfer raced on @dmabuf->memcg.
+ * On failure the caller retains its reference to @new_memcg.
+ */
+bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
+				       struct mem_cgroup *new_memcg)
+{
+	struct mem_cgroup *old_memcg = READ_ONCE(dmabuf->memcg);
+	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
+
+	if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
+		return false;
+
+	if (cmpxchg(&dmabuf->memcg, old_memcg, new_memcg) != old_memcg) {
+		if (!mem_cgroup_is_root(new_memcg))
+			refill_stock(new_memcg, nr_pages);
+		return false;
+	}
+
+	mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages);
+
+	if (old_memcg) {
+		/* Uncharge old cgroup */
+		mod_memcg_state(old_memcg, MEMCG_DMABUF, -nr_pages);
+		if (!mem_cgroup_is_root(old_memcg))
+			refill_stock(old_memcg, nr_pages);
+		/* unref from buffer - buffer keeps new ref to new_memcg */
+		mem_cgroup_put(old_memcg);
+	}
+
+	return true;
+}
+
 /**
  * mem_cgroup_uncharge_dmabuf - uncharge a dma-buf from its memcg
  * @dmabuf: buffer to uncharge

-- 
2.55.0

Re: [PATCH RFC v2 7/8] dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE

[Christian König]

On 10/21/26 17:42, Albert Esteve wrote:
> dma-heap already supports charging a specific cgroup at allocation
> time via charge_fd in DMA_HEAP_IOCTL_ALLOC. However, dma-bufs can
> originate from any exporter (DRM, V4L2, etc.), and the allocator may
> not know the final owner at allocation time.
> 
> Add DMA_BUF_IOCTL_XFER_CHARGE to transfer a dma-buf's memcg charge
> to a different cgroup after allocation. The target is identified by
> charge_fd, which may be a cgroupfd or a pidfd.
> 
> The transfer charges the new cgroup before uncharging the old one,
> preserving the original charge on failure. An atomic cmpxchg on
> dmabuf->memcg prevents races with concurrent transfers.
> 
> Usage example:
>   int cg_fd = open("/sys/fs/cgroup/clients/app1", O_RDONLY | O_DIRECTORY);
> 
>   struct dma_buf_transfer_charge xfer = {
>       .charge_fd = cg_fd,
>   };
>   ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
>   /* charge now on app1's cgroup */
>   close(cg_fd);
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  drivers/dma-buf/dma-buf.c    | 77 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/memcontrol.h   |  9 ++++++
>  include/uapi/linux/dma-buf.h | 26 +++++++++++++++
>  mm/memcontrol.c              | 40 +++++++++++++++++++++++
>  4 files changed, 152 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f31f49ebd0af..7fa607d9bf65 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -540,6 +540,80 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf,
>  }
>  #endif
>  

A single line code comment what this is all about would be nice to have.

Some note to look at struct dma_buf_transfer_charge for details is most likely good enough.

> +static long dma_buf_xfer_charge(struct dma_buf *dmabuf,
> +				const void __user *user_data)
> +{
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
> +	struct dma_buf_transfer_charge arg;
> +	struct mem_cgroup *memcg;
> +	struct task_struct *task;
> +	const struct cred *tcred;
> +	unsigned int pidfd_flags;
> +	int ret = 0;
> +
> +	if (copy_from_user(&arg, user_data, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (!IS_ENABLED(CONFIG_MEMCG))
> +		return 0;
> +
> +	if (arg.__padding != 0)
> +		return -EINVAL;
> +
> +	cgrp = cgroup_get_from_fd(arg.charge_fd);
> +	if (!IS_ERR(cgrp)) {
> +		/* charge_fd is a cgroupfd */
> +		rcu_read_lock();
> +		css = cgroup_css(cgrp, &memory_cgrp_subsys);
> +		if (!css || !css_tryget_online(css)) {
> +			rcu_read_unlock();
> +			cgroup_put(cgrp);
> +			return -ENOENT;
> +		}
> +
> +		memcg = mem_cgroup_from_css(css);
> +		rcu_read_unlock();
> +
> +		ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
> +		cgroup_put(cgrp);
> +		if (ret) {


> +			mem_cgroup_put(memcg);
> +			return ret;

Maybe use goto skip_transfer; instead?

> +		}
> +	} else {
> +		/* charge_fd is a pidfd  */
> +		task = pidfd_get_task(arg.charge_fd, &pidfd_flags);
> +		if (IS_ERR(task))
> +			return PTR_ERR(task);
> +
> +		tcred = get_task_cred(task);
> +		ret = security_dma_buf_charge_task(current_cred(), tcred);
> +		put_cred(tcred);
> +		if (ret) {
> +			put_task_struct(task);
> +			return ret;
> +		}
> +
> +		memcg = get_mem_cgroup_from_mm(task->mm);
> +		put_task_struct(task);
> +	}
> +
> +	if (dmabuf->memcg == memcg)
> +		goto skip_transfer;
> +
> +	if (!mem_cgroup_transfer_dmabuf_charge(dmabuf, memcg)) {
> +		ret = -ENOMEM;
> +		goto skip_transfer;
> +	}
> +
> +	return 0;
> +
> +skip_transfer:
> +	mem_cgroup_put(memcg);
> +	return ret;
> +}
> +
>  static long dma_buf_ioctl(struct file *file,
>  			  unsigned int cmd, unsigned long arg)
>  {
> @@ -590,6 +664,9 @@ static long dma_buf_ioctl(struct file *file,
>  		return dma_buf_import_sync_file(dmabuf, (const void __user *)arg);
>  #endif
>  
> +	case DMA_BUF_IOCTL_XFER_CHARGE:
> +		return dma_buf_xfer_charge(dmabuf, (const void __user *)arg);
> +
>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index aa08fbb0c26c..3a6acfda1423 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -674,6 +674,9 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, unsigned short id,
>  bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
>  			      gfp_t gfp_mask);
>  
> +bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
> +				       struct mem_cgroup *new_memcg);
> +
>  void __mem_cgroup_uncharge(struct folio *folio);
>  
>  /**
> @@ -1183,6 +1186,12 @@ static inline bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf,
>  	return true;
>  }
>  
> +static inline bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
> +						     struct mem_cgroup *new_memcg)
> +{
> +	return true;
> +}
> +
>  static inline void mem_cgroup_uncharge(struct folio *folio)
>  {
>  }
> diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
> index e827c9d20c5d..4970a7b8e08e 100644
> --- a/include/uapi/linux/dma-buf.h
> +++ b/include/uapi/linux/dma-buf.h
> @@ -168,6 +168,31 @@ struct dma_buf_import_sync_file {
>  	__s32 fd;
>  };
>  
> +/**
> + * struct dma_buf_transfer_charge - Transfer a dma-buf's memcg charge
> + *
> + * Any holder of a dma-buf file descriptor can perform a
> + * DMA_BUF_IOCTL_XFER_CHARGE to attribute the buffer's memory
> + * cost to a specific cgroup, or move it from one cgroup to
> + * another.  The ioctl charges the new cgroup before uncharging
> + * the old one atomically.  On failure the original charge is
> + * preserved.  If the buffer has no existing charge, a new
> + * charge is established.
> + */
> +struct dma_buf_transfer_charge {
> +	/**
> +	 * @charge_fd: File descriptor identifying the target cgroup.
> +	 *
> +	 * May be either a pidfd, in which case the charge moves to the
> +	 * memory cgroup of the referenced process, or a cgroupfd obtained
> +	 * by opening a cgroup v2 directory, in which case the charge moves
> +	 * directly to that cgroup's memory controller.
> +	 */
> +	__u32 charge_fd;
> +	/** @__padding: Reserved, must be zero */
> +	__u32 __padding;

Maybe rename to flags, put before charge_fd and add "/* unused for now */".

> +};
> +
>  #define DMA_BUF_BASE		'b'
>  #define DMA_BUF_IOCTL_SYNC	_IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
>  
> @@ -179,5 +204,6 @@ struct dma_buf_import_sync_file {
>  #define DMA_BUF_SET_NAME_B	_IOW(DMA_BUF_BASE, 1, __u64)
>  #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE	_IOWR(DMA_BUF_BASE, 2, struct dma_buf_export_sync_file)
>  #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE	_IOW(DMA_BUF_BASE, 3, struct dma_buf_import_sync_file)
> +#define DMA_BUF_IOCTL_XFER_CHARGE	_IOW(DMA_BUF_BASE, 4, struct dma_buf_transfer_charge)
>  
>  #endif
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e94222237d31..d994a4b2d35e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5688,6 +5688,46 @@ bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
>  	return true;
>  }
>  
> +/**
> + * mem_cgroup_transfer_dmabuf_charge - move a dma-buf charge
> + * @dmabuf: buffer to transfer charge
> + * @new_memcg: target memcg; caller must hold a reference
> + *
> + * Transfers the charge for @dmabuf to @new_memcg and, on
> + * success, stores @new_memcg in @dmabuf->memcg.
> + * Returns %true on success, %false if the charge against @new_memcg
> + * failed or a concurrent transfer raced on @dmabuf->memcg.
> + * On failure the caller retains its reference to @new_memcg.
> + */
> +bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
> +				       struct mem_cgroup *new_memcg)
> +{
> +	struct mem_cgroup *old_memcg = READ_ONCE(dmabuf->memcg);
> +	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;

That doesn't work as easily as this.

DMA-buf can contain litterally *anything*, ranging from system memory over device memory all the way to functional HW blocks. In other words doorbell, GWS, GDS, OA, I2C, I2S, any random data consuming or providng HW block...

For memory the "size" is the actually number of bytes the buffer represents, but for everything else it is just an arbitary number given by the exporter. In most cases just 1 or actually number of HW blocks/channels, but for GDS for example it is actually a pretty large number.

Saying that you most likely need some flag from the exporter if charging and/or the changing of charging should be possible or not. Otherwise you can potentially do quite a bunch of nonsense with that.

Regards,
Christian.

> +
> +	if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
> +		return false;
> +
> +	if (cmpxchg(&dmabuf->memcg, old_memcg, new_memcg) != old_memcg) {
> +		if (!mem_cgroup_is_root(new_memcg))
> +			refill_stock(new_memcg, nr_pages);
> +		return false;
> +	}
> +
> +	mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages);
> +
> +	if (old_memcg) {
> +		/* Uncharge old cgroup */
> +		mod_memcg_state(old_memcg, MEMCG_DMABUF, -nr_pages);
> +		if (!mem_cgroup_is_root(old_memcg))
> +			refill_stock(old_memcg, nr_pages);
> +		/* unref from buffer - buffer keeps new ref to new_memcg */
> +		mem_cgroup_put(old_memcg);
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * mem_cgroup_uncharge_dmabuf - uncharge a dma-buf from its memcg
>   * @dmabuf: buffer to uncharge
> 

Re: [PATCH RFC v2 7/8] dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] dma-buf: task->mm UAF and wrong cgroup fallback in dma_buf_xfer_charge()
- [Critical] mm: memcontrol: new_memcg UAF race in mem_cgroup_transfer_dmabuf_charge()
- [High] dma-buf: missing DAC permission checks in dma_buf_xfer_charge()
- [Medium] dma-buf: double charge and -ENOMEM race in mem_cgroup_transfer_dmabuf_charge()
- [Medium] dma-buf: broken runtime mem_cgroup_disabled() handling in dma_buf_xfer_charge()
- [Low] dma-buf: build failure in dma_buf_xfer_charge() when CONFIG_MEMCG=n
--

commit c7c3a18bea48fc8f09ccf4a84a6aa11e6f991c4c
Author: Albert Esteve <aesteve@redhat.com>

dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE

This commit adds the DMA_BUF_IOCTL_XFER_CHARGE ioctl and
mem_cgroup_transfer_dmabuf_charge() helper to transfer a dma-buf's memory
cgroup charge to a target cgroup identified by a cgroupfd or pidfd.

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f31f49ebd0af3..7fa607d9bf657 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -540,6 +540,80 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf,
>  }
>  #endif
>  
> +static long dma_buf_xfer_charge(struct dma_buf *dmabuf,
> +				const void __user *user_data)
> +{
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
> +	struct dma_buf_transfer_charge arg;
> +	struct mem_cgroup *memcg;
> +	struct task_struct *task;
> +	const struct cred *tcred;
> +	unsigned int pidfd_flags;
> +	int ret = 0;
> +
> +	if (copy_from_user(&arg, user_data, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (!IS_ENABLED(CONFIG_MEMCG))
> +		return 0;

[Severity: Low]
Will drivers/dma-buf/dma-buf.c fail to build in dma_buf_xfer_charge() when
CONFIG_DMA_SHARED_BUFFER=y and CONFIG_MEMCG=n (or CONFIG_CGROUPS=n)?

Because if (!IS_ENABLED(CONFIG_MEMCG)) is a runtime C conditional rather
than a preprocessor guard, the compiler still resolves all symbols in the
rest of dma_buf_xfer_charge().

In include/linux/cgroup_subsys.h, memory_cgrp_subsys is only declared when
CONFIG_MEMCG is enabled:

    #if IS_ENABLED(CONFIG_MEMCG)
    SUBSYS(memory)
    #endif

And when CONFIG_CGROUPS=n, cgroup_get_from_fd(), cgroup_css(),
css_tryget_online(), and cgroup_put() in include/linux/cgroup.h have no
stub declarations. Should the body of dma_buf_xfer_charge() be guarded with
#if IS_ENABLED(CONFIG_MEMCG) instead?

> +
> +	if (arg.__padding != 0)
> +		return -EINVAL;
> +
> +	cgrp = cgroup_get_from_fd(arg.charge_fd);
> +	if (!IS_ERR(cgrp)) {
> +		/* charge_fd is a cgroupfd */
> +		rcu_read_lock();
> +		css = cgroup_css(cgrp, &memory_cgrp_subsys);
> +		if (!css || !css_tryget_online(css)) {
> +			rcu_read_unlock();
> +			cgroup_put(cgrp);
> +			return -ENOENT;
> +		}

[Severity: Medium]
What happens in dma_buf_xfer_charge() when CONFIG_MEMCG=y but memory cgroups
are disabled at boot via cgroup_disable=memory (so mem_cgroup_disabled() is
true)?

In that configuration, passing a valid cgroupfd causes
cgroup_css(cgrp, &memory_cgrp_subsys) to return NULL and fail with -ENOENT,
whereas passing a pidfd calls get_mem_cgroup_from_mm() in mm/memcontrol.c,
which returns NULL when mem_cgroup_disabled() is true, matches
dmabuf->memcg == NULL, and returns 0.

Similarly, in mm/memcontrol.c, mem_cgroup_transfer_dmabuf_charge() only
skips try_charge() when mem_cgroup_disabled() is true:

    if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
            return false;

Unlike mem_cgroup_charge_dmabuf(), which returns true immediately when
mem_cgroup_disabled() is true, mem_cgroup_transfer_dmabuf_charge() still
proceeds to cmpxchg(), mod_memcg_state(), and refill_stock(). Should both
dma_buf_xfer_charge() and mem_cgroup_transfer_dmabuf_charge() return early
when mem_cgroup_disabled() is true?

> +
> +		memcg = mem_cgroup_from_css(css);
> +		rcu_read_unlock();
> +
> +		ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
> +		cgroup_put(cgrp);
> +		if (ret) {
> +			mem_cgroup_put(memcg);
> +			return ret;
> +		}
> +	} else {
> +		/* charge_fd is a pidfd  */
> +		task = pidfd_get_task(arg.charge_fd, &pidfd_flags);
> +		if (IS_ERR(task))
> +			return PTR_ERR(task);
> +
> +		tcred = get_task_cred(task);
> +		ret = security_dma_buf_charge_task(current_cred(), tcred);
> +		put_cred(tcred);
> +		if (ret) {
> +			put_task_struct(task);
> +			return ret;
> +		}

[Severity: High]
Is a DAC or cgroup permission check needed in dma_buf_xfer_charge() in
addition to the security_dma_buf_charge_cgroup() and
security_dma_buf_charge_task() LSM hooks?

In drivers/dma-buf/dma-buf.c, dma_buf_xfer_charge() relies solely on those
LSM hooks to authorize the transfer:

- In the cgroupfd path, cgroup_get_from_fd(arg.charge_fd) only checks that
  the file descriptor refers to a cgroup v2 directory, without verifying
  write access (such as inode_permission() with MAY_WRITE or
  cgroup_procs_write_permission()). Because /sys/fs/cgroup directories are
  world-readable (0755) by default, an unprivileged process can open
  /sys/fs/cgroup or another service's cgroup with O_RDONLY | O_DIRECTORY.
- In the pidfd path, pidfd_get_task() resolves any pidfd without checking
  whether current may act on that task (for example via ptrace_may_access()
  or credential/cgroup authorization).

When CONFIG_SECURITY=n, when no LSM implementing these hooks is active, when
a cgroup directory has no explicit security.selinux xattr, or when two
cgroups share the same SELinux domain, the hooks return 0. Could an
unprivileged holder of a dma-buf fd use DMA_BUF_IOCTL_XFER_CHARGE to shift
its charges to the root cgroup (where try_charge() in
mem_cgroup_transfer_dmabuf_charge() is a no-op, bypassing its own
memory.max), or transfer large charges into another cgroup to exhaust its
memory.max and trigger mem_cgroup_oom()?

> +
> +		memcg = get_mem_cgroup_from_mm(task->mm);
> +		put_task_struct(task);
> +	}

[Severity: Critical]
Can passing task->mm directly to get_mem_cgroup_from_mm(task->mm) in
dma_buf_xfer_charge() cause a use-after-free or charge the wrong cgroup?

In drivers/dma-buf/dma-buf.c, pidfd_get_task() pins task->usage, but it
does not pin task->mm or hold task_lock(task). If the target task
concurrently exits or execs:

    do_exit()
      exit_mm()
        task_lock(current);
        current->mm = NULL;
        ...
        task_unlock(current);
        ...
        mmput(mm); -> __mmdrop(mm) -> free_mm(mm)

or replaces its mm via execve() -> exec_mmap() -> exec_mm_put_old(), the
old mm_struct can be freed while get_mem_cgroup_from_mm() in mm/memcontrol.c
dereferences mm->owner:

    rcu_read_lock();
    do {
            memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));

In addition, if task->mm is already NULL (such as a zombie task that has
completed exit_mm(), for which pidfd_get_task() still succeeds),
get_mem_cgroup_from_mm(NULL) in mm/memcontrol.c falls back to the caller's
context:

    if (unlikely(!mm)) {
            memcg = active_memcg();
            if (unlikely(memcg)) {
                    /* remote memcg must hold a ref */
                    css_get(&memcg->css);
                    return memcg;
            }
            mm = current->mm;
            if (unlikely(!mm))
                    return root_mem_cgroup;
    }

Instead of failing when the target task has no mm, this silently attributes
the dma-buf charge to the caller's own cgroup (or root_mem_cgroup). Should
dma_buf_xfer_charge() acquire a reference via get_task_mm(task), return an
error such as -ESRCH when mm is NULL, and call mmput(mm) after
get_mem_cgroup_from_mm(mm)?

> +
> +	if (dmabuf->memcg == memcg)
> +		goto skip_transfer;
> +
> +	if (!mem_cgroup_transfer_dmabuf_charge(dmabuf, memcg)) {
> +		ret = -ENOMEM;
> +		goto skip_transfer;
> +	}

[ ... ]

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e94222237d31a..d994a4b2d35e4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5688,6 +5688,46 @@ bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
>  	return true;
>  }
>  
> +/**
> + * mem_cgroup_transfer_dmabuf_charge - move a dma-buf charge
> + * @dmabuf: buffer to transfer charge
> + * @new_memcg: target memcg; caller must hold a reference
> + *
> + * Transfers the charge for @dmabuf to @new_memcg and, on
> + * success, stores @new_memcg in @dmabuf->memcg.
> + * Returns %true on success, %false if the charge against @new_memcg
> + * failed or a concurrent transfer raced on @dmabuf->memcg.
> + * On failure the caller retains its reference to @new_memcg.
> + */
> +bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
> +				       struct mem_cgroup *new_memcg)
> +{
> +	struct mem_cgroup *old_memcg = READ_ONCE(dmabuf->memcg);
> +	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
> +
> +	if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
> +		return false;
> +
> +	if (cmpxchg(&dmabuf->memcg, old_memcg, new_memcg) != old_memcg) {
> +		if (!mem_cgroup_is_root(new_memcg))
> +			refill_stock(new_memcg, nr_pages);
> +		return false;
> +	}

[Severity: Medium]
How does mem_cgroup_transfer_dmabuf_charge() behave when two threads race
to transfer the same dmabuf?

First, dma_buf_xfer_charge() in drivers/dma-buf/dma-buf.c checks
dmabuf->memcg == memcg before calling mem_cgroup_transfer_dmabuf_charge(),
which then reloads old_memcg = READ_ONCE(dmabuf->memcg) without rechecking
old_memcg == new_memcg. If two threads concurrently transfer dmabuf to the
same target cgroup, both can pass the check in dma_buf_xfer_charge(); after
the first transfer finishes, the second thread reads
old_memcg == new_memcg and calls try_charge(new_memcg, GFP_KERNEL, nr_pages)
before uncharging old_memcg, transiently charging 2 * nr_pages to new_memcg
and potentially triggering unnecessary reclaim, OOM, or -ENOMEM even though
dmabuf is already charged to new_memcg.

Second, old_memcg is read before try_charge(new_memcg, GFP_KERNEL, nr_pages),
which can sleep in direct reclaim. If another transfer updates dmabuf->memcg
while try_charge() is sleeping, cmpxchg(&dmabuf->memcg, old_memcg, new_memcg)
fails without retrying, and dma_buf_xfer_charge() returns -ENOMEM to
userspace even though charging new_memcg succeeded. Should the cmpxchg loop
retry with the updated old_memcg (and treat old_memcg == new_memcg as a
no-op)?

> +
> +	mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages);
> +
> +	if (old_memcg) {
> +		/* Uncharge old cgroup */
> +		mod_memcg_state(old_memcg, MEMCG_DMABUF, -nr_pages);
> +		if (!mem_cgroup_is_root(old_memcg))
> +			refill_stock(old_memcg, nr_pages);
> +		/* unref from buffer - buffer keeps new ref to new_memcg */
> +		mem_cgroup_put(old_memcg);
> +	}

[Severity: Critical]
Could publishing new_memcg into dmabuf->memcg via cmpxchg() before calling
mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages) lead to a use-after-free
on new_memcg when two DMA_BUF_IOCTL_XFER_CHARGE calls race?

In mm/memcontrol.c, dma_buf_xfer_charge() holds a single reference to
new_memcg that is transferred to dmabuf->memcg as soon as cmpxchg()
succeeds:

Thread A                              Thread B
mem_cgroup_transfer_dmabuf_charge()
  cmpxchg(&dmabuf->memcg,
          old_memcg, new_memcg)
                                      mem_cgroup_transfer_dmabuf_charge()
                                        old_memcg = READ_ONCE(dmabuf->memcg);
                                        /* old_memcg is new_memcg */
                                        cmpxchg(&dmabuf->memcg,
                                                new_memcg, memcg_C)
                                        mod_memcg_state(new_memcg,
                                                        MEMCG_DMABUF,
                                                        -nr_pages);
                                        refill_stock(new_memcg, nr_pages);
                                        mem_cgroup_put(new_memcg);
  mod_memcg_state(new_memcg,
                  MEMCG_DMABUF,
                  nr_pages);

If new_memcg was concurrently offlined after cgroup_put(cgrp) in
dma_buf_xfer_charge(), and refill_stock() uncharges directly (for example
when nr_pages > MEMCG_CHARGE_BATCH), Thread B's mem_cgroup_put(new_memcg)
can drop the last reference and free new_memcg before Thread A executes
mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages). Even when new_memcg is
not freed, Thread B decrements MEMCG_DMABUF before Thread A increments it.

Would updating mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages) before the
cmpxchg() (and reverting it if cmpxchg() fails), or serializing transfers,
avoid exposing new_memcg before its state update completes?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=7

[PATCH RFC v2 8/8] selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

[Albert Esteve]

Add tests for the new charge_fd field in struct
dma_heap_allocation_data.

When the charge_fd feature is absent (unpatched kernel),
the probe in pidfd_alloc_supported() detects this and the
tests are skipped gracefully.

Add vmtest.sh similar to other subsystem suites, to orchestrate
building the selftests (optionally with a freshly compiled kernel)
inside a virtme-ng VM, so the tests can be run without modifying
the host system. Add a config fragment with required Kconfig symbols.

Also add test_memcg_dmabuf() to the existing test_memcontrol suite
to verify end-to-end cross-cgroup accounting: a parent process opens
a pidfd for a child in a separate cgroup, allocates a dma-buf via
DMA_HEAP_IOCTL_ALLOC with that pidfd, and asserts that memory.stat
dmabuf in the child's cgroup reflects the allocation. If the dmabuf
key is missing (unpatched kernel) or /dev/dma_heap/system is absent,
the test is skipped.

Assisted-by: LLM
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 tools/testing/selftests/cgroup/Makefile            |   2 +-
 tools/testing/selftests/cgroup/test_memcontrol.c   | 145 +++++++++++++-
 tools/testing/selftests/dmabuf-heaps/config        |   1 +
 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 222 ++++++++++++++++++++-
 tools/testing/selftests/dmabuf-heaps/vmtest.sh     | 205 +++++++++++++++++++
 5 files changed, 571 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
index e01584c2189a..9edfc9f1de5c 100644
--- a/tools/testing/selftests/cgroup/Makefile
+++ b/tools/testing/selftests/cgroup/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-CFLAGS += -Wall -pthread
+CFLAGS += -Wall -pthread $(KHDR_INCLUDES)
 
 all: ${HELPER_PROGS}
 
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 0ed82347044e..51eb60e7cdd4 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -19,9 +19,17 @@
 #include <errno.h>
 #include <sys/mman.h>
 
+#include <linux/dma-heap.h>
+#include <signal.h>
+#include <sys/ioctl.h>
+
+#include "../pidfd/pidfd.h"
 #include "kselftest.h"
 #include "cgroup_util.h"
 
+#define DMA_HEAP_SYSTEM		"/dev/dma_heap/system"
+#define ONE_MEG			(1024 * 1024)
+
 #define MEMCG_SOCKSTAT_WAIT_RETRIES        30
 
 static bool has_localevents;
@@ -1764,6 +1772,125 @@ static int test_memcg_inotify_delete_dir(const char *root)
 	return ret;
 }
 
+static int memcg_dmabuf_child(const char *cgroup, void *arg)
+{
+	pause();
+	return 0;
+}
+
+/*
+ * This test allocates a dma-buf via DMA_HEAP_IOCTL_ALLOC with a pidfd
+ * pointing to a child process in a separate cgroup, then checks that
+ * memory.stat[dmabuf] in the child's cgroup rises by the allocation size
+ * and returns to zero after the buffer fd is closed.
+ */
+static int test_memcg_dmabuf(const char *root)
+{
+	char *parent = NULL, *child_cg = NULL;
+	int ret = KSFT_FAIL;
+	int heap_fd = -1, dmabuf_fd = -1, pidfd = -1;
+	pid_t child_pid;
+	int child_status;
+	long dmabuf_stat;
+	struct dma_heap_allocation_data alloc = {
+		.len      = ONE_MEG,
+		.fd_flags = O_RDWR | O_CLOEXEC,
+	};
+
+	if (access(DMA_HEAP_SYSTEM, R_OK | W_OK)) {
+		ret = KSFT_SKIP;
+		goto cleanup;
+	}
+
+	parent = cg_name(root, "dmabuf_memcg_test");
+	if (!parent)
+		goto cleanup;
+
+	if (cg_create(parent))
+		goto cleanup_parent;
+
+	if (cg_write(parent, "cgroup.subtree_control", "+memory"))
+		goto cleanup_parent;
+
+	child_cg = cg_name(parent, "child");
+	if (!child_cg)
+		goto cleanup_parent;
+
+	if (cg_create(child_cg))
+		goto cleanup_parent;
+
+	child_pid = cg_run_nowait(child_cg, memcg_dmabuf_child, NULL);
+	if (child_pid < 0)
+		goto cleanup_child;
+
+	if (cg_wait_for_proc_count(child_cg, 1))
+		goto cleanup_kill;
+
+	pidfd = sys_pidfd_open(child_pid, 0);
+	if (pidfd < 0) {
+		ret = KSFT_SKIP;
+		goto cleanup_kill;
+	}
+
+	heap_fd = open(DMA_HEAP_SYSTEM, O_RDWR);
+	if (heap_fd < 0) {
+		ret = KSFT_SKIP;
+		goto cleanup_pidfd;
+	}
+
+	alloc.charge_fd = (__u32)pidfd;
+	if (ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &alloc) < 0)
+		goto cleanup_heap;
+	dmabuf_fd = (int)alloc.fd;
+
+	dmabuf_stat = cg_read_key_long(child_cg, "memory.stat", "dmabuf ");
+	if (dmabuf_stat == -1) {
+		ret = KSFT_SKIP;
+		goto cleanup_dmabuf;
+	}
+	if (dmabuf_stat != ONE_MEG)
+		dmabuf_stat = cg_read_key_long_poll(child_cg, "memory.stat",
+						    "dmabuf ", ONE_MEG,
+						    15, 200000);
+	if (dmabuf_stat != ONE_MEG) {
+		fprintf(stderr, "Expected dmabuf stat %d, got %ld\n",
+			ONE_MEG, dmabuf_stat);
+		goto cleanup_dmabuf;
+	}
+
+	close(dmabuf_fd);
+	dmabuf_fd = -1;
+
+	dmabuf_stat = cg_read_key_long_poll(child_cg, "memory.stat",
+					    "dmabuf ", 0, 15, 200000);
+	if (dmabuf_stat != 0) {
+		fprintf(stderr, "Expected dmabuf stat 0 after close, got %ld\n",
+			dmabuf_stat);
+		goto cleanup_heap;
+	}
+
+	ret = KSFT_PASS;
+
+cleanup_dmabuf:
+	if (dmabuf_fd >= 0)
+		close(dmabuf_fd);
+cleanup_heap:
+	close(heap_fd);
+cleanup_pidfd:
+	close(pidfd);
+cleanup_kill:
+	kill(child_pid, SIGTERM);
+	waitpid(child_pid, &child_status, 0);
+cleanup_child:
+	cg_destroy(child_cg);
+	free(child_cg);
+cleanup_parent:
+	cg_destroy(parent);
+	free(parent);
+cleanup:
+	return ret;
+}
+
 #define T(x) { x, #x }
 struct memcg_test {
 	int (*fn)(const char *root);
@@ -1785,13 +1912,18 @@ struct memcg_test {
 	T(test_memcg_oom_group_score_events),
 	T(test_memcg_inotify_delete_file),
 	T(test_memcg_inotify_delete_dir),
+	T(test_memcg_dmabuf),
 };
 #undef T
 
 int main(int argc, char **argv)
 {
 	char root[PATH_MAX];
-	int i, proc_status;
+	int i, proc_status, count = ARRAY_SIZE(tests);
+	const char *filter = NULL;
+
+	if (argc > 1)
+		filter = argv[1];
 
 	page_size = sysconf(_SC_PAGE_SIZE);
 	if (page_size <= 0)
@@ -1822,8 +1954,17 @@ int main(int argc, char **argv)
 		ksft_exit_skip("Failed to query cgroup mount option\n");
 	has_localevents = proc_status;
 
-	ksft_set_plan(ARRAY_SIZE(tests));
+	if (filter) {
+		count = 0;
+		for (i = 0; i < ARRAY_SIZE(tests); i++) {
+			if (!strcmp(tests[i].name, filter))
+				count++;
+		}
+	}
+	ksft_set_plan(count);
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (filter && strcmp(tests[i].name, filter))
+			continue;
 		switch (tests[i].fn(root)) {
 		case KSFT_PASS:
 			ksft_test_result_pass("%s\n", tests[i].name);
diff --git a/tools/testing/selftests/dmabuf-heaps/config b/tools/testing/selftests/dmabuf-heaps/config
index be091f1cdfa0..94c8f33b71a2 100644
--- a/tools/testing/selftests/dmabuf-heaps/config
+++ b/tools/testing/selftests/dmabuf-heaps/config
@@ -1,3 +1,4 @@
+CONFIG_MEMCG=y
 CONFIG_DMABUF_HEAPS=y
 CONFIG_DMABUF_HEAPS_SYSTEM=y
 CONFIG_DRM_VGEM=y
diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
index 1d49df671919..b1c58e5883f6 100644
--- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
+++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
@@ -3,6 +3,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -10,11 +11,15 @@
 #include <unistd.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <sys/syscall.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 
 #include <linux/dma-buf.h>
 #include <linux/dma-heap.h>
+#include <linux/limits.h>
 #include <drm/drm.h>
+#include "../pidfd/pidfd.h"
 #include "kselftest.h"
 
 #define DEVPATH "/dev/dma_heap"
@@ -320,6 +325,8 @@ static int dmabuf_heap_alloc_newer(int fd, size_t len, unsigned int flags,
 		__u32 fd;
 		__u32 fd_flags;
 		__u64 heap_flags;
+		__u32 charge_fd;
+		__u32 __padding;
 		__u64 garbage1;
 		__u64 garbage2;
 		__u64 garbage3;
@@ -328,6 +335,8 @@ static int dmabuf_heap_alloc_newer(int fd, size_t len, unsigned int flags,
 		.fd = 0,
 		.fd_flags = O_RDWR | O_CLOEXEC,
 		.heap_flags = flags,
+		.charge_fd = 0,
+		.__padding = 0,
 		.garbage1 = 0xffffffff,
 		.garbage2 = 0x88888888,
 		.garbage3 = 0x11111111,
@@ -496,6 +505,213 @@ static void test_alloc_no_fd_leak_on_efault(char *heap_name)
 	ksft_test_result(fd_before == fd_after,
 			 "fd leak on EFAULT: before=%d after=%d\n",
 			 fd_before, fd_after);
+
+out:
+	close(heap_fd);
+}
+
+static int dmabuf_heap_alloc_pidfd(int fd, size_t len, unsigned int heap_flags,
+				   unsigned int charge_fd, int *dmabuf_fd)
+{
+	struct dma_heap_allocation_data data = {
+		.len = len,
+		.fd = 0,
+		.fd_flags = O_RDWR | O_CLOEXEC,
+		.heap_flags = heap_flags,
+		.charge_fd = charge_fd,
+	};
+	int ret;
+
+	if (!dmabuf_fd)
+		return -EINVAL;
+
+	ret = ioctl(fd, DMA_HEAP_IOCTL_ALLOC, &data);
+	if (ret < 0)
+		return ret;
+	*dmabuf_fd = (int)data.fd;
+	return ret;
+}
+
+/*
+ * Probe whether the kernel honours charge_fd in DMA_HEAP_IOCTL_ALLOC.
+ */
+static bool pidfd_alloc_supported(int heap_fd)
+{
+	int devnull_fd, dmabuf_fd = -1, ret;
+
+	devnull_fd = open("/dev/null", O_RDONLY);
+	if (devnull_fd < 0)
+		return false;
+
+	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, devnull_fd, &dmabuf_fd);
+	if (dmabuf_fd >= 0) {
+		close(dmabuf_fd);
+		dmabuf_fd = -1;
+	}
+	close(devnull_fd);
+	return ret < 0;
+}
+
+/*
+ * Test: allocate charging the calling process's own cgroup via a self pidfd.
+ */
+static void test_alloc_pidfd_self(char *heap_name)
+{
+	int heap_fd = -1, pidfd = -1, dmabuf_fd = -1, ret;
+
+	heap_fd = dmabuf_heap_open(heap_name);
+
+	if (!pidfd_alloc_supported(heap_fd)) {
+		ksft_test_result_skip("charge_fd not supported by this kernel\n");
+		goto out;
+	}
+
+	pidfd = sys_pidfd_open(getpid(), 0);
+	if (pidfd < 0) {
+		ksft_test_result_skip("pidfd_open not available\n");
+		goto out;
+	}
+
+	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, pidfd, &dmabuf_fd);
+	ksft_test_result(!ret, "Allocation with self pidfd %d\n", ret);
+	if (dmabuf_fd >= 0)
+		close(dmabuf_fd);
+	close(pidfd);
+out:
+	close(heap_fd);
+}
+
+/*
+ * Test: allocate charging a child process's cgroup via a child pidfd.
+ */
+static void test_alloc_pidfd_child(char *heap_name)
+{
+	int heap_fd = -1, pidfd = -1, dmabuf_fd = -1;
+	pid_t child_pid;
+	int status, ret;
+
+	heap_fd = dmabuf_heap_open(heap_name);
+
+	if (!pidfd_alloc_supported(heap_fd)) {
+		ksft_test_result_skip("charge_fd not supported by this kernel\n");
+		goto out;
+	}
+
+	child_pid = fork();
+	if (child_pid == 0) {
+		pause();
+		_exit(0);
+	}
+	if (child_pid < 0)
+		ksft_exit_fail_msg("fork failed: %s\n", strerror(errno));
+
+	pidfd = sys_pidfd_open(child_pid, 0);
+	if (pidfd < 0) {
+		kill(child_pid, SIGTERM);
+		waitpid(child_pid, &status, 0);
+		ksft_test_result_skip("pidfd_open for child failed\n");
+		goto out;
+	}
+
+	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, pidfd, &dmabuf_fd);
+	ksft_test_result(!ret, "Allocation with child pidfd %d\n", ret);
+	if (dmabuf_fd >= 0)
+		close(dmabuf_fd);
+	close(pidfd);
+	kill(child_pid, SIGTERM);
+	waitpid(child_pid, &status, 0);
+out:
+	close(heap_fd);
+}
+
+static int open_self_cgroupfd(void)
+{
+	char cgroup_path[PATH_MAX];
+	char buf[PATH_MAX];
+	FILE *f;
+
+	f = fopen("/proc/self/cgroup", "r");
+	if (!f)
+		return -1;
+
+	cgroup_path[0] = '\0';
+	while (fgets(buf, sizeof(buf), f)) {
+		char *p = buf;
+
+		if (strncmp(p, "0::", 3) == 0) {
+			p += 3;
+			p[strcspn(p, "\n")] = '\0';
+			snprintf(cgroup_path, sizeof(cgroup_path),
+				 "/sys/fs/cgroup%s", p);
+			break;
+		}
+	}
+	fclose(f);
+
+	if (!cgroup_path[0])
+		return -1;
+
+	return open(cgroup_path, O_RDONLY | O_DIRECTORY);
+}
+
+/*
+ * Test: allocate charging the calling process's own cgroup via a cgroupfd.
+ */
+static void test_alloc_cgroupfd_self(char *heap_name)
+{
+	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
+
+	heap_fd = dmabuf_heap_open(heap_name);
+
+	if (!pidfd_alloc_supported(heap_fd)) {
+		ksft_test_result_skip("charge_fd not supported by this kernel\n");
+		goto out;
+	}
+
+	cgroup_fd = open_self_cgroupfd();
+	if (cgroup_fd < 0) {
+		ksft_test_result_skip("cannot open self cgroupfd\n");
+		goto out;
+	}
+
+	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, cgroup_fd, &dmabuf_fd);
+	ksft_test_result(!ret, "Allocation with cgroupfd %d\n", ret);
+	if (dmabuf_fd >= 0)
+		close(dmabuf_fd);
+	close(cgroup_fd);
+out:
+	close(heap_fd);
+}
+
+/*
+ * Test: transfer a dma-buf's charge to self cgroup via DMA_BUF_IOCTL_XFER_CHARGE.
+ */
+static void test_xfer_charge_cgroupfd_self(char *heap_name)
+{
+	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
+	struct dma_buf_transfer_charge xfer = { 0 };
+
+	heap_fd = dmabuf_heap_open(heap_name);
+
+	ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd);
+	if (ret) {
+		ksft_test_result_fail("Allocation failed %d\n", ret);
+		goto out;
+	}
+
+	cgroup_fd = open_self_cgroupfd();
+	if (cgroup_fd < 0) {
+		ksft_test_result_skip("cannot open self cgroupfd\n");
+		close(dmabuf_fd);
+		goto out;
+	}
+
+	xfer.charge_fd = cgroup_fd;
+	ret = ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
+	ksft_test_result(!ret, "XFER_CHARGE to self cgroupfd %d\n", ret);
+
+	close(cgroup_fd);
+	close(dmabuf_fd);
 out:
 	close(heap_fd);
 }
@@ -530,7 +746,7 @@ int main(void)
 		return KSFT_SKIP;
 	}
 
-	ksft_set_plan(12 * numer_of_heaps());
+	ksft_set_plan(16 * numer_of_heaps());
 
 	while ((dir = readdir(d))) {
 		if (!strncmp(dir->d_name, ".", 2))
@@ -546,6 +762,10 @@ int main(void)
 		test_alloc_compat(dir->d_name);
 		test_alloc_errors(dir->d_name);
 		test_alloc_no_fd_leak_on_efault(dir->d_name);
+		test_alloc_pidfd_self(dir->d_name);
+		test_alloc_pidfd_child(dir->d_name);
+		test_alloc_cgroupfd_self(dir->d_name);
+		test_xfer_charge_cgroupfd_self(dir->d_name);
 	}
 	closedir(d);
 
diff --git a/tools/testing/selftests/dmabuf-heaps/vmtest.sh b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
new file mode 100755
index 000000000000..434857b765b8
--- /dev/null
+++ b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
@@ -0,0 +1,205 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2026 Red Hat
+#
+# Dependencies:
+#		* virtme-ng
+#		* qemu	(used by virtme-ng)
+
+readonly SCRIPT_DIR="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
+readonly KERNEL_CHECKOUT=$(realpath "${SCRIPT_DIR}"/../../../../)
+readonly CGROUP_DIR="${KERNEL_CHECKOUT}/tools/testing/selftests/cgroup"
+
+source "${SCRIPT_DIR}"/../kselftest/ktap_helpers.sh
+
+readonly DMABUF_HEAP_TEST="${SCRIPT_DIR}"/dmabuf-heap
+readonly MEMCONTROL_TEST="${CGROUP_DIR}"/test_memcontrol
+readonly TMP_DIR=$(mktemp -d /tmp/dmabuf-vmtest.XXXXXXXX)
+
+VERBOSE=false
+BUILD=false
+BUILD_HOST=""
+BUILD_HOST_PODMAN_CONTAINER_NAME=""
+
+usage() {
+	echo
+	echo "$0 [OPTIONS]"
+	echo
+	echo "Options"
+	echo "  -b: build the kernel from the current source tree and use it for the VM"
+	echo "  -H: hostname for remote build host (used with -b)"
+	echo "  -p: podman container name for remote build host (used with -b)"
+	echo "      Example: -H beefyserver -p vng"
+
+	echo "  -v: enable verbose vng/qemu output"
+	echo
+
+	exit 1
+}
+
+die() {
+	echo "$*" >&2
+	exit "${KSFT_FAIL}"
+}
+
+cleanup() {
+	rm -rf "${TMP_DIR}"
+}
+
+check_deps() {
+	for dep in vng make; do
+		if [[ ! -x $(command -v "${dep}") ]]; then
+			echo -e "skip:    dependency ${dep} not found!\n"
+			exit "${KSFT_SKIP}"
+		fi
+	done
+
+	if [[ ! -x "${DMABUF_HEAP_TEST}" ]]; then
+		printf "skip:    %s not found!" "${DMABUF_HEAP_TEST}"
+		printf " Please build the kselftest dmabuf-heaps target (or use -b).\n"
+		exit "${KSFT_SKIP}"
+	fi
+
+	if [[ ! -x "${MEMCONTROL_TEST}" ]]; then
+		printf "skip:    %s not found!" "${MEMCONTROL_TEST}"
+		printf " Please build the kselftest cgroup target (or use -b).\n"
+		exit "${KSFT_SKIP}"
+	fi
+}
+
+check_vng() {
+	local tested_versions=("1.36" "1.37")
+	local version
+	local ok=0
+
+	version="$(vng --version)"
+	for tv in "${tested_versions[@]}"; do
+		if [[ "${version}" == *"${tv}"* ]]; then
+			ok=1
+			break
+		fi
+	done
+
+	if [[ "${ok}" -eq 0 ]]; then
+		printf "warning: vng version '%s' has not been tested and may " "${version}" >&2
+		printf "not function properly.\n\tThe following versions have been tested: " >&2
+		echo "${tested_versions[@]}" >&2
+	fi
+}
+
+build_selftests() {
+	make -C "${KERNEL_CHECKOUT}" headers_install \
+		INSTALL_HDR_PATH="${TMP_DIR}/usr" -j"$(nproc)"
+
+	local khdr="-isystem ${TMP_DIR}/usr/include"
+
+	if ! make -C "${SCRIPT_DIR}" KHDR_INCLUDES="${khdr}" -j"$(nproc)"; then
+		die "failed to build dmabuf-heaps selftests"
+	fi
+
+	if ! make -C "${CGROUP_DIR}" KHDR_INCLUDES="${khdr}" \
+		"${MEMCONTROL_TEST}" -j"$(nproc)"; then
+		die "failed to build cgroup/test_memcontrol selftest"
+	fi
+}
+
+handle_build() {
+	if ! ${BUILD}; then
+		return
+	fi
+
+	if [[ ! -d "${KERNEL_CHECKOUT}" ]]; then
+		echo "-b requires vmtest.sh called from the kernel source tree" >&2
+		exit 1
+	fi
+
+	pushd "${KERNEL_CHECKOUT}" &>/dev/null
+
+	if ! vng --kconfig --config "${SCRIPT_DIR}/config"; then
+		die "failed to generate .config for kernel source tree (${KERNEL_CHECKOUT})"
+	fi
+
+	local vng_args=("-v" "--config" "${SCRIPT_DIR}/config" "--build")
+
+	if [[ -n "${BUILD_HOST}" ]]; then
+		vng_args+=("--build-host" "${BUILD_HOST}")
+	fi
+
+	if [[ -n "${BUILD_HOST_PODMAN_CONTAINER_NAME}" ]]; then
+		vng_args+=("--build-host-exec-prefix" \
+			   "podman exec -ti ${BUILD_HOST_PODMAN_CONTAINER_NAME}")
+	fi
+
+	if ! vng "${vng_args[@]}"; then
+		die "failed to build kernel from source tree (${KERNEL_CHECKOUT})"
+	fi
+
+	build_selftests
+
+	popd &>/dev/null
+}
+
+make_runner() {
+	# virtme-ng shares the host filesystem, so TMP_DIR is accessible
+	# inside the VM at the same absolute path.
+	cat > "${TMP_DIR}/run_tests.sh" <<-EOF
+	#!/bin/sh
+	set -u
+	PASS=0; FAIL=0; SKIP=0; N=0
+
+	run() {
+		name="\$1"; shift
+		N=\$((N+1))
+		"\$@"; rc=\$?
+		if   [ \$rc -eq 0 ]; then echo "ok \$N \$name";        PASS=\$((PASS+1))
+		elif [ \$rc -eq 4 ]; then echo "ok \$N \$name # SKIP"; SKIP=\$((SKIP+1))
+		else                      echo "not ok \$N \$name";    FAIL=\$((FAIL+1))
+		fi
+	}
+
+	run "dmabuf-heap charge_fd ioctl"	${DMABUF_HEAP_TEST}
+	run "memcontrol dma-buf memcg"	${MEMCONTROL_TEST} test_memcg_dmabuf
+	echo "# PASS=\$PASS SKIP=\$SKIP FAIL=\$FAIL"
+	[ \$FAIL -eq 0 ]
+	EOF
+	chmod +x "${TMP_DIR}/run_tests.sh"
+}
+
+run_vm() {
+	local verbose_opt=""
+	local kernel_opt=""
+
+	${VERBOSE} && verbose_opt="--verbose"
+
+	# If we are running from within the kernel source tree, use the kernel
+	# source tree as the kernel to boot, otherwise use the running kernel.
+	if [[ "$(realpath "$(pwd)")" == "${KERNEL_CHECKOUT}"* ]]; then
+		kernel_opt="${KERNEL_CHECKOUT}"
+	fi
+
+	vng --run ${kernel_opt} ${verbose_opt} --user root --memory 512M \
+		--exec "${TMP_DIR}/run_tests.sh"
+}
+
+while getopts :hvbH:p: o
+do
+	case $o in
+	v) VERBOSE=true;;
+	b) BUILD=true;;
+	H) BUILD_HOST=$OPTARG;;
+	p) BUILD_HOST_PODMAN_CONTAINER_NAME=$OPTARG;;
+	h|*) usage;;
+	esac
+done
+shift $((OPTIND-1))
+
+trap cleanup EXIT
+
+check_vng
+handle_build
+check_deps
+make_runner
+
+echo "Booting VM and running tests..."
+run_vm

-- 
2.55.0

Re: [PATCH RFC v2 8/8] selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/dmabuf-heaps: missing skip check in test_xfer_charge_cgroupfd_self()
- [Medium] selftests/dmabuf-heaps: missing cgroup2 and memcg checks in open_self_cgroupfd()
- [Medium] selftests/dmabuf-heaps: vmtest.sh executes runner from unshared /tmp in guest
- [Medium] selftests/dmabuf-heaps: vmtest.sh runs vng/make before check_deps()
--

commit 0809a03b2fbc81e6f2b637ef5c0744b2c8ae99f1
Author: Albert Esteve <aesteve@redhat.com>

selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

This commit adds selftests for dma-buf memcg accounting via charge_fd in
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c and
tools/testing/selftests/cgroup/test_memcontrol.c, along with a virtme-ng
runner script in tools/testing/selftests/dmabuf-heaps/vmtest.sh.

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index 1d49df6719198..b1c58e5883f64 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c

[ ... ]

> @@ -496,6 +505,213 @@ static void test_alloc_no_fd_leak_on_efault(char *heap_name)

[ ... ]

> +static int open_self_cgroupfd(void)
> +{
> +	char cgroup_path[PATH_MAX];
> +	char buf[PATH_MAX];
> +	FILE *f;
> +
> +	f = fopen("/proc/self/cgroup", "r");
> +	if (!f)
> +		return -1;
> +
> +	cgroup_path[0] = '\0';
> +	while (fgets(buf, sizeof(buf), f)) {
> +		char *p = buf;
> +
> +		if (strncmp(p, "0::", 3) == 0) {
> +			p += 3;
> +			p[strcspn(p, "\n")] = '\0';
> +			snprintf(cgroup_path, sizeof(cgroup_path),
> +				 "/sys/fs/cgroup%s", p);
> +			break;
> +		}
> +	}
> +	fclose(f);
> +
> +	if (!cgroup_path[0])
> +		return -1;
> +
> +	return open(cgroup_path, O_RDONLY | O_DIRECTORY);
> +}
> +
> +/*
> + * Test: allocate charging the calling process's own cgroup via a cgroupfd.
> + */
> +static void test_alloc_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	if (!pidfd_alloc_supported(heap_fd)) {
> +		ksft_test_result_skip("charge_fd not supported by this kernel\n");
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		goto out;
> +	}
> +
> +	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, cgroup_fd, &dmabuf_fd);
> +	ksft_test_result(!ret, "Allocation with cgroupfd %d\n", ret);
> +	if (dmabuf_fd >= 0)
> +		close(dmabuf_fd);
> +	close(cgroup_fd);
> +out:
> +	close(heap_fd);
> +}

[Severity: Medium]
Can open_self_cgroupfd() return a directory fd that causes
test_alloc_cgroupfd_self() and test_xfer_charge_cgroupfd_self() to report a
hard failure instead of skipping when cgroup v2 or the memory controller is
not active on that path?

In open_self_cgroupfd(), the 0:: entry from /proc/self/cgroup is appended to a
hardcoded /sys/fs/cgroup prefix without checking that /sys/fs/cgroup is a
cgroup2 mount. On systems using a hybrid hierarchy where /sys/fs/cgroup is a
tmpfs and unified cgroup v2 is mounted at /sys/fs/cgroup/unified, opening
/sys/fs/cgroup/ succeeds on tmpfs, and the kernel's cgroup_get_from_fd() then
rejects charge_fd with EBADF.

Furthermore, unlike the pidfd path in dma_heap_ioctl_allocate() and
dma_buf_xfer_charge() which resolves the effective memcg via
get_mem_cgroup_from_mm(task->mm), the cgroupfd path calls
cgroup_css(cgrp, &memory_cgrp_subsys) directly on the target cgroup and
returns ENOENT if the memory controller is not enabled for that specific
cgroup (for example, when the test runs in a non-root cgroup whose parent has
not enabled +memory in cgroup.subtree_control).

Should open_self_cgroupfd() verify that the resolved directory is on a
cgroup2 filesystem with the memory controller enabled, or should the callers
skip when the ioctl fails with ENOENT or EBADF?

> +
> +/*
> + * Test: transfer a dma-buf's charge to self cgroup via DMA_BUF_IOCTL_XFER_CHARGE.
> + */
> +static void test_xfer_charge_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +	struct dma_buf_transfer_charge xfer = { 0 };
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd);
> +	if (ret) {
> +		ksft_test_result_fail("Allocation failed %d\n", ret);
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		close(dmabuf_fd);
> +		goto out;
> +	}
> +
> +	xfer.charge_fd = cgroup_fd;
> +	ret = ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
> +	ksft_test_result(!ret, "XFER_CHARGE to self cgroupfd %d\n", ret);
> +
> +	close(cgroup_fd);
> +	close(dmabuf_fd);
>  out:
>  	close(heap_fd);
>  }

[Severity: Medium]
Unlike test_alloc_pidfd_self(), test_alloc_pidfd_child(), and
test_alloc_cgroupfd_self(), test_xfer_charge_cgroupfd_self() does not call
pidfd_alloc_supported(heap_fd) or check for errno == ENOTTY when
ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer) fails.

On an unpatched kernel where dmabuf_heap_alloc() and open_self_cgroupfd()
both succeed, won't ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer) return
-1 with errno == ENOTTY and cause ksft_test_result(!ret, ...) to report a
failure instead of skipping?

[ ... ]

> diff --git a/tools/testing/selftests/dmabuf-heaps/vmtest.sh b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> new file mode 100755
> index 0000000000000..434857b765b84
> --- /dev/null
> +++ b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> @@ -0,0 +1,205 @@

[ ... ]

> +readonly DMABUF_HEAP_TEST="${SCRIPT_DIR}"/dmabuf-heap
> +readonly MEMCONTROL_TEST="${CGROUP_DIR}"/test_memcontrol
> +readonly TMP_DIR=$(mktemp -d /tmp/dmabuf-vmtest.XXXXXXXX)

[ ... ]

> +build_selftests() {
> +	make -C "${KERNEL_CHECKOUT}" headers_install \
> +		INSTALL_HDR_PATH="${TMP_DIR}/usr" -j"$(nproc)"
> +
> +	local khdr="-isystem ${TMP_DIR}/usr/include"
> +
> +	if ! make -C "${SCRIPT_DIR}" KHDR_INCLUDES="${khdr}" -j"$(nproc)"; then
> +		die "failed to build dmabuf-heaps selftests"
> +	fi
> +
> +	if ! make -C "${CGROUP_DIR}" KHDR_INCLUDES="${khdr}" \
> +		"${MEMCONTROL_TEST}" -j"$(nproc)"; then
> +		die "failed to build cgroup/test_memcontrol selftest"
> +	fi
> +}

[ ... ]

> +make_runner() {
> +	# virtme-ng shares the host filesystem, so TMP_DIR is accessible
> +	# inside the VM at the same absolute path.
> +	cat > "${TMP_DIR}/run_tests.sh" <<-EOF

[ ... ]

> +	EOF
> +	chmod +x "${TMP_DIR}/run_tests.sh"
> +}
> +
> +run_vm() {
> +	local verbose_opt=""
> +	local kernel_opt=""
> +
> +	${VERBOSE} && verbose_opt="--verbose"
> +
> +	# If we are running from within the kernel source tree, use the kernel
> +	# source tree as the kernel to boot, otherwise use the running kernel.
> +	if [[ "$(realpath "$(pwd)")" == "${KERNEL_CHECKOUT}"* ]]; then
> +		kernel_opt="${KERNEL_CHECKOUT}"
> +	fi
> +
> +	vng --run ${kernel_opt} ${verbose_opt} --user root --memory 512M \
> +		--exec "${TMP_DIR}/run_tests.sh"
> +}

[Severity: Medium]
Will "${TMP_DIR}/run_tests.sh" be visible inside the guest VM when TMP_DIR is
created under /tmp?

When virtme-ng boots the guest, virtme-init mounts a fresh tmpfs over /tmp
inside the VM, which masks the host's /tmp directory unless TMP_DIR is
explicitly shared via --rodir / --rwdir or created under a shared directory
such as SCRIPT_DIR. Won't vng --run --exec "${TMP_DIR}/run_tests.sh" fail in
the guest because /tmp/dmabuf-vmtest.XXXXXXXX/run_tests.sh does not exist
there?

> +
> +while getopts :hvbH:p: o
> +do
> +	case $o in
> +	v) VERBOSE=true;;
> +	b) BUILD=true;;
> +	H) BUILD_HOST=$OPTARG;;
> +	p) BUILD_HOST_PODMAN_CONTAINER_NAME=$OPTARG;;
> +	h|*) usage;;
> +	esac
> +done
> +shift $((OPTIND-1))
> +
> +trap cleanup EXIT
> +
> +check_vng
> +handle_build
> +check_deps
> +make_runner
> +
> +echo "Booting VM and running tests..."
> +run_vm

[Severity: Medium]
Should check_deps() verify vng and make before check_vng() and handle_build()
are called?

Right now, if vng or make is not installed, check_vng() runs vng --version
first and prints a command-not-found error plus an untested-version warning,
and if -b is passed, handle_build() fails via die() with KSFT_FAIL instead of
exiting with KSFT_SKIP in check_deps().

Also, in handle_build(), the script uses pushd "${KERNEL_CHECKOUT}" and popd
to build the kernel, whereas run_vm() only sets kernel_opt="${KERNEL_CHECKOUT}"
when $(pwd) is inside KERNEL_CHECKOUT. If vmtest.sh -b is invoked from
outside KERNEL_CHECKOUT, won't run_vm() ignore the freshly built kernel and
boot the running host kernel instead? And in build_selftests(), should the
make headers_install command also be checked for failure before building the
tests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=8