On Thu 2019-10-03 21:56:34, Will Deacon wrote:
> Hi Kees,
>
> On Wed, Oct 02, 2019 at 01:58:46PM -0700, Kees Cook wrote:
> > On Wed, Oct 02, 2019 at 01:35:38PM +0100, Will Deacon wrote:
> > > Calling 'panic()' on a kernel with CONFIG_PREEMPT=y can leave the
> > > calling CPU in an infinite loop, but with interrupts and preemption
> > > enabled. From this state, userspace can continue to be scheduled,
> > > despite the system being "dead" as far as the kernel is concerned. This
> > > is easily reproducible on arm64 when booting with "nosmp" on the command
> > > line; a couple of shell scripts print out a periodic "Ping" message
> > > whilst another triggers a crash by writing to /proc/sysrq-trigger:
> > >
> > > | sysrq: Trigger a crash
> > > | Kernel panic - not syncing: sysrq triggered crash
> > > | CPU: 0 PID: 1 Comm: init Not tainted 5.2.15 #1
> > > | Hardware name: linux,dummy-virt (DT)
> > > | Call trace:
> > > | dump_backtrace+0x0/0x148
> > > | show_stack+0x14/0x20
> > > | dump_stack+0xa0/0xc4
> > > | panic+0x140/0x32c
> > > | sysrq_handle_reboot+0x0/0x20
> > > | __handle_sysrq+0x124/0x190
> > > | write_sysrq_trigger+0x64/0x88
> > > | proc_reg_write+0x60/0xa8
> > > | __vfs_write+0x18/0x40
> > > | vfs_write+0xa4/0x1b8
> > > | ksys_write+0x64/0xf0
> > > | __arm64_sys_write+0x14/0x20
> > > | el0_svc_common.constprop.0+0xb0/0x168
> > > | el0_svc_handler+0x28/0x78
> > > | el0_svc+0x8/0xc
> > > | Kernel Offset: disabled
> > > | CPU features: 0x0002,24002004
> > > | Memory Limit: none
> > > | ---[ end Kernel panic - not syncing: sysrq triggered crash ]---
> > > | Ping 2!
> > > | Ping 1!
> > > | Ping 1!
> > > | Ping 2!
> > >
> > > The issue can also be triggered on x86 kernels if CONFIG_SMP=n, otherwise
> > > local interrupts are disabled in 'smp_send_stop()'.
> > >
> > > Disable preemption in 'panic()' before re-enabling interrupts.
> >
> > Is this perhaps the correct solution for what commit c39ea0b9dd24 ("panic:
> > avoid the extra noise dmesg") was trying to fix?
>
> Hmm, maybe, although that looks like it's focussed more on irq handling
> than preemption.
Exactly, the backtrace mentioned in commit c39ea0b9dd24 ("panic: avoid
the extra noise dmesg") is printed by wake_up() called from
wake_up_klogd_work_func(). It is irq_work. Therefore disabling
preemption would not prevent this.
> I've deliberately left the irq part alone, since I think
> having magic sysrq work via the keyboard interrupt is desirable from the
> panic loop.
I agree that we should keep sysrq working.
One pity thing is that led_panic_blink() in
leds/drivers/trigger/ledtrig-panic.c uses workqueues:
+ led_panic_blink()
+ led_trigger_event()
+ led_set_brightness()
+ schedule_work()
It means that it depends on the scheduler. I guess that it
does not work in many panic situations. But this patch
will always block it.
I agree that it is strange that userspace still works at
this stage. But does it cause any real problems?
Best Regards,
Petr
