Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <87imfwd5f6.fsf@nanos.tec.linutronix.de> <8DD3180E-0E69-4FD6-92C3-311AAB3F688F@amacapital.net>
 <87d064d13p.fsf@nanos.tec.linutronix.de> <CA+G9fYvVEMVaF=qzhCpJ8NMb1-VN4cEh6sw8P_eNFCLQYOjCzA@mail.gmail.com>
 <20200702150250.GS4800@hirez.programming.kicks-ass.net>
In-Reply-To: <20200702150250.GS4800@hirez.programming.kicks-ass.net>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 2 Jul 2020 11:21:01 -0700
Message-ID: <CALCETrVM8YNdssYF38hB=Sg3BgwiFaaBYfz3AeBqVYP3biucLg@mail.gmail.com>
Subject: Re: Perf: WARNING: arch/x86/entry/common.c:624 idtentry_exit_cond_rcu+0x92/0xc0
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Naresh Kamboju <naresh.kamboju@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andy Lutomirski <luto@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        X86 ML <x86@kernel.org>, cj.chengjian@huawei.com,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
        Minchan Kim <minchan@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Michel Lespinasse <walken@google.com>,
        lkft-triage@lists.linaro.org,
        Dave Hansen <dave.hansen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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