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 9:43 AM David Wang <00107082@163.com> wrote:
>
>
>
>
> At 2025-05-05 23:33:50, "Suren Baghdasaryan" <surenb@google.com> wrote:
> >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.
>
>
> Adding back pgalloc_tag_get() seems just reverting 51ff4d7486f0.....

Not quite. pgalloc_tag_split() and pgalloc_tag_swap() will still be
using __pgalloc_tag_get(), so would avoid the extra checks.

> Do you want me to do it or you take over and make further adjustments?

The change I suggested should be simple, so take a stab at it and I'll
review and ack. If you prefer me to make the change, let me know.

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