Re: [PATCH] mm/pagewalk: fix stale walk->action escaping walk_pmd_range()

[Hyunwoo Kim <imv4bel@gmail.com>]

On Mon, Aug 10, 2026 at 12:19:54PM +0100, Lorenzo Stoakes (ARM) wrote:
> Since it's 2026 + this is your first patch in mm AFAICT, 

Yeah, my home town is netdev :)

> and it's also a
> very fiddly and specific issue, I do have to ask - was there was any AI
> involved in making this?
> 
> If so you should add an Assisted-by: tag as per
> https://docs.kernel.org/process/coding-assistants.html please :)
> 
> On Mon, Aug 10, 2026 at 06:45:17PM +0900, Hyunwoo Kim wrote:
> > walk_pmd_range() resets walk->action in only one place in its loop body,
> > and that place is after the pmd_none() branch. For a walker with no
> 
> Newline after full stop please, this is too many words in one big block.
> 
> What is a 'reset'? This doesn't really mean anything. You mean sets
> walk->action = ACTION_SUBTREE, which is the default action.
> 
> Your commit message doesn't mention ACTION_SUBTREE anywhere, it should.
> 
> Also what you're saying here is just untrue if you consider reset to be
> assignment to walk->action (which is a reasonable interpretation) -
> walk_pmd_range() changes walk->action in _two_ places.
> 
> Just be clear that you by reset you mean assigning walk->action =
> ACTION_SUBTREE.
> 
> > ->install_pte, that branch continues to the next entry without passing the
> > reset. So if ->pmd_entry() sets ACTION_AGAIN and returns 0, and the PMD has
> 
> Not passing what reset? This is so unclear.
> 
> > become none by the time the loop restarts at the again label, the reset is
> > skipped. If the remaining entries are all none too, the loop returns 0 with
> > ACTION_AGAIN still set. The ACTION_AGAIN that walk_pte_range() sets when
> > pte_offset_map_lock() fails escapes the same way.
> 
> OK so what you mean to say is:
> 
> 	if (ops->pmd_entry)
> 		err = ops->pmd_entry(pmd, addr, next, walk); <- 1. sets walk->action =
> 								   ACTION_AGAIN
> 		if (err)
> 			break;
> 
> 		if (walk->action == ACTION_AGAIN)
> 			goto again;
> 
> Then above that code:
> 
> again:
> 		next = pmd_addr_end(addr, end);
> 		if (pmd_none(*pmd)) {   <- 2. This triggers because PMD became empty
> 			if (has_install)
> 				err = __pte_alloc(walk->mm, pmd);
> 			else if (ops->pte_hole)
> 				err = ops->pte_hole(addr, next, depth, walk);
> 			if (err)
> 				break;
> 			if (!has_install)
> 				continue; <- 3. Loop around to the next entry, with
> 						walk->action erroneously set to
> 						ACTION_AGAIN still.
> 		}
> 
> 		walk->action = ACTION_SUBTREE; <- 4. This would reset it EXCEPT if
> 		                                     you are at the end of the range.
> 
> Then in walk_pud_range():
> 
> 		err = walk_pmd_range(pud, addr, next, walk);
> 		if (err)
> 			break;
> 
> 		if (walk->action == ACTION_AGAIN) <- 5. Uh oh doing a retry for no
> 		                                        reason.
> 			goto again;
> 
> So it's only if you're at the end of the PMD range that it's a problem
> right? 

Yes, or more precisely when nothing after it sets walk->action = ACTION_SUBTREE.

> You should say that.
> 
> The user-visible problem here is that you can end up calling the callbacks
> too many times which explicitly breaks mincore.
> 
> Given I had to decode this it does make me wonder if you fully understand
> this or if it's just unclear language. This sort of word salad is very
> LLM-ish :)

The commit message was written with help from the lovely Claude. Not the
most readable, admittedly :)

> 
> Anyway overall maybe rewrite the commit message to something like:
> 
> 	If ->pmd_entry() sets walk->action = ACTION_AGAIN, the pmd_none()
> 	check is retried. The PMD entry may be cleared at the point of retry.
> 
> 	In this case, if walk->ops->install_pte is not specified, the code
> 	continues to the next PMD entry in the range without resetting
> 	walk->action to ACTION_SUBTREE.
> 
> 	This leaves walk->action erroneously set to ACTION_AGAIN, which is
> 	incorrect.
> 
> 	This was incorrect but not problematic up until commit 3b89863c3fa4
> 	("mm/pagewalk: fix race between concurrent split and refault")
> 	which updated walk_pud_range() to check for walk->action ==
> 	ACTION_AGAIN upon walk_pmd_range()'s return, causing the PUD walk
> 	to be retried.
> 
> 	In this case this results in duplicate walk callbacks being
> 	invoked, which is erroneous and will break any caller that is not
> 	idempotent with respect to this (and waste time for those which
> 	are).
> 
> 	A specific example of this breaking things is mincore which walks
> 	an internal cursor data structure a byte at a time on assumption
> 	that page table entry callbacks are called only once for each
> 	entry.
> 
> 	Fix the problem by resetting walk->action to ACTION_SUBTREE prior
> 	to the none check.
> 
> 	The pattern also exists in walk_pud_entry() so fix it there too.
> 
> 
> >
> > Nothing looked at that value after walk_pmd_range() returned until commit
> > 3b89863c3fa4 ("mm/pagewalk: fix race between concurrent split and refault")
> > turned that into a problem. It added both the PUD check that sets
> > ACTION_AGAIN before the loop is entered and the test in walk_pud_range()
> > that picks the value up right after walk_pmd_range() returns and walks
> > [addr, pud_addr_end(addr, end)) again. That is fine for the PUD check,
> > since none of walk_pmd_range()'s own callbacks have run at that point, but
> > a value that escaped as described above arrives after those callbacks have
> > already covered the range.
> >
> > For mincore(2) this becomes an out-of-bounds write. ->pmd_entry() and
> > ->pte_hole() advance the walk->private cursor by one byte per page, the
> > buffer is a single page from __get_free_page(), and mincore(2) asks for at
> > most PAGE_SIZE entries at a time, so there is no room to spare. Walking
> > the range a second time pushes the cursor past the end of the buffer, and
> > it does so again every time the race is hit. Reproducing this needs no
> > privileges: run mincore(2) over a 16 MiB anonymous mapping marked
> > MADV_NOHUGEPAGE while another thread repeatedly faults in a PMD-aligned
> > 2 MiB range inside it and then drops it with madvise(MADV_DONTNEED).
> 
> Obviouisly see above on commit message.
> 
> Is it possible to add a self test that does something like this or is it too
> racey to be practical?

It looks quite race dependent. Triggering it deterministically would need
some of the race window widening tricks from exploit work, which I don't
think belongs in a selftest. I can still write one if you'd like.

> 
> >
> > Move the reset to the first statement of the loop body. walk_pud_range()
> > has the same shape and gets the same change; walk_p4d_range() never looks
> > at walk->action, so that hunk keeps the two functions in sync rather than
> > fixing a second bug.
> 
> Your commit message doesn't mention how you discovered this. If it was AI
> suggesting it (whether locally or sashiko in reply to some review) you
> should reference it.
> 
> If it was simply hardcore code inspection then you should say so too :)

It was found by accident while fuzzing a different subsystem. Either way,
the fuzzer itself was written by AI, so,

Assisted-by: Claude:claude-opus-5

> 
> >
> > Fixes: 3b89863c3fa4 ("mm/pagewalk: fix race between concurrent split and refault")
> > Cc: stable@vger.kernel.org
> 
> This all seems right.
> 
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> 
> The fix itself looks right, but you need to address the other feedback and
> respin (also please send a respin with at least 1 day's delay).

OK, will do.


Best regards,
Hyunwoo Kim