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

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 <[email protected]>

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/[email protected]/ [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.

Or does anyone like this (copied from hlist_for_each_safe)?

   #define for_each_folio_safe(start, end, sub_folio, next) \
      for (sub_folio = (start); \
           sub_folio != (end) && ({ next = folio_next(sub_folio); 1; }); \
           sub_folio = next)