Re: [PATCH] mm/hugetlb_cgroup: prevent use-after-free during cgroup offlining

[Andrew Morton]

On Tue, 20 Oct 2026 19:03:45 +0900 Daehyeon Ko <4ncienth@xxxxxxxxx> wrote:

> A normal hugetlb charge obtains the current task's hugetlb cgroup with
> css_tryget(), charges its page counter, and drops the CSS reference before
> the folio is committed under hugetlb_lock.
> 
> The task can be migrated while the allocation is in progress.  If the old
> CSS is then offlined, its final active-folio scan can finish before the
> charge becomes visible.  The allocation can subsequently charge that dying
> CSS and commit its pointer to the folio.  Once the CSS is freed, uncharging
> the folio accesses the freed hugetlb_cgroup, including its page counter and
> per-node usage.
> 
> Keep the reference acquired by the charge until commit or cancellation.  At
> commit time, while holding hugetlb_lock, move an uncommitted charge out of
> each dying CSS to the first live ancestor.  The hierarchical charge already
> includes every ancestor, so moving it only requires canceling each dying
> CSS's local count.
> 
> This orders commit against the offline active-folio scan: either the scan
> runs after commit and reparents the folio, or commit observes the dying CSS
> after the scan and selects a live ancestor.  The retained reference also
> keeps the originally selected hugetlb_cgroup alive through this decision.
> Reservation charges retain their existing CSS lifetime and are not
> reparented.
> 
> On an unmodified v7.3 kernel with a 128-page 2 MiB pool, a
> capability-free uid 1000 allocator racing administrator rmdir reproduced
> KASAN writes to a freed hugetlb_cgroup in three of three boots.  Controller
> disable reproduced the same fault.  With this change, 5,000 rmdir rounds
> with 1,075,055 successful faults and 2,000 controller-disable rounds with
> 1,707,348 successful faults completed without KASAN, LOCKDEP, page-counter,
> or refcount reports.
> 
> AI-assisted source review was used to identify the race and to help draft
> the reproducer and fix.  The interleaving and the resulting allocation,
> free, and use-after-free stacks were verified with tracefs and KASAN.

Thanks.  I ran this through my evolving reviewbot.  It liked the change
and agreed with it.

One claim:

>
> ...
>
> @@ -419,15 +438,17 @@ static void __hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,
>  					     struct hugetlb_cgroup *h_cg,
>  					     bool rsvd)
>  {
> -	if (hugetlb_cgroup_disabled() || !h_cg)
> +	if (!h_cg)
>  		return;
>  
> -	page_counter_uncharge(__hugetlb_cgroup_counter_from_cgroup(h_cg, idx,
> -								   rsvd),
> -			      nr_pages);
> +	if (!hugetlb_cgroup_disabled()) {

This test is unnecessary.  Once h_cg is non-NULL, a successful charge
necessarily came from an enabled controller, and cgroup_disable= is a
boot-time static-key setting.  Please just unconditionally call
page_counter_uncharge() after the existing !h_cg check, followed by
css_put().

> +		struct page_counter *counter;
>  
> -	if (rsvd)
> -		css_put(&h_cg->css);
> +		counter = __hugetlb_cgroup_counter_from_cgroup(h_cg, idx, rsvd);
> +		page_counter_uncharge(counter, nr_pages);
> +	}
> +
> +	css_put(&h_cg->css);
>  }
>  
>  void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,