Re: [PATCH 04/11] memcg: add cma charge/uncharge functions for area counters

On Fri, Aug 21, 2026 at 07:21:16PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Dynamically disabling memory_cma_accounting via cgroupfs remount prevents previously charged CMA pages from being uncharged, leading to permanent resource leaks and buddy allocator corruption.
> - [High] Hardcoded GFP_KERNEL strips caller allocation constraints, causing unwanted memcg OOM kills and warnings.
> --
> 
> --- Patch [4]: [PATCH 04/11] memcg: add cma charge/uncharge functions for area counters ---
> commit 637000e435d7ad7a20a55418ada4d77115a47082
> Author: Eric Chanudet <[email protected]>
> 
> memcg: add cma charge/uncharge functions for area counters
> 
> Introduce mem_cgroup_charge_cma() and mem_cgroup_uncharge_cma() to
> account CMA allocations under memcg when memory_cma_accounting is
> enabled in cgroupfs. memcg counters to account for allocation in each
> CMA area are kcalloc'd in mem_cgroup_alloc() and added to the
> page-counter hierarchy in mem_cgroup_css_alloc().
> 
> [ ... ]
> 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 6dc4888a90f3f..8eca00d30fc50 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> 
> [ ... ]
> 
> > @@ -5213,6 +5233,102 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, unsigned short id,
> >  	return ret;
> >  }
> >  
> > +#ifdef CONFIG_CMA
> > +static bool memcg_accounts_cma(void)
> > +{
> > +	return cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_CMA_ACCOUNTING;
> > +}
> > +
> > +int mem_cgroup_charge_cma(struct page *page, unsigned long count,
> > +			  struct cma *cma)
> > +{
> > +	struct page_counter *counter, *fail;
> > +	struct obj_cgroup *objcg;
> > +	struct mem_cgroup *memcg;
> > +	unsigned int i;
> > +	int rc = 0;
> > +
> > +	if (mem_cgroup_disabled() || !memcg_accounts_cma())
> > +		return 0;
> > +
> > +	memcg = get_mem_cgroup_from_current();
> > +	if (!memcg)
> > +		return 0;
> > +
> > +	if (mem_cgroup_is_root(memcg))
> > +		goto cgroup_put;
> > +
> > +	rc = try_charge_memcg(memcg, GFP_KERNEL, count);
> 
> [Severity: High]
> Can this hardcoded GFP_KERNEL cause unwanted memcg OOM kills and warnings? By
> stripping the caller's allocation constraints, if the memcg is at its limit and
> the caller requested no warnings or retry constraints, try_charge_memcg() could
> invoke the memcg OOM killer and print allocation failure warnings.
> 
> Is it possible to pass the caller's gfp_t mask to this routine instead of
> hardcoding GFP_KERNEL?
> 

Yes, charge_cma should pass gfp. I don't think CMA allocs will have
other flags that affect try_charge_memcg for now, but still.

> > +	if (rc)
> > +		goto cgroup_put;
> > +
> > +	counter = &memcg->cma_counters[cma_area_index(cma)];
> > +	if (!page_counter_try_charge(counter, count, &fail)) {
> > +		refill_stock(memcg, count);
> > +		rc = -ENOMEM;
> > +		goto cgroup_put;
> > +	}
> > +
> > +	objcg = get_obj_cgroup_from_memcg(memcg);
> > +	if (folio_test_large(page_folio(page))) {
> > +		commit_charge(page_folio(page), objcg);
> > +	} else {
> > +		obj_cgroup_get_many(objcg, count - 1);
> > +		for (i = 0; i < count; i++)
> > +			commit_charge(page_folio(page + i), objcg);
> > +	}
> > +
> > +cgroup_put:
> > +	mem_cgroup_put(memcg);
> > +	return rc;
> > +}
> > +
> > +void mem_cgroup_uncharge_cma(struct page *page, unsigned long count,
> > +			     struct cma *cma)
> > +{
> > +	struct page_counter *counter;
> > +	struct obj_cgroup *objcg;
> > +	struct mem_cgroup *memcg;
> > +	struct folio *folio;
> > +	unsigned int i;
> > +
> > +	if (mem_cgroup_disabled() || !memcg_accounts_cma())
> > +		return;
> 
> [Severity: High]
> Can dynamically disabling memory_cma_accounting via cgroupfs remount cause
> permanent resource leaks and buddy allocator corruption here? 
> 

It does. folio->memcg_data/objcg should be checked instead.

> If a CMA allocation is charged while memory_cma_accounting is enabled, and a
> privileged user later remounts cgroup2 without specifying the
> memory_cma_accounting option, memcg_accounts_cma() will evaluate to false. 
> 
> This early return would skip the uncharge and leave folio->memcg_data
> populated. When the pages are returned to the buddy allocator, wouldn't
> __free_pages_prepare() detect the non-zero memcg_data and fire a bad_page()
> warning, permanently leaking the cgroup and cma_counters?
> 
> > +
> > +	/*
> > +	 * Get the objcg from the first page.
> > +	 * page_objcg() expects MEMCG_DATA_KMEM, but for CMA we used
> > +	 * commit_charge() which sets folio->memcg_data = objcg
> > +	 * without flags, so we cannot use it.
> > +	 */
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
> 

-- 
Eric Chanudet