Re: [PATCH v6 9/9] mm/page_owner: use memcg_data snapshot to avoid TOCTOU in print_page_owner_memcg()

[Vlastimil Babka (SUSE)]

On 7/14/26 03:51, Ye Liu wrote:
> print_page_owner_memcg() takes a snapshot of page->memcg_data via
> READ_ONCE at the top of the function and guards against tail pages and
> NULL memcg_data.  However, it later calls two functions that re-read
> page->memcg_data locklessly:
> 
>   1) page_memcg_check(page) — re-reads page->memcg_data;
>   2) PageMemcgKmem(page) — calls folio_memcg_kmem(), which re-reads
>      folio->memcg_data and folio->page->compound_head, wrapping both
>      in VM_BUG_ON assertions:
> 
>         VM_BUG_ON_PGFLAGS(PageTail(&folio->page), &folio->page);
>         VM_BUG_ON_FOLIO(folio->memcg_data & MEMCG_DATA_OBJEXTS, folio);
> 
> If the page is concurrently freed and reallocated as a THP tail page
> or a slab page between the initial guards and these later calls, the
> VM_BUG_ON assertions can fire on debug builds (CONFIG_DEBUG_VM=y),
> causing a kernel panic.
> 
> Fix both TOCTOU issues by using the memcg_data snapshot throughout:
>   - Extract objcg from the snapshot via objcg = (void *)(memcg_data &
>     ~OBJEXTS_FLAGS_MASK) instead of calling page_memcg_check(page);
>   - Test (memcg_data & MEMCG_DATA_KMEM) instead of calling
>     PageMemcgKmem(page), which is semantically equivalent:
>     PageMemcgKmem()->folio_memcg_kmem()->folio->memcg_data &
>     MEMCG_DATA_KMEM.
>   - When memcg_data has MEMCG_DATA_OBJEXTS set, early-return after
>     printing "Slab cache page\n" since objcg != memcg for slab pages
>     and there is no meaningful cgroup to look up.

These points have too much detail that's already in the code. Would just
mention that we opencode applicable parts of page_memcg_check() and
PageMemcgKmem() using the snapshot?

> This avoids both TOCTOU windows and the assertions entirely.
> 
> Signed-off-by: Ye Liu <ye.liu@xxxxxxxxx>

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

Was all of this reported by sashiko? At least the new-in-v6 was?
Then:

Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>

But it's no longer a cleanup but a fix, so probably this?

Fixes: fcf8935832b8 ("mm/page_owner: print memcg information")
Cc: stable@xxxxxxxxxxxxxxx

It's not fixing a new regression so I think it's fine to keep it part of
this series for next release and not need to split out for mm-hotfixes.

> ---
> Changes in v6:
> - Rename patch to cover both TOCTOU fixes rather than only
>   PageMemcgKmem().
> - Also replace page_memcg_check(page) with extracting objcg from the
>   memcg_data snapshot to fix a second TOCTOU issue.
> - Add early return for the MEMCG_DATA_OBJEXTS (slab) case since
>   objcg != memcg for slab pages and there is no cgroup to look up.
> - Update commit message to cover all changes.
> - Link: https://lore.kernel.org/all/20260701061101.344679-10-ye.liu@xxxxxxxxx/
>  mm/page_owner.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 2e3880053a34..e18512a49e38 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -540,6 +540,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
>  					 struct page *page)
>  {
>  	unsigned long memcg_data;
> +	struct obj_cgroup *objcg;
>  	struct mem_cgroup *memcg;
>  	bool online;
>  	char name[80];
> @@ -549,11 +550,14 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
>  	if (!memcg_data || PageTail(page))
>  		goto out_unlock;
>  
> -	if (memcg_data & MEMCG_DATA_OBJEXTS)
> +	if (memcg_data & MEMCG_DATA_OBJEXTS) {
>  		ret += scnprintf(kbuf + ret, count - ret,
>  				"Slab cache page\n");
> +		goto out_unlock;
> +	}
>  
> -	memcg = page_memcg_check(page);
> +	objcg = (void *)(memcg_data & ~OBJEXTS_FLAGS_MASK);
> +	memcg = objcg ? obj_cgroup_memcg(objcg) : NULL;
>  	if (!memcg)
>  		goto out_unlock;
>  
> @@ -561,7 +565,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
>  	cgroup_name(memcg->css.cgroup, name, sizeof(name));
>  	ret += scnprintf(kbuf + ret, count - ret,
>  			"Charged %sto %smemcg %s\n",
> -			PageMemcgKmem(page) ? "(via objcg) " : "",
> +			(memcg_data & MEMCG_DATA_KMEM) ? "(via objcg) " : "",
>  			online ? "" : "offline ",
>  			name);
>  out_unlock: