Re: [PATCH] mm/codetag: sub in advance when free non-compound high order pages

[Suren Baghdasaryan <surenb@google.com>]

On Mon, May 5, 2025 at 7:55 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 5/5/25 16:31, David Wang wrote:
> >
> >
> > At 2025-05-05 21:12:55, "Vlastimil Babka" <vbabka@suse.cz> wrote:
> >>On 5/4/25 08:19, David Wang wrote:
> >>> When page is non-compound, page[0] could be released by other
> >>> thread right after put_page_testzero failed in current thread,
> >>> pgalloc_tag_sub_pages afterwards would manipulate an invalid
> >>> page for accounting remaining pages:
> >>>
> >>> [timeline]   [thread1]                     [thread2]
> >>>   |          alloc_page non-compound
> >>>   V
> >>>   |                                        get_page, rf counter inc
> >>>   V
> >>>   |          in ___free_pages
> >>>   |          put_page_testzero fails
> >>>   V
> >>>   |                                        put_page, page released
> >>>   V
> >>>   |          in ___free_pages,
> >>>   |          pgalloc_tag_sub_pages
> >>>   |          manipulate an invalid page
> >>>   V
> >>>   V
> >>>
> >>> Move the tag page accounting ahead, and only account remaining pages
> >>> for non-compound pages with non-zero order.
> >>>
> >>> Signed-off-by: David Wang <00107082@163.com>

Thanks for reporting!

> >>
> >>Hmm, I think the problem was introduced by 51ff4d7486f0 ("mm: avoid extra
> >>mem_alloc_profiling_enabled() checks"). Previously we'd get the tag pointer
> >>upfront and avoid the page use-after-free.

Right, sorry I missed that.

> >
> >
> > Oh, you're right. I forgot to check history......
> >
> >
> >>
> >>It would likely be nicer to fix it by going back to that approach for
> >>___free_pages(), while hopefully keeping the optimisations of 51ff4d7486f0
> >>for the other call sites where it applies?
> >
> > After checking that commit, I kind of feels the changes in __free_pages are
> >  the major optimization of the commit....
>
> We could have both pgalloc_tag_get() to use in __free_page() as before
> 51ff4d7486f0, and keep __pgalloc_tag_get() to use in pgalloc_tag_split() and
> pgalloc_tag_swap().

Yes, we can add back pgalloc_tag_get() which would call
__pgalloc_tag_get() if mem_alloc_profiling_enabled() is true and
change pgalloc_tag_sub_pages() back to use tags instead of pages.
__free_pages() is the only user of that function, so that change
should not affect anything else.

>
> I think __free_page() didn't benefit from the stated purpose of "avoiding
> mem_alloc_profiling_enabled() ... which is often called after that check was
> already done"
>
> > What about revert that commit and make optimization by condition checks,
> > similar to what this patch did?
>
> The downside of the condition checks is they make the code more complex and
> might actually increase overhead when mem_alloc_profiling_enabled() is
> false, as those checks add non-static branches outside of the static branch
> that's mem_alloc_profiling_enabled().
>
> I think __free_pages() before 51ff4d7486f0 was quite ok.
>
> - pgalloc_tag_get() is done unconditionally, but its code is all inside the
> mem_alloc_profiling_enabled() static branch so that's a no-op when profiling
> is not enabled
>
> - pgalloc_tag_sub_pages() is also all behind the static branch inside. Also
> it's a very rare path anyway, most freeing should go through the
> put_page_testzero() being true.

Yeah, the main goal of that change in __free_page() was to make
__pgalloc_tag_get() a local function for alloc_tags and limiting the
direct use of struct alloc_tag in the core mm code. Obviously I
screwed up forgetting why we had to store the tag in the first place.
An additional comment in __free_page() is probably a good idea to
avoid confusion in the future.
Thanks,
Suren.

>
> > David
> >
>