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

["David Wang" <00107082@163.com>]

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.....
Do you want me to do it or you take over and make further adjustments?



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