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?