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

Date: Wed, 13 Mar 2024 10:33:54 +0800
From: "Yin, Fengwei" <fengwei.yin@...el.com>
To: Matthew Wilcox <willy@...radead.org>, Zi Yan <ziy@...dia.com>
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 3/13/2024 10:07 AM, Yin, Fengwei wrote:
> 
> 
> On 3/13/2024 2:46 AM, Matthew Wilcox wrote:
>> On Tue, Mar 12, 2024 at 02:32:43PM -0400, Zi Yan wrote:
>>> On 12 Mar 2024, at 12:38, Matthew Wilcox wrote:
>>>> Folios with a positive refcount are
>>>> removed from the per-node or per-cgroup list _at which point there is
>>>> an undocumented assumption_ that they will not be removed from the
>>>> local list because they have a positive refcount.
>>>
>>> But that sounds very subtle if not broken. As an outsider of
>>
>> I merely deduced this requirement; I didn't come up with it ...
> My understanding is that this requirement is because of just local
> list in deferred_split_scan().
> 
> Using fbatch instead of local list here as your created for that
> issue debugging can eliminate this subtlety?
May not good idea because it's possible the folios in fbatch can
be removed from deferred_list by migration.

> 
> 
> Regards
> Yin, Fengwei
> 
>>
>>> deferred_split_scan(), only !list_empty(folio->_deferred_list) is 
>>> checked.
>>> The condition can be true if the folio is on split_queue or
>>> local list of deferred_split_scan() with elevated refcount. In that 
>>> case,
>>> the folio cannot be removed from the list (either split_queue or 
>>> local list)
>>> even if split_queue_lock is held, since local list manipulation is 
>>> not under
>>> split_queue_lock. This makes _deferred_list a one-way train to anyone
>>> except deferred_split_scan(), namely folios can only be added into
>>> _deferred_list until they are freed or split by deferred_split_scan().
>>>
>>> Is that intended? If yes, maybe we should document it. If not, using
>>> split_queue_lock to protect local list, or more explicitly 
>>> folio->_deferred_list
>>> might be better?
>>
>> To be fair, the folio can be split by anybody as
>> split_huge_page_to_list_to_order() is careful to only manipulate the
>> deferred list while the refcount is frozen at 0.  I'm still trying to
>> figure out where to document this behaviour of the deferred list that
>> someone (for example, your good self) would actually see it.
>>
>>
>>
>