Re: [PATCH 05/11] mm/cma: charge cma allocation to memcg per area counters

On Fri, Aug 21, 2026 at 07:28:14PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Double memcg charging for HugeTLB CMA allocations causing `VM_BUG_ON_FOLIO` panics.
> - [High] Dynamic reconfiguration of `memory_cma_accounting` leaks charges and crashes the buddy allocator.
> - [Low] trace_cma_alloc_finish() logs a successful error code when the allocation fails due to memcg charging.
> --
> 
> --- Patch [5]: [PATCH 05/11] mm/cma: charge cma allocation to memcg per area counters ---
> commit 2d35f7be0d6e01fd43ca7628f3674cf1cec9ac0b
> Author: Eric Chanudet <[email protected]>
> 
> mm/cma: charge cma allocation to memcg per area counters
>     
> This commit hooks mem_cgroup_charge_cma() and mem_cgroup_uncharge_cma()
> into the CMA allocation and release paths. It charges pages to the
> current task's cgroup upon successful allocation, and handles uncharging
> and releasing pages if the allocation limit is reached.
> 
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 03f98f6d45a54..03eb3acbd3c75 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -904,6 +904,12 @@ static struct page *__cma_alloc_frozen(struct cma *cma,
> >  		cma_debug_show_areas(cma);
> >  	}
> >  
> > +	if (page && mem_cgroup_charge_cma(page, count, cma)) {
> 
> [Severity: Critical]
> Will this cause a panic for HugeTLB pages backed by CMA?
> 
> HugeTLB utilizes CMA for its pool. When the pool is expanded,
> cma_alloc_frozen_compound() invokes this path, which blindly charges the
> pages and populates folio->memcg_data.
> 
> Later, when a user process faults on the HugeTLB page, alloc_hugetlb_folio()
> distributes the page to the user and charges it again by calling
> mem_cgroup_charge_hugetlb(). This eventually invokes commit_charge(), which
> has this check:
> 
> mm/memcontrol.c:commit_charge() {
>     ...
>     VM_BUG_ON_FOLIO(folio_memcg_charged(folio), folio);
>     folio->memcg_data = (unsigned long)objcg;
>     ...
> }
> 
> Since folio->memcg_data was already set during the CMA allocation, won't this
> trigger the VM_BUG_ON_FOLIO panic?
> 

Indeed, and charging at allocation just won't work for hugepages in CMA.
I'm not sure how to reconcile it. Maybe just leave it accounted by
hugetlb cgroup and memcg as it is until the regular CMA is figured out?

> > +		free_contig_frozen_range(page_to_pfn(page), count);
> > +		cma_clear_bitmap(cma, &cma->ranges[r], page_to_pfn(page), count);
> > +		page = NULL;
> > +	}
> > +
> >  	pr_debug("%s(): returned %p\n", __func__, page);
> >  	trace_cma_alloc_finish(name, page ? page_to_pfn(page) : 0,
> >  			       page, count, align, ret);
> 
> [Severity: Low]
> Does this tracepoint log a success status when charging fails?
> 
> If mem_cgroup_charge_cma() fails, page is set to NULL, but the local variable
> ret is not updated. Since ret was likely set to 0 by a successful
> cma_range_alloc() earlier, trace_cma_alloc_finish() will receive ret = 0.
> 
> Should ret be updated to an error code like -ENOMEM before the tracepoint
> executes?

Yes.

> 
> [ ... ]
> 
> > @@ -997,6 +1003,7 @@ static void __cma_release_frozen(struct cma *cma, struct cma_memrange *cmr,
> >  
> >  	pr_debug("%s(page %p, count %lu)\n", __func__, (void *)pages, count);
> >  
> > +	mem_cgroup_uncharge_cma(pages, count, cma);
> 
> [Severity: High]
> Can this leak charges and crash the buddy allocator if CMA accounting is
> dynamically reconfigured?
> 
> If the system is booted or mounted with memory_cma_accounting, CMA pages
> are charged and folio->memcg_data is set. If the cgroup v2 filesystem is
> later remounted without memory_cma_accounting, mem_cgroup_uncharge_cma()
> will skip the uncharge because of this check:
> 
> mm/memcontrol.c:mem_cgroup_uncharge_cma() {
>     if (mem_cgroup_disabled() || !memcg_accounts_cma())
>         return;
>     ...
> }
> 
> When these pages are returned to the buddy allocator, they will still have
> a populated memcg_data field. The buddy allocator detects this in
> free_pages_prepare() and throws a bad page state:
> 
> mm/page_alloc.c:check_free_page_bad() {
>     ...
>     if (unlikely(page->memcg_data))
>         bad_reason = "page still charged to cgroup";
>     ...
> }
> 
> Should the uncharge path rely on the object's state (checking if
> folio->memcg_data is non-NULL) rather than the global configuration flag?
> 

Same as in Patch 4.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
> 

-- 
Eric Chanudet