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: <5bb20271-a92a-454e-90e7-8812fd01d31d@redhat.com>
Date: Thu, 27 Feb 2025 14:50:12 +0100
From: David Hildenbrand <david@...hat.com>
To: Luiz Capitulino <luizcap@...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 25.02.25 23:30, Luiz Capitulino wrote:
> 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?

How we are deferring page_ext_put() when not actually working on these
values anymore. The page_owner itself should not go away here unless we
have a serious bug.

To be precise, can't we simply do the following on top?

diff --git a/mm/page_owner.c b/mm/page_owner.c
index c9d2c688eb981..12044340adf89 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -356,26 +356,24 @@ void __split_page_owner(struct page *page, int old_order, int new_order)
  
  void __folio_copy_owner(struct folio *newfolio, struct folio *old)
  {
-       struct page_ext *old_ext;
-       struct page_ext *new_ext;
         struct page_ext *page_ext;
         struct page_ext_iter iter;
         struct page_owner *old_page_owner;
         struct page_owner *new_page_owner;
         depot_stack_handle_t migrate_handle;
  
-       old_ext = page_ext_get(&old->page);
-       if (unlikely(!old_ext))
+       page_ext = page_ext_get(&old->page);
+       if (unlikely(!page_ext))
                 return;
+       old_page_owner = get_page_owner(page_ext);
+       page_ext_put(page_ext);
  
-       new_ext = page_ext_get(&newfolio->page);
-       if (unlikely(!new_ext)) {
-               page_ext_put(old_ext);
+       page_ext = page_ext_get(&newfolio->page);
+       if (unlikely(!page_ext))
                 return;
-       }
+       new_page_owner = get_page_owner(page_ext);
+       page_ext_put(page_ext);
  
-       old_page_owner = get_page_owner(old_ext);
-       new_page_owner = get_page_owner(new_ext);
         migrate_handle = new_page_owner->handle;
         __update_page_owner_handle(&newfolio->page, old_page_owner->handle,
                                    old_page_owner->order, old_page_owner->gfp_mask,
@@ -402,9 +400,6 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
                 old_page_owner->handle = migrate_handle;
         }
         rcu_read_unlock();
-
-       page_ext_put(new_ext);
-       page_ext_put(old_ext);
  }
  
  void pagetypeinfo_showmixedcount_print(struct seq_file *m,


-- 
Cheers,

David / dhildenb