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

[Kairui Song]

On Sat, Aug 15, 2026 at 10:37 AM Zi Yan <ziy@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx/ [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.

Thanks! Let me just open code it for now. It's like just...  <10 lines
of code duplication after this series? I think that is acceptable to
make the code cleaner and lower stack usage.