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

[Ye Liu]

在 2026/7/14 16:24, 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?
Yes,It's a bit wordy.
> 
>> 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.
> 
Hi Andrew,

Could you please help me revise the above? Should I send you V7?


>> ---
>> 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:
> 

-- 
Thanks,
Ye Liu