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

[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.

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