Skip to content

Commit

Permalink
mm: document warning in move_normal_pmd() and make it warn only once
Browse files Browse the repository at this point in the history
Naresh Kamboju reported that the LTP tests can cause warnings on i386
going back all the way to v5.0, and bisected it to commit 2c91bd4
("mm: speed up mremap by 20x on large regions").

The warning in move_normal_pmd() is actually mostly correct, but we have
a very unusual special case at process creation time, when we may move
the stack down with an overlapping mode (kind of like a "memmove()"
except using the page tables).

And when you have just the right condition of "move a large initial
stack by the right alignment in the end, but with the early part of the
move being only page-aligned", we'll be in a situation where we're
trying to move a normal PMD entry on top of an already existing - but
now empty - PMD entry.

The warning is still worth having, in case it ever triggers other cases,
and perhaps as a reminder that we could do the stack move case more
efficiently (although it's clearly rare enough that it probably doesn't
matter).

But make it do WARN_ON_ONCE(), so that you can't flood the logs with it.

And add a *big* comment above it to explain and remind us what's going
on, because it took some figuring out to see how this could trigger.
Kudos to Joel Fernandes for debugging this.

Reported-by: Naresh Kamboju <[email protected]>
Debugged-and-acked-by: Joel Fernandes <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
  • Loading branch information
torvalds committed Jul 13, 2020
1 parent 11ba468 commit f81fdd0
Showing 1 changed file with 21 additions and 2 deletions.
23 changes: 21 additions & 2 deletions mm/mremap.c
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,28 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,

/*
* The destination pmd shouldn't be established, free_pgtables()
* should have release it.
* should have released it.
*
* However, there's a case during execve() where we use mremap
* to move the initial stack, and in that case the target area
* may overlap the source area (always moving down).
*
* If everything is PMD-aligned, that works fine, as moving
* each pmd down will clear the source pmd. But if we first
* have a few 4kB-only pages that get moved down, and then
* hit the "now the rest is PMD-aligned, let's do everything
* one pmd at a time", we will still have the old (now empty
* of any 4kB pages, but still there) PMD in the page table
* tree.
*
* Warn on it once - because we really should try to figure
* out how to do this better - but then say "I won't move
* this pmd".
*
* One alternative might be to just unmap the target pmd at
* this point, and verify that it really is empty. We'll see.
*/
if (WARN_ON(!pmd_none(*new_pmd)))
if (WARN_ON_ONCE(!pmd_none(*new_pmd)))
return false;

/*
Expand Down

0 comments on commit f81fdd0

Please sign in to comment.