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

[sashiko-bot@kernel.org]

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