Re: [PATCH v5 9/9] mm/page_owner: use memcg_data snapshot instead of PageMemcgKmem() to avoid TOCTOU VM_BUG_ON

[Ye Liu]

在 2026/7/10 23:56, Zi Yan 写道:
> On 10 Jul 2026, at 2:51, Ye Liu wrote:
> 
>> 在 2026/7/2 10:02, Zi Yan 写道:
>>> On 1 Jul 2026, at 2:10, 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, at the end it calls PageMemcgKmem(page)
>>>> which internally calls folio_memcg_kmem() — and that function re-reads
>>>> folio->memcg_data and page->compound_head locklessly, 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 this final call, the
>>>> VM_BUG_ON assertions can fire on debug builds (CONFIG_DEBUG_VM=y),
>>>> causing a kernel panic.
>>>>
>>>> Fix by reusing the memcg_data snapshot already taken at function entry
>>>> instead of calling PageMemcgKmem(), which is semantically equivalent:
>>>> PageMemcgKmem()->folio_memcg_kmem()->folio->memcg_data & MEMCG_DATA_KMEM.
>>>> This avoids both the TOCTOU window and the assertions entirely.
>>>>
>>>> Signed-off-by: Ye Liu <ye.liu@xxxxxxxxx>
>>>> ---
>>>>  mm/page_owner.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>> LGTM.
>>>
>>> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>Hi,Zi,Vlastimil
>>
>> This patch still has warnings (see sashiko link[1]).
>> I've made the following modifications, or do you have any better suggestions?
> 
> Maybe use snapshot_page() from mm/debug.c, so that you do not need to replicate
> the code from folio_memcg_check(). You probably still need the rcu_read_lock()
> when reading memcg_data from the snapshot to prevent memcg going away.
> 
But it does a full struct page + struct folio memcpy with retry loops,        
which is more than what we need here -- we only care about memcg_data.        
The READ_ONCE() snapshot is sufficient since the function already             
holds rcu_read_lock() to keep the memcg alive once resolved.                  
                                                                              
The two-line inline of folio_memcg_check() logic is minimal, and we need      
the OBJEXTS check to be explicit anyway (to print "Slab cache page\n"         
 -- page_memcg_check() just returns NULL silently for slab pages).            
                                                                              
Therefore, I don't recommend using snaps_page. Are there any better solutions?

>>
>> [1]:https://sashiko.dev/#/patchset/20260701061101.344679-1-ye.liu@xxxxxxxxx
>>
>> 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:
>>
>>>
>>> Best Regards,
>>> Yan, Zi
>>
>> -- 
>> Thanks,
>> Ye Liu
> 
> 
> Best Regards,
> Yan, Zi

-- 
Thanks,
Ye Liu