Re: [PATCH v2 04/17] mm/huge_memory: split the routine for splitting anon and file folio

["Zi Yan" <ziy@nvidia.com>]

On Thu Aug 13, 2026 at 2:11 PM EDT, Kairui Song wrote:
> Sashiko reports (a valid case this time):
>
> --- Patch [4]: [PATCH v2 04/17] mm/huge_memory: split the routine for
> splitting anon and file folio ---
> commit 249f031b2182770a8cf754edfb3833e148a033df
> Author: Kairui Song <kasong@tencent.com>
>
> mm/huge_memory: split the routine for splitting anon and file folio
>
> No functional change intended. Before adding more logic, split
> __folio_freeze_and_split_unmapped() into an anon and a file variant so
> each path can evolve independently. The two paths shared little beyond
> the folio freeze call, the LRU locking, and the unfreeze skeleton, but
> differed in all other per-folio bookkeeping and routines.
>
> While splitting, some cleanups become easy to apply, and helped drop a
> few now-redundant checks. Also introduce a folio iteration helper to
> avoid a common pitfall of iterating post-split sub-folios: a sub folio
> might get freed mid-iteration as pointed out by Zi [1].
>
> Link: https://lore.kernel.org/linux-mm/DKJSFCLP967N.YBR4DNK1NM2N@nvidia.com/ [1]
>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 7fb603ac500f6..7587eeb09e4a6 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>
> [ ... ]
>
>> @@ -3634,6 +3634,18 @@ static bool page_range_has_hwpoisoned(struct page *page, long nr_pages)
>>   return false;
>>  }
>>
>> +/**
>> + * for_each_folio_safe - iterate over contiguous folios safe against folio free
>> + * @start: the first folio to iterate
>> + * @end: sentinel, folio_next() of the last folio to iterate
>> + * @sub_folio: struct folio * to use as the loop cursor
>> + * @next: struct folio * used as temporary storage
>> + */
>> +#define for_each_folio_safe(start, end, sub_folio, next) \
>> + for (sub_folio = (start), next = folio_next(sub_folio); \
>> +     sub_folio != (end); \
>> +     sub_folio = next, next = folio_next(next))
>
> Does this macro unconditionally dereference the sentinel pointer?
>
> When the loop increment step executes, it evaluates both sub_folio = next
> and next = folio_next(next). If next has reached the end sentinel, calling
> folio_next(next) will dereference the sentinel pointer to read its metadata.
>
> If the split large folio happens to be located at the end of a populated
> memory section, the sentinel pointer (end) could point to the next section's
> vmemmap. If that next section is unpopulated, this out-of-bounds read will
> trigger a fatal kernel page fault.
>
> ---
>
> Right, I actually hesitated on that but forgot about it midway. I
> think I'll just change back to the opencoded style, it's easier to
> understand that way.

Let's open code it. I also tried a similar macro when I was developing
folio_split(), but got convinced it buys nothing.

If the motivation comes my prior comment on code duplication, feel free
to ignore that. If we over engineer it, like different function pointers
for anon/swapcache and file, it might be doable. But that might kill
readability.

What I mean is something like:

__folio_freeze_and_split_unmapped(func_t pre_freeze_func, func_t
unfreeze_func, func_t post_freeze_func)
{
    pre_freeze_func();

    for () {
        unfreeze_func();
    }

    post_freeze_func();
}

for anon/swapcache:

__folio_freeze_and_split_unmapped(anon_swapcache_pre_freeze,
anon_swapcache_unfreeze, anon_swapcache_post_freeze);

for file:

__folio_freeze_and_split_unmapped(file_pre_freeze, file_unfreeze,
file_post_freeze);


-- 
Best Regards,
Yan, Zi