linux-kernel - Re: [PATCH v2] kernel: sched: fix preempt_disable_ip recodring for preempt

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:	Thu, 25 Feb 2016 09:15:30 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kernel: sched: fix preempt_disable_ip recodring for
 preempt_disable()

On Fri, 12 Feb 2016 16:20:20 +0100
Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:

> The preempt_disable() invokes preempt_count_add() which saves the caller
> in ->preempt_disable_ip. It uses CALLER_ADDR1 which does not look for
> its caller but for the parent of the caller. Which means we get the correct
> caller for something like spin_lock() unless the architectures inlines
> those invocations. It is always wrong for preempt_disable() or
> local_bh_disable().
> 
> This patch makes the function get_parent_ip() which tries
> CALLER_ADDR0,1,2 if the former is a locking function.
> This seems to record the preempt_disable() caller properly for
> preempt_disable() itself as well as for get_cpu_var() or
> local_bh_disable().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
> v1…v2: drop the header split and move it to ftrace.h instead
> 
>  include/linux/ftrace.h | 12 ++++++++++++
>  include/linux/sched.h  |  2 --
>  kernel/sched/core.c    | 14 ++------------
>  kernel/softirq.c       |  4 ++--
>  4 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 81de7123959d..f473d9408bc3 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -711,6 +711,18 @@ static inline void __ftrace_enabled_restore(int enabled)
>  #define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5))
>  #define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6))
>  
> +static inline unsigned long get_parent_ip(void)

I wonder if we should rename this to get_lock_parent_ip(). I know you
are keeping the original name, but placing it into a header makes it
more visible to others, and making it a less generic name is probably a
good idea.

You have to modify all users anyway, to get rid of the parameter.

Want to send a v3?

-- Steve


> +{
> +	unsigned long addr = CALLER_ADDR0;
> +
> +	if (!in_lock_functions(addr))
> +		return addr;
> +	addr = CALLER_ADDR1;
> +	if (!in_lock_functions(addr))
> +		return addr;
> +	return CALLER_ADDR2;
> +}
> +
>  #ifdef CONFIG_IRQSOFF_TRACER
>    extern void time_hardirqs_on(unsigned long a0, unsigned long a1);
>    extern void time_hardirqs_off(unsigned long a0, unsigned long a1);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a10494a94cc3..f45f947500c9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -182,8 +182,6 @@ extern void update_cpu_load_nohz(int active);
>  static inline void update_cpu_load_nohz(int active) { }
>  #endif
>  
> -extern unsigned long get_parent_ip(unsigned long addr);
> -
>  extern void dump_cpu_task(int cpu);
>  
>  struct seq_file;
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9503d590e5ef..12c2527f5957 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3010,16 +3010,6 @@ u64 scheduler_tick_max_deferment(void)
>  }
>  #endif
>  
> -notrace unsigned long get_parent_ip(unsigned long addr)
> -{
> -	if (in_lock_functions(addr)) {
> -		addr = CALLER_ADDR2;
> -		if (in_lock_functions(addr))
> -			addr = CALLER_ADDR3;
> -	}
> -	return addr;
> -}
> -
>  #if defined(CONFIG_PREEMPT) && (defined(CONFIG_DEBUG_PREEMPT) || \
>  				defined(CONFIG_PREEMPT_TRACER))
>  
> @@ -3041,7 +3031,7 @@ void preempt_count_add(int val)
>  				PREEMPT_MASK - 10);
>  #endif
>  	if (preempt_count() == val) {
> -		unsigned long ip = get_parent_ip(CALLER_ADDR1);
> +		unsigned long ip = get_parent_ip();
>  #ifdef CONFIG_DEBUG_PREEMPT
>  		current->preempt_disable_ip = ip;
>  #endif
> @@ -3068,7 +3058,7 @@ void preempt_count_sub(int val)
>  #endif
>  
>  	if (preempt_count() == val)
> -		trace_preempt_on(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
> +		trace_preempt_on(CALLER_ADDR0, get_parent_ip());
>  	__preempt_count_sub(val);
>  }
>  EXPORT_SYMBOL(preempt_count_sub);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 479e4436f787..ec71033a87a2 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -116,9 +116,9 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
>  
>  	if (preempt_count() == cnt) {
>  #ifdef CONFIG_DEBUG_PREEMPT
> -		current->preempt_disable_ip = get_parent_ip(CALLER_ADDR1);
> +		current->preempt_disable_ip = get_parent_ip();
>  #endif
> -		trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1));
> +		trace_preempt_off(CALLER_ADDR0, get_parent_ip());
>  	}
>  }
>  EXPORT_SYMBOL(__local_bh_disable_ip);