Re: Perf: WARNING: arch/x86/entry/common.c:624 idtentry_exit_cond_rcu+0x92/0xc0

[Andy Lutomirski]

On Thu, Jul 2, 2020 at 8:03 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.