linux-kernel - Re: BUG: using __this_cpu_read() in preemptible code in trace_hardirqs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201021103433.38fed220@gandalf.local.home>
Date:   Wed, 21 Oct 2020 10:34:33 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     syzbot <syzbot+53f8ce8bbc07924b6417@...kaller.appspotmail.com>,
        linux-kernel@...r.kernel.org, mingo@...hat.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: BUG: using __this_cpu_read() in preemptible code in
 trace_hardirqs_on

On Wed, 21 Oct 2020 15:17:33 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> > And I'm also guessing that we can call this with interrupts enabled (based
> > on the comment).
> > 
> > And we have this:
> > 
> >    local_irq_enable()
> >       trace_hardirqs_on()
> >          lockdep_hardirqs_on()
> >              __this_cpu_read()  
> 
> Moo, two threads..
> 
> 20201019183355.GS2611@...ez.programming.kicks-ass.net

But this one's much older ;-)

> 
> ---
> 
> On Tue, Oct 20, 2020 at 12:55:46AM +0800, kernel test robot wrote:
> > [   92.898145] BUG: using __this_cpu_read() in preemptible [00000000] code: trinity-c6/526  
> 
> > [   92.903305] Call Trace:
> > [   92.905182]  __this_cpu_preempt_check+0xf/0x11
> > [   92.905968]  lockdep_hardirqs_on_prepare+0x2c/0x18f
> > [   92.906853]  trace_hardirqs_on+0x49/0x53
> > [   92.907578]  __bad_area_nosemaphore+0x3a/0x134  
> 
> Hurph, that's a spurious local_irq_enable(). I suppose this'll fix it.
> 
> ---
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 3e99dfef8408..9f818145ef7d 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4057,9 +4057,6 @@ void lockdep_hardirqs_on_prepare(unsigned long ip)
>  	if (unlikely(in_nmi()))
>  		return;
>  
> -	if (unlikely(__this_cpu_read(lockdep_recursion)))
> -		return;
> -
>  	if (unlikely(lockdep_hardirqs_enabled())) {

Hmm, would moving the recursion check below the check of the
lockdep_hardirqs_enable() cause a large skew in the spurious enable stats?
May not be an issue, but something we should check to make sure that
there's not a path that constantly hits this.

-- Steve

>  		/*
>  		 * Neither irq nor preemption are disabled here
> @@ -4070,6 +4067,9 @@ void lockdep_hardirqs_on_prepare(unsigned long ip)
>  		return;
>  	}
>  
> +	if (unlikely(__this_cpu_read(lockdep_recursion)))
> +		return;
> +