linux-kernel - Re: [PATCH v2 4/4] mm: page_owner: use new iteration API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68e82e87-606e-4443-99d3-7de6f665ce05@redhat.com>
Date: Tue, 25 Feb 2025 17:30:27 -0500
From: Luiz Capitulino <luizcap@...hat.com>
To: David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, yuzhao@...gle.com, pasha.tatashin@...een.com
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, muchun.song@...ux.dev
Subject: Re: [PATCH v2 4/4] mm: page_owner: use new iteration API

On 2025-02-25 11:44, David Hildenbrand wrote:
> On 24.02.25 22:59, Luiz Capitulino wrote:
>> The page_ext_next() function assumes that page extension objects for a
>> page order allocation always reside in the same memory section, which
>> may not be true and could lead to crashes. Use the new page_ext
>> iteration API instead.
>>
>> Fixes: cf54f310d0d3 ("mm/hugetlb: use __GFP_COMP for gigantic folios")
>> Signed-off-by: Luiz Capitulino <luizcap@...hat.com>
>> ---
>>   mm/page_owner.c | 61 +++++++++++++++++++++++--------------------------
>>   1 file changed, 29 insertions(+), 32 deletions(-)
>>
> 
> [...]
> 
>>   void __reset_page_owner(struct page *page, unsigned short order)
>> @@ -293,11 +297,11 @@ void __reset_page_owner(struct page *page, unsigned short order)
>>       page_owner = get_page_owner(page_ext);
>>       alloc_handle = page_owner->handle;
>> +    page_ext_put(page_ext);
>>       handle = save_stack(GFP_NOWAIT | __GFP_NOWARN);
>> -    __update_page_owner_free_handle(page_ext, handle, order, current->pid,
>> +    __update_page_owner_free_handle(page, handle, order, current->pid,
>>                       current->tgid, free_ts_nsec);
>> -    page_ext_put(page_ext);
> 
> I assume moving that is fine ...
> 
> but I'll not that ...
> 
>> -    for (i = 0; i < (1 << new_page_owner->order); i++) {
>> +    rcu_read_lock();
>> +    for_each_page_ext(&old->page, 1 << new_page_owner->order, page_ext, iter) {
>> +        old_page_owner = get_page_owner(page_ext);
>>           old_page_owner->handle = migrate_handle;
>> -        old_ext = page_ext_next(old_ext);
>> -        old_page_owner = get_page_owner(old_ext);
>>       }
>> +    rcu_read_unlock();
>>       page_ext_put(new_ext);
>>       page_ext_put(old_ext);
> 
> ... here you are not moving it?
> 
> 
> In general, LGTM, only the remaining page_ext_put() are a bit confusing.

Which part you found confusing: the fact that I'm not moving them up or that
we still make use of them?

For this hunk, I decided to keep them where they are because 'new_page_owner',
which is a page extension from 'next_ext', is still used in the last loop. So
I decided to free them all at the end for simplicity.

The other part is, page_ext_get() and page_ext_put() are still valid functions
for getting specific page extensions outside of loops and the usage in
__folio_copy_owner() (and a few other cases) seems valid to me.