Re:Re: [PATCH] mm/huge_memory: avoid TOCTOU race in min_order_for_split()

At 2026-08-04 18:34:28, "Lorenzo Stoakes (ARM)" <[email protected]> wrote:
>On Tue, Aug 04, 2026 at 11:58:28AM +0800, [email protected] wrote:
>> From: liyouhong <[email protected]>
>>
>> min_order_for_split() reads folio->mapping twice without any
>
>Actually 3 times, since folio_test_anon() also references folio->mapping.
>
>But in reality, it's once not twice or 3 times, as the compiler just folds
>it all.
>
>So I don't think there's a race here at all with a NULL pointer deref?
>
>	<hoist folio->mapping to register>
>
>	if (folio_test_anon(folio)) -> inlined -> folio->mapping & FOLIO_MAPPING_ANON [ safe no null deref anyway ]
>		return 0;
>
>	/*
>	 * If the folio got truncated, we don't know the previous mapping and
>	 * consequently the old min order. But it doesn't matter, as any split
>	 * attempt will immediately fail with -EBUSY as the folio cannot get
>	 * split until freed.
>	 */
>	if (!folio->mapping) -> hoisted folio->mapping value checked
>		return 0;
>
>	return mapping_min_folio_order(folio->mapping); -> 	mapping->flags & AS_FOLIO_ORDER_MIN_MASK >> AS_FOLIO_ORDER_MIN;
>
>You do however have an issue potentially with a torn read I think?
>
>> synchronization. Concurrent truncate or invalidate can clear
>> folio->mapping between the check and subsequent function call.
>
>I mean David gets at this, but can this actually happen where this is
>called from?
>
>This is a local function.
>
>> Even with a held folio reference preventing the folio from
>> being freed, folio->mapping can still be overwritten to NULL. This
>> TOCTOU race allows passing a NULL mapping into mapping_min_folio_order(),
>> which leads to a NULL pointer dereference.
>
>Theoretical. You're not reporting a bug, you're reporting AI output. Be
>clear about that please.
>
>>
>> Cache folio->mapping to a local variable using READ_ONCE() to guarantee a
>> single memory load and remove the race window.
>
>It's already cached, and it doesn't guarantee a single memory load since
>your code as-is loads it twice via folio_test_anon().
>
>You see, this is workslopping here - you've not taken the time to carefully
>check this - and now David and I are having to point things out, and you're
>acting on behalf of an AI agent.
>
>Please in future _properly investigate the claim_. Do NOT assume Sashiko is
>right.
>
>This fix is not in the right place.
>
>>
>> Link: https://sashiko.dev/#/patchset/[email protected]
>
>Don't just take sashiko suggestions as reality. I've found it gives 50% noise,
>50% signal.
>
>Also doubly don't do this for 'this is not this patch but' stuff.
>
>It's worth investigating and _reproducing_ if possible, or even examining
>generated assembly.
>
>We already have enough work as it is without having to by-proxy check to
>see if Sashiko is hallucinating :)
>
>And in any case, go and _look at the callers_.  Having reviews have to do
>this for you is workslopping-by-proxy and isn't OK.
>
>> Signed-off-by: liyouhong <[email protected]>
>
>You're also missing Fixes & Cc: stable tags.
>
>> ---
>>  mm/huge_memory.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 2bccb0a53a0a..54d4261a2a15 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -4284,19 +4284,23 @@ int folio_split(struct folio *folio, unsigned int new_order,
>>   */
>>  unsigned int min_order_for_split(struct folio *folio)
>>  {
>> +	struct address_space *mapping;
>> +
>>  	if (folio_test_anon(folio))
>>  		return 0;
>
>Read folio->mapping.
>
>>
>> +	mapping = READ_ONCE(folio->mapping);
>
>Oh, read it again.
>
>This isn't great, and you can't hoist the mapping above because then you
>can't use folio_test_anon()...
>
>That should have been a clue that maybe this wasn't the right place to
>'fix' this 'bug' :)
>
>> +
>>  	/*
>>  	 * If the folio got truncated, we don't know the previous mapping and
>>  	 * consequently the old min order. But it doesn't matter, as any split
>>  	 * attempt will immediately fail with -EBUSY as the folio cannot get
>>  	 * split until freed.
>>  	 */
>> -	if (!folio->mapping)
>> +	if (!mapping)
>>  		return 0;
>>
>> -	return mapping_min_folio_order(folio->mapping);
>> +	return mapping_min_folio_order(mapping);
>>  }
>>
>>  int split_folio_to_list(struct folio *folio, struct list_head *list)
>> --
>> 2.25.1
>>
>
>The callers are memory_failure() and
>soft_offline_in_use_page(). memory_failure() drops a folio lock before
>doing this (!). soft_offline_in_use_page() seems to do this without even
>bothering with a folio lock.
>
>So as David said, the bug's there and needs proper investigation. This
>patch ain't it.
>
>P.S. I really think we need a way to disable this 'not an issue in your
>series but' stuff from sashiko, it's a huge distraction. If sashiko wants
>to do passive monitoring of existing issues, it should do it properly and
>separately, not adding more to unmanageable reviewer workload.
>

Thanks for the detailed analysis. You and David are both correct — the
compiler folds the reads so there's no real TOCTOU race inside
min_order_for_split() itself. The actual problem is the callers in
memory-failure.c not holding the folio lock. I'll send a v2 with:

1. VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio) in
   min_order_for_split().
2. Taking the folio lock around the min_order_for_split() calls in both
   memory_failure() and soft_offline_in_use_page().

Appreciate the thorough review, and apologies for the noise from the
initial AI-generated report — I'll make sure to properly investigate
before posting in the future.

Cheers,
liyouhong