On Thu, Jul 2, 2020 at 8:03 AM Peter Zijlstra <pet...@infradead.org> wrote: > > On Thu, Jul 02, 2020 at 07:57:54PM +0530, Naresh Kamboju wrote: > > I have reported this warning on linux-next and now it is happening on > > linux mainline tree. > > May I know , are we missing a fix patch on linus 's tree ? > > > > - Naresh > > --- > > While running selftest x86 single_step_syscall_32 on > > i386 kernel linux 5.8.0-rc3 kernel warning noticed. > > > > steps to reproduce: > > -------------------------- > > perf test > > and > > cd /opt/kselftests/default-in-kernel/x86 > > ./single_step_syscall_32 > > > > crash dump, > > [ 1324.774385] kselftest: Running tests in x86 > > [ 1324.830187] ------------[ cut here ]------------ > > [ 1324.834820] IRQs not disabled as expected > > [ 1324.838838] WARNING: CPU: 2 PID: 5365 at > > /usr/src/kernel/arch/x86/entry/common.c:645 > > idtentry_exit_cond_rcu+0x92/0xc0 > > How's this? > > DEFINE_IDTENTRY_DEBUG() is DEFINE_IDTENTRY_RAW on x86_64 > DEFINE_IDTENTRY on i386 > > calling exc_debug_*() from DEFINE_IDTENTRY() does a double layer of > idtentry_{enter,exit}*() functions. > > Also, handle_debug() is still a trainwreck, we should never get to > cond_local_irq_enable() when !user. > > Completely untested... > > --- > arch/x86/kernel/traps.c | 49 > +++++++++++++++++++++++-------------------------- > 1 file changed, 23 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index b0195771c932..47d6b46e1564 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -806,8 +806,17 @@ static void handle_debug(struct pt_regs *regs, unsigned > long dr6, bool user) > * If DR6 is zero, no point in trying to handle it. The kernel is > * not using INT1. > */ > - if (!user && !dr6) > - return; > + if (!user) { > + /* > + * Catch SYSENTER with TF set and clear DR_STEP. If this hit a > + * watchpoint at the same time then that will still be > handled. > + */ > + if ((dr6 & DR_STEP) && is_sysenter_singlestep(regs)) > + dr6 &= ~DR_STEP; > + > + if (!dr6) > + return; > + } > > /* > * If dr6 has no reason to give us about the origin of this trap, > @@ -859,25 +868,29 @@ static void handle_debug(struct pt_regs *regs, unsigned > long dr6, bool user) > cond_local_irq_disable(regs); > } > > +#ifdef CONFIG_X86_64 > static __always_inline void exc_debug_kernel(struct pt_regs *regs, > unsigned long dr6) > { > bool irq_state = idtentry_enter_nmi(regs); > instrumentation_begin(); > > - /* > - * Catch SYSENTER with TF set and clear DR_STEP. If this hit a > - * watchpoint at the same time then that will still be handled. > - */ > - if ((dr6 & DR_STEP) && is_sysenter_singlestep(regs)) > - dr6 &= ~DR_STEP; > - > handle_debug(regs, dr6, false); > > instrumentation_end(); > idtentry_exit_nmi(regs, irq_state); > } > > +/* IST stack entry */ > +DEFINE_IDTENTRY_DEBUG(exc_debug) > +{ > + unsigned long dr6, dr7; > + > + debug_enter(&dr6, &dr7); > + exc_debug_kernel(regs, dr6); > + debug_exit(dr7); > +} > + > static __always_inline void exc_debug_user(struct pt_regs *regs, > unsigned long dr6) > { > @@ -890,17 +903,6 @@ static __always_inline void exc_debug_user(struct > pt_regs *regs, > idtentry_exit_user(regs); > } > > -#ifdef CONFIG_X86_64 > -/* IST stack entry */ > -DEFINE_IDTENTRY_DEBUG(exc_debug) > -{ > - unsigned long dr6, dr7; > - > - debug_enter(&dr6, &dr7); > - exc_debug_kernel(regs, dr6); > - debug_exit(dr7); > -} > - > /* User entry, runs on regular task stack */ > DEFINE_IDTENTRY_DEBUG_USER(exc_debug) > { > @@ -917,12 +919,7 @@ DEFINE_IDTENTRY_DEBUG(exc_debug) > unsigned long dr6, dr7; > > debug_enter(&dr6, &dr7); > - > - if (user_mode(regs)) > - exc_debug_user(regs, dr6); > - else > - exc_debug_kernel(regs, dr6); > - > + handle_debug(regs, dr6, user_mode(regs)); > debug_exit(dr7); > } > #endif
I'll fold something like this into my pile of pending fixes.