Date: Sun, 28 Apr 2024 18:33:41 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Hillf Danton <hdanton@...a.com>, Andy Lutomirski <luto@...capital.net>, Peter Anvin <hpa@...or.com>, 
	Ingo Molnar <mingo@...nel.org>, Adrian Bunk <bunk@...nel.org>
Cc: syzbot <syzbot+83e7f982ca045ab4405c@...kaller.appspotmail.com>, 
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, andrii@...nel.org, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [bpf?] [trace?] possible deadlock in force_sig_info_to_task

On Sun, 28 Apr 2024 at 17:50, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
>    But the immediate problem is
> not the user space access, it's that something goes horribly wrong
> *around* it.

Side note: that stack trace from hell actually has three nested page
faults, 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 that 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.

And I think I finally see what may be 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 is ENTIRELY UNTESTED, but looks like the
ObviouslyCorrect(tm) thing to do.

NOTE! This broken code goes back to commit 4fc3490114bb ("x86-64: Set
siginfo and context on vsyscall emulation faults") in 2011, and back
then the reason was to get all the siginfo details right. Honestly, I
do not for a moment believe that it's worth getting the siginfo
details right here, but part of the commit says

    This fixes issues with UML when vsyscall=emulate.

and so my patch to remove this garbage will probably break UML in this
situation.

I cannot find it in myself to care, since I do not believe that
anybody should be running with vsyscall=emulate in 2024 in the first
place, much less if you are doing things like UML. But let's see if
somebody screams.

Also, somebody should obviously test my COMPLETELY UNTESTED patch.

Did I make it clear enough that this is UNTESTED and just does
crapectgomy on something that is clearly broken?

           Linus "UNTESTED" Torvalds

View attachment "patch.diff" of type "text/x-patch" (4002 bytes)

Powered by blists - more mailing lists