mirror of
https://github.com/torvalds/linux.git
synced 2024-11-29 15:41:36 +00:00
02b670c1f8
The syzbot-reported stack trace from hell in this discussion thread
actually has three nested page faults:
https://lore.kernel.org/r/000000000000d5f4fc0616e816d4@google.com
... and I think that's actually the important thing here:
- the first page fault is from user space, and triggers the vsyscall
emulation.
- the second page fault is from __do_sys_gettimeofday(), and that should
just have caused the exception that then sets the return value to
-EFAULT
- the third nested page fault is due to _raw_spin_unlock_irqrestore() ->
preempt_schedule() -> trace_sched_switch(), which then causes a BPF
trace program to run, which does that bpf_probe_read_compat(), which
causes that page fault under pagefault_disable().
It's quite the nasty backtrace, and there's a lot going on.
The problem is literally the vsyscall emulation, which sets
current->thread.sig_on_uaccess_err = 1;
and that causes the fixup_exception() code to send the signal *despite* the
exception being caught.
And I think that is in fact completely bogus. It's completely bogus
exactly because it sends that signal even when it *shouldn't* be sent -
like for the BPF user mode trace gathering.
In other words, I think the whole "sig_on_uaccess_err" thing is entirely
broken, because it makes any nested page-faults do all the wrong things.
Now, arguably, I don't think anybody should enable vsyscall emulation any
more, but this test case clearly does.
I think we should just make the "send SIGSEGV" be something that the
vsyscall emulation does on its own, not this broken per-thread state for
something that isn't actually per thread.
The x86 page fault code actually tried to deal with the "incorrect nesting"
by having that:
if (in_interrupt())
return;
which ignores the sig_on_uaccess_err case when it happens in interrupts,
but as shown by this example, these nested page faults do not need to be
about interrupts at all.
IOW, I think the only right thing is to remove that horrendously broken
code.
The attached patch looks like the ObviouslyCorrect(tm) thing to do.
NOTE! This broken code goes back to this commit in 2011:
|
||
|---|---|---|
| .. | ||
| pat | ||
| amdtopology.c | ||
| cpu_entry_area.c | ||
| debug_pagetables.c | ||
| dump_pagetables.c | ||
| extable.c | ||
| fault.c | ||
| highmem_32.c | ||
| hugetlbpage.c | ||
| ident_map.c | ||
| init_32.c | ||
| init_64.c | ||
| init.c | ||
| iomap_32.c | ||
| ioremap.c | ||
| kasan_init_64.c | ||
| kaslr.c | ||
| kmmio.c | ||
| kmsan_shadow.c | ||
| maccess.c | ||
| Makefile | ||
| mem_encrypt_amd.c | ||
| mem_encrypt_boot.S | ||
| mem_encrypt_identity.c | ||
| mem_encrypt.c | ||
| mm_internal.h | ||
| mmap.c | ||
| mmio-mod.c | ||
| numa_32.c | ||
| numa_64.c | ||
| numa_emulation.c | ||
| numa_internal.h | ||
| numa.c | ||
| pf_in.c | ||
| pf_in.h | ||
| pgprot.c | ||
| pgtable_32.c | ||
| pgtable.c | ||
| physaddr.c | ||
| physaddr.h | ||
| pkeys.c | ||
| pti.c | ||
| srat.c | ||
| testmmiotrace.c | ||
| tlb.c | ||