linux-kernel - Re: [PATCH v9 2/8] mm/huge_memory: add two new (not yet used) functions for folio

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] [day] [month] [year] [list]

Message-ID: <4a9f102a-60db-475a-a933-975edb2fb1dd@redhat.com>
Date: Thu, 6 Mar 2025 10:19:41 +0100
From: David Hildenbrand <david@...hat.com>
To: Zi Yan <ziy@...dia.com>, Hugh Dickins <hughd@...gle.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 Ryan Roberts <ryan.roberts@....com>, Yang Shi <yang@...amperecomputing.com>,
 Miaohe Lin <linmiaohe@...wei.com>, Kefeng Wang <wangkefeng.wang@...wei.com>,
 Yu Zhao <yuzhao@...gle.com>, John Hubbard <jhubbard@...dia.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Kairui Song <kasong@...cent.com>, Liu Shixin <liushixin2@...wei.com>
Subject: Re: [PATCH v9 2/8] mm/huge_memory: add two new (not yet used)
 functions for folio_split()

On 05.03.25 22:08, Zi Yan wrote:
> On 5 Mar 2025, at 15:50, Hugh Dickins wrote:
> 
>> On Wed, 5 Mar 2025, Zi Yan wrote:
>>> On 4 Mar 2025, at 6:49, Hugh Dickins wrote:
>>>>
>>>> I think (might be wrong, I'm in a rush) my mods are all to this
>>>> "add two new (not yet used) functions for folio_split()" patch:
>>>> please merge them in if you agree.
>>>>
>>>> 1. From source inspection, it looks like a folio_set_order() was missed.
>>>
>>> Actually no. folio_set_order(folio, new_order) is called multiple times
>>> in the for loop above. It is duplicated but not missing.
>>
>> I was about to disagree with you, when at last I saw that, yes,
>> it is doing that on "folio" at the time of setting up "new_folio".
>>
>> That is confusing: in all other respects, that loop is reading folio
>> to set up new_folio.  Do you have a reason for doing it there?
> 
> No. I agree your fix is better. Just point out folio_set_order() should
> not trigger a bug.
> 
>>
>> The transient "nested folio" situation is anomalous either way.
>> I'd certainly prefer it to be done at the point where you
>> ClearPageCompound when !new_order; but if you think there's an issue
>> with racing isolate_migratepages_block() or something like that, which
>> your current placement handles better, then please add a line of comment
>> both where you do it and where I expected to find it - thanks.
> 
> Sure. I will use your patch unless I find some racing issue.
> 
>>
>> (Historically, there was quite a lot of difficulty in getting the order
>> of events in __split_huge_page_tail() to be safe: I wonder whether we
>> shall see a crop of new weird bugs from these changes. I note that your
>> loops advance forwards, whereas the old ones went backwards: but I don't
>> have anything to say you're wrong.  I think it's mainly a matter of how
>> the first tail or two gets handled: which might be why you want to
>> folio_set_order(folio, new_order) at the earliest opportunity.)
> 
> I am worried about that too. In addition, in __split_huge_page_tail(),
> page refcount is restored right after new tail folio split is done,
> whereas I needed to delay them until all new after-split folios
> are done, since non-uniform split is iterative and only the after-split
> folios NOT containing the split_at page will be released. These
> folios are locked and frozen after __split_folio_to_order() like
> the original folio. Maybe because there are more such locked frozen
> folios than before?

What's the general concern here?

A frozen folio cannot be referenced and consequently not trusted. For 
example, if we want to speculatively lookup a folio in the pagecache and 
find it to be frozen, we'll have to spin (retry) until we find a folio 
that is unfrozen.

While a folio has a refcount of 0, there are no guarantees. It could 
change its size, it could be freed + reallocated (changed mapping etc) ...

So whoever wants to grab a speculative reference -- using 
folio_try_get() -- must re-verify folio properties after grabbing the 
speculative reference succeeded. Including whether it is small/large, 
number of pages, mapping, ...

The important part is to unfreeze a folio only once it was fully 
prepared (e.g., order set, compound pages links to head set up etc).

I am not sure if the sequence in which you process folios during a split 
matters here when doing a split: only that, whatever new folio  is 
unfrozen, is properly initialized.

-- 
Cheers,

David / dhildenb