Skip to content

Commit

Permalink
gup: make the stack expansion warning a bit more targeted
Browse files Browse the repository at this point in the history
I added a warning about about GUP no longer expanding the stack in
commit a425ac5 ("gup: add warning if some caller would seem to want
stack expansion"), but didn't really expect anybody to hit it.

And it's true that nobody seems to have hit a _real_ case yet, but we
certainly have a number of reports of false positives.  Which not only
causes extra noise in itself, but might also end up hiding any real
cases if they do exist.

So let's tighten up the warning condition, and replace the simplistic

	vma = find_vma(mm, start);
	if (vma && (start < vma->vm_start)) {
		WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);

with a

	vma = gup_vma_lookup(mm, start);

helper function which works otherwise like just "vma_lookup()", but with
some heuristics for when to warn about gup no longer causing stack
expansion.

In particular, don't just warn for "below the stack", but warn if it's
_just_ below the stack (with "just below" arbitrarily defined as 64kB,
because why not?).  And rate-limit it to at most once per hour, which
means that any false positives shouldn't completely hide subsequent
reports, but we won't be flooding the logs about it either.

The previous code triggered when some GUP user (chromium crashpad)
accessing past the end of the previous vma, for example.  That has never
expanded the stack, it just causes GUP to return early, and as such we
shouldn't be warning about it.

This is still going trigger the randomized testers, but to mitigate the
noise from that, use "dump_stack()" instead of "WARN_ON_ONCE()" to get
the kernel call chain.  We'll get the relevant information, but syzbot
shouldn't get too upset about it.

Also, don't even bother with the GROWSUP case, which would be using
different heuristics entirely, but only happens on parisc.

Reported-by: kernel test robot <[email protected]>
Reported-by: John Hubbard <[email protected]>
Reported-by: [email protected]
Signed-off-by: Linus Torvalds <[email protected]>
  • Loading branch information
torvalds committed Jul 5, 2023
1 parent d528014 commit 6cd06ab
Showing 1 changed file with 41 additions and 10 deletions.
51 changes: 41 additions & 10 deletions mm/gup.c
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,45 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
return 0;
}

/*
* This is "vma_lookup()", but with a warning if we would have
* historically expanded the stack in the GUP code.
*/
static struct vm_area_struct *gup_vma_lookup(struct mm_struct *mm,
unsigned long addr)
{
#ifdef CONFIG_STACK_GROWSUP
return vma_lookup(mm, addr);
#else
static volatile unsigned long next_warn;
struct vm_area_struct *vma;
unsigned long now, next;

vma = find_vma(mm, addr);
if (!vma || (addr >= vma->vm_start))
return vma;

/* Only warn for half-way relevant accesses */
if (!(vma->vm_flags & VM_GROWSDOWN))
return NULL;
if (vma->vm_start - addr > 65536)
return NULL;

/* Let's not warn more than once an hour.. */
now = jiffies; next = next_warn;
if (next && time_before(now, next))
return NULL;
next_warn = now + 60*60*HZ;

/* Let people know things may have changed. */
pr_warn("GUP no longer grows the stack in %s (%d): %lx-%lx (%lx)\n",
current->comm, task_pid_nr(current),
vma->vm_start, vma->vm_end, addr);
dump_stack();
return NULL;
#endif
}

/**
* __get_user_pages() - pin user pages in memory
* @mm: mm_struct of target mm
Expand Down Expand Up @@ -1168,11 +1207,7 @@ static long __get_user_pages(struct mm_struct *mm,

/* first iteration or cross vma bound */
if (!vma || start >= vma->vm_end) {
vma = find_vma(mm, start);
if (vma && (start < vma->vm_start)) {
WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
vma = NULL;
}
vma = gup_vma_lookup(mm, start);
if (!vma && in_gate_area(mm, start)) {
ret = get_gate_page(mm, start & PAGE_MASK,
gup_flags, &vma,
Expand Down Expand Up @@ -1337,13 +1372,9 @@ int fixup_user_fault(struct mm_struct *mm,
fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;

retry:
vma = find_vma(mm, address);
vma = gup_vma_lookup(mm, address);
if (!vma)
return -EFAULT;
if (address < vma->vm_start ) {
WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
return -EFAULT;
}

if (!vma_permits_fault(vma, fault_flags))
return -EFAULT;
Expand Down

0 comments on commit 6cd06ab

Please sign in to comment.