Re: [PATCH v3] mm/page_alloc: clear page->private in free_pages_prepare()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 2/9/26 17:16, David Hildenbrand (Arm) wrote:
>>> I recall that freeing pages with page->private set was allowed. Although
>>> I once wondered whether we should actually change that.
>>
>> But if that is allowed, we can end up with tail page's private non zero,
>> because that free page can merge with a lower PFN buddy and its ->private
>> is not reset. See __free_one_page().
> 
> Right. Or someone could use page->private on tail pages and free non- 
> zero ->private that way.
> 
> [...]
> 
>>
>> For the issue reported by Mikhail[2], the page comes from vmalloc(), 
>> so it will not be split.
>> For other cases, a page/folio needs to be compound to be splittable 
>> and prep_compound_tail()
>> sets all tail page's private to 0. So that check is not that useful.
> 
> Thanks.
> 
>>
>> And the issue we are handling here is non compound high order page 
>> allocation. No one is
>> clearing ->private for all pages right now.
> 
> Right.
> 
>>
>> OK, I think we want to decide whether it is OK to have a page with set 
>> ->private at
>> page free time.
> 
> Right. And whether it is okay to have any tail->private be non-zero.
> 
>> If no, we can get this patch in and add a VM_WARN_ON_ONCE(page->private)
>> to catch all violators. If yes, we can use Mikhail's original patch, 
>> zeroing ->private
>> in split_page() and add a comment on ->private:
>>
>> 1. for compound page allocation, prep_compound_tail() is responsible 
>> for resetting ->private;
>> 2. for non compound high order page allocation, split_page() is 
>> responsible for resetting ->private.
> 
> Ideally, I guess, we would minimize the clearing of the ->private fields.
> 
> If we could guarantee that *any* pages in the buddy have ->private 
> clear, maybe
> prep_compound_tail() could stop clearing it (and check instead).
> 
> So similar to what Vlasta said, maybe we want to (not check but actually 
> clear):
> 
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index e4104973e22f..4960a36145fe 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1410,6 +1410,7 @@ __always_inline bool free_pages_prepare(struct 
> page *page,
>                                  }
>                          }
>                          (page + i)->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> +                       set_page_private(page + i, 0);
>                  }
>          }

Thinking again, maybe it is indeed better to rework the code to not 
allow freeing pages with ->private on any page. Then, we only have to 
zero it out where we actually used it and could check here that all
->private is 0.

I guess that's a bit more work, and any temporary fix would likely just do.

-- 
Cheers,

David