Skip to content

Commit 6cd06ab

Browse files
committed
gup: make the stack expansion warning a bit more targeted
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]>
1 parent d528014 commit 6cd06ab

File tree

1 file changed

+41
-10
lines changed

1 file changed

+41
-10
lines changed

mm/gup.c

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,6 +1091,45 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
10911091
return 0;
10921092
}
10931093

1094+
/*
1095+
* This is "vma_lookup()", but with a warning if we would have
1096+
* historically expanded the stack in the GUP code.
1097+
*/
1098+
static struct vm_area_struct *gup_vma_lookup(struct mm_struct *mm,
1099+
unsigned long addr)
1100+
{
1101+
#ifdef CONFIG_STACK_GROWSUP
1102+
return vma_lookup(mm, addr);
1103+
#else
1104+
static volatile unsigned long next_warn;
1105+
struct vm_area_struct *vma;
1106+
unsigned long now, next;
1107+
1108+
vma = find_vma(mm, addr);
1109+
if (!vma || (addr >= vma->vm_start))
1110+
return vma;
1111+
1112+
/* Only warn for half-way relevant accesses */
1113+
if (!(vma->vm_flags & VM_GROWSDOWN))
1114+
return NULL;
1115+
if (vma->vm_start - addr > 65536)
1116+
return NULL;
1117+
1118+
/* Let's not warn more than once an hour.. */
1119+
now = jiffies; next = next_warn;
1120+
if (next && time_before(now, next))
1121+
return NULL;
1122+
next_warn = now + 60*60*HZ;
1123+
1124+
/* Let people know things may have changed. */
1125+
pr_warn("GUP no longer grows the stack in %s (%d): %lx-%lx (%lx)\n",
1126+
current->comm, task_pid_nr(current),
1127+
vma->vm_start, vma->vm_end, addr);
1128+
dump_stack();
1129+
return NULL;
1130+
#endif
1131+
}
1132+
10941133
/**
10951134
* __get_user_pages() - pin user pages in memory
10961135
* @mm: mm_struct of target mm
@@ -1168,11 +1207,7 @@ static long __get_user_pages(struct mm_struct *mm,
11681207

11691208
/* first iteration or cross vma bound */
11701209
if (!vma || start >= vma->vm_end) {
1171-
vma = find_vma(mm, start);
1172-
if (vma && (start < vma->vm_start)) {
1173-
WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
1174-
vma = NULL;
1175-
}
1210+
vma = gup_vma_lookup(mm, start);
11761211
if (!vma && in_gate_area(mm, start)) {
11771212
ret = get_gate_page(mm, start & PAGE_MASK,
11781213
gup_flags, &vma,
@@ -1337,13 +1372,9 @@ int fixup_user_fault(struct mm_struct *mm,
13371372
fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
13381373

13391374
retry:
1340-
vma = find_vma(mm, address);
1375+
vma = gup_vma_lookup(mm, address);
13411376
if (!vma)
13421377
return -EFAULT;
1343-
if (address < vma->vm_start ) {
1344-
WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
1345-
return -EFAULT;
1346-
}
13471378

13481379
if (!vma_permits_fault(vma, fault_flags))
13491380
return -EFAULT;

0 commit comments

Comments
 (0)