linux-kernel - Re: [PATCH 4/4] tracing: Remove RCU work arounds from stack tracer

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]

Date:   Fri, 22 Sep 2017 15:54:55 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, ngo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        stable@...r.kernel.org
Subject: Re: [PATCH 4/4] tracing: Remove RCU work arounds from stack tracer

On Fri, Sep 22, 2017 at 06:15:47PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
> 
> While debugging some RCU issues with the stack tracer, it was discovered
> that the problem was much more than with the stack tracer itself, but with
> the saving of the stack trace, which could happen from any WARN() as well.
> The problem was fixed within kernel_text_address().
> 
> One of the bugs that was discovered was that the stack tracer called
> rcu_enter_irq() unconditionally. Paul McKenney said that could cause issues
> as well. Instead of adding logic to only call rcu_enter_irq() if RCU is not
> watching from within the stack tracer, since the core issue has been fixed
> (within save_stack_trace()), we can simply remove all the logic in the stack
> tracer that deals with RCU work arounds.

I must confess that I am having some difficulty parsing this paragraph,
especially the last sentence...

Does this capture it?

	One problem is that the stack tracer called rcu_irq_enter()
	unconditionally, which is problematic if RCU's last
	not-watching-to-watching transition was carried out by
	rcu_nmi_enter.	In that case, rcu_irq_enter() actually switches
	RCU back to the not-watching state for this CPU, which results
	in lockdep splats complaining about rcu_read_lock() being
	used on an idle (not-watched) CPU.  The first patch of this
	series addressed this problem by having rcu_irq_enter() and
	rcu_irq_exit() refrain from doing anything when rcu_nmi_enter()
	caused RCU to start watching this CPU.	The third patch in this
	series caused save_stack_trace() to invoke rcu_nmi_enter() and
	rcu_nmi_exit() as needed, so this fourth patch now removes the
	rcu_irq_enter() and rcu_irq_exit() from within the stack tracer.

One further question...  Can I now remove the rcu_irq_enter_disabled()
logic?

> Cc: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
> Cc: stable@...r.kernel.org
> Fixes: 0be964be0 ("module: Sanitize RCU usage and locking")
> Suggested-by: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

With the hard-to-parse paragraph fixed:

Acked-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>

> ---
>  kernel/trace/trace_stack.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index a4df67cbc711..49cb41412eec 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -96,23 +96,9 @@ check_stack(unsigned long ip, unsigned long *stack)
>  	if (in_nmi())
>  		return;
> 
> -	/*
> -	 * There's a slight chance that we are tracing inside the
> -	 * RCU infrastructure, and rcu_irq_enter() will not work
> -	 * as expected.
> -	 */
> -	if (unlikely(rcu_irq_enter_disabled()))
> -		return;
> -
>  	local_irq_save(flags);
>  	arch_spin_lock(&stack_trace_max_lock);
> 
> -	/*
> -	 * RCU may not be watching, make it see us.
> -	 * The stack trace code uses rcu_sched.
> -	 */
> -	rcu_irq_enter();
> -
>  	/* In case another CPU set the tracer_frame on us */
>  	if (unlikely(!frame_size))
>  		this_size -= tracer_frame;
> @@ -205,7 +191,6 @@ check_stack(unsigned long ip, unsigned long *stack)
>  	}
> 
>   out:
> -	rcu_irq_exit();
>  	arch_spin_unlock(&stack_trace_max_lock);
>  	local_irq_restore(flags);
>  }
> -- 
> 2.13.2
> 
>