linux-kernel - Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if source was there.

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]

Date: Tue, 12 Mar 2024 11:51:13 -0400
From: Zi Yan <ziy@...dia.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 Yang Shi <shy828301@...il.com>, Huang Ying <ying.huang@...el.com>,
 "\"Kirill A . Shutemov\"" <kirill.shutemov@...ux.intel.com>,
 Ryan Roberts <ryan.roberts@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if
 source was there.

On 12 Mar 2024, at 10:19, Matthew Wilcox wrote:

> On Tue, Mar 12, 2024 at 10:13:16AM -0400, Zi Yan wrote:
>> On 11 Mar 2024, at 23:45, Matthew Wilcox wrote:
>>> Much more important: You're doing this with a positive refcount, which
>>> breaks the (undocumented) logic in deferred_split_scan() that a folio
>>> with a positive refcount will not be removed from the list.
>>
>> What is the issue here? I thought as long as the split_queue_lock is held,
>> it should be OK to manipulate the list.
>
> I just worked this out yesterday:
> https://lore.kernel.org/linux-mm/Ze9EFdFLXQEUVtKl@casper.infradead.org/
> (the last chunk, starting with Ryan asking me "what about the first bug
> you found")

Hmm, like you said a folio with a positive refcount will not be removed
from ds_queue->split_queue, it will have no chance going to the separate
list in deferred_list_scan() and list_del_init() will not corrupt
that list. So it should be safe. Or the issue is that before migration
adding a refcount, the folio is removed from ds_queue->split_queue
and put on the list in deferred_list_scan(), as a result, any manipulation
of folio->_deferred_list could corrupt the list. Basically,
!list_empty(&folio->_deferred_list) cannot tell if the folio is on
ds_queue->split_queue or another list. I am not sure about why "a positive
refcount" is related here.

That makes me wonder whether ds_queue->split_queue_lock is also needed
for list_for_each_entry_safe() in deferred_split_scan(). Basically,
ds_queue->split_queue_lock protects folio->_deferred_list in addition to
ds_queue->split_queue.

>>> Maximally important: Wer shouldn't be doing any of this!  This folio is
>>> on the deferred split list.  We shouldn't be migrating it as a single
>>> entity; we should be splitting it now that we're in a context where we
>>> can do the right thing and split it.  Documentation/mm/transhuge.rst
>>> is clear that we don't split it straight away due to locking context.
>>> Splitting it on migration is clearly the right thing to do.
>>>
>>> If splitting fails, we should just fail the migration; splitting fails
>>> due to excess references, and if the source folio has excess references,
>>> then migration would fail too.
>>
>> You are suggesting:
>> 1. checking if the folio is on deferred split list or not
>> 2. if yes, split the folio
>> 3. if split fails, fail the migration as well.
>>
>> It sounds reasonable to me. The split folios should be migrated since
>> the before-split folio wants to be migrated. This split is not because
>> no new page cannot be allocated, thus the split folios should go
>> into ret_folios list instead of split_folios list.
>
> Yes, I'm happy for the split folios to be migrated.  Bonus points if you
> want to figure out what order to split the folio to ;-)  I don't think
> it's critical.

--
Best Regards,
Yan, Zi

Download attachment "signature.asc" of type "application/pgp-signature" (855 bytes)