Re: [PATCH v4 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:
- [Medium] mm: memcontrol: 32-bit nr_pages truncation in dmabuf charge helpers
--

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

memcg: Add MEMCG_DMABUF accounting infrastructure

This commit introduces the MEMCG_DMABUF stat counter and helper functions
to reserve, cancel, and uncharge dma-buf memory against a memory cgroup,
tracking the active charge via a memcg pointer on struct dma_buf.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 856a7d07586cc..e53dd7eebc3b5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c

[ ... ]

> @@ -5658,6 +5661,73 @@ void mem_cgroup_flush_workqueue(void)
>  	flush_workqueue(memcg_wq);
>  }
>  
> +/**
> + * mem_cgroup_reserve_dmabuf - charge a memcg for a dma-buf allocation
> + * @memcg: memcg to charge (may be %NULL)
> + * @gfp_mask: reclaim mode
> + * @nr_pages: number of pages to charge
> + *
> + * Charges @nr_pages against @memcg. Call this before allocating pages so
> + * the cgroup limit is enforced up front. On success the caller must
> + * finalize the charge with the MEMCG_DMABUF stat update, or call
> + * mem_cgroup_cancel_dmabuf() to undo it.
> + *
> + * Returns %true on success, %false if the charge would exceed the limit.
> + */
> +bool mem_cgroup_reserve_dmabuf(struct mem_cgroup *memcg, gfp_t gfp_mask,
> +			       unsigned int nr_pages)
> +{
> +	if (!memcg || mem_cgroup_disabled())
> +		return true;
> +
> +	if (try_charge(memcg, gfp_mask, nr_pages))
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(mem_cgroup_reserve_dmabuf);
> +
> +/**
> + * mem_cgroup_cancel_dmabuf - undo a dma-buf charge
> + * @memcg: memcg to uncharge (may be %NULL)
> + * @nr_pages: same value passed to mem_cgroup_reserve_dmabuf()
> + *
> + * Returns @nr_pages of charge to @memcg. Use when the allocation fails
> + * after mem_cgroup_reserve_dmabuf().
> + */
> +void mem_cgroup_cancel_dmabuf(struct mem_cgroup *memcg, unsigned int nr_pages)
> +{
> +	if (!memcg)
> +		return;
> +
> +	if (!mem_cgroup_is_root(memcg))
> +		refill_stock(memcg, nr_pages);
> +}
> +EXPORT_SYMBOL_GPL(mem_cgroup_cancel_dmabuf);
> +
> +/**
> + * mem_cgroup_uncharge_dmabuf - uncharge a dma-buf from its memcg
> + * @dmabuf: buffer to uncharge
> + *
> + * Reverses a finalized dma-buf charge. 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);
> +	mem_cgroup_cancel_dmabuf(memcg, nr_pages);
> +	mem_cgroup_put(memcg);
> +	dmabuf->memcg = NULL;
> +}

[Severity: Medium]
Can passing a 32-bit unsigned int for nr_pages in
mem_cgroup_reserve_dmabuf(), mem_cgroup_cancel_dmabuf(), and
mem_cgroup_uncharge_dmabuf() in mm/memcontrol.c (and include/linux/memcontrol.h)
truncate 64-bit dma-buf sizes?

struct dma_buf stores size as a 64-bit size_t, and in the following commit
("dma-heap: charge dma-buf memory via explicit memcg"),
dma_heap_ioctl_allocate() -> system_heap_allocate() passes a 64-bit
PAGE_ALIGN(len) >> PAGE_SHIFT to mem_cgroup_reserve_dmabuf() before
entering its while (size_remaining > 0) page allocation loop without
__GFP_ACCOUNT.

If a userspace caller requests an allocation where len >= (1ULL << (32 +
PAGE_SHIFT)) (for example, (1ULL << 44) + PAGE_SIZE on a 4 KiB page
system), PAGE_ALIGN(len) >> PAGE_SHIFT wraps to 1 page when converted to
unsigned int nr_pages. try_charge() will then succeed against a small
memory.max limit while system_heap_allocate() proceeds to allocate
unaccounted pages using the full 64-bit size_remaining.

Additionally, in mem_cgroup_uncharge_dmabuf(), passing -nr_pages to
mod_memcg_state(memcg, MEMCG_DMABUF, -nr_pages) (which takes a signed int
val) will overflow when nr_pages exceeds INT_MAX. Should nr_pages use
unsigned long (or size_t) across these helpers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-v2_20230123_tjmercier_google_com-v4-0-1819a017000b@redhat.com?part=1