linux-kernel - Re: [PATCH v6 00/14] uprobes: Add uprobes support for ARM

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: <53131DBE.7020500@linaro.org>
Date:	Sun, 02 Mar 2014 07:02:06 -0500
From:	David Long <dave.long@...aro.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>,
	Oleg Nesterov <oleg@...hat.com>
CC:	linux-arm-kernel@...ts.infradead.org, Rabin Vincent <rabin@....in>,
	"Jon Medhurst (Tixy)" <tixy@...aro.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	davem@...emloft.net, Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 00/14] uprobes: Add uprobes support for ARM

Oleg,

I've been looking at arch/Kconfig and kernel/trace/Kconfig where they 
deal with uprobes.  The relevant items are CONFIG_UPROBES and 
CONFIG_UPROBE_EVENT.  It just doesn't look right to me.  It looks like 
"select" is used in part maybe just to avoid the recursive dependency 
error that would be generated if "depends on" were used in both places. 
However I don't think UPROBES should be dependent on UPROBE_EVENT, only 
the other way around.  As RK noted in previous email (copied in part 
below) the select does not pull in the lower level dependencies.  This 
all works on x86 only because arch/x86/Kconfig defines CONFIG_PERF_EVENT 
(which feels like a big hammer).  We don't need to do this on ARM, and 
we don't do it.  The result is that, unless PERF_EVENT is set 
separately, uprobes tends not to build.  I was lucking-out in my testing 
due to other default config items turning on PERF_EVENT.

What do you think about the following patch?:

> diff --git a/arch/Kconfig b/arch/Kconfig
> index 80bbb8c..8793066 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -87,7 +87,8 @@ config KPROBES_ON_FTRACE
>
>  config UPROBES
>  	bool "Transparent user-space probes (EXPERIMENTAL)"
> -	depends on UPROBE_EVENT && PERF_EVENTS
> +	depends on ARCH_SUPPORTS_UPROBES
> +	depends on PERF_EVENTS
>  	default n
>  	select PERCPU_RWSEM
>  	help
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 015f85a..7d2647e 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -422,9 +422,8 @@ config KPROBE_EVENT
>
>  config UPROBE_EVENT
>  	bool "Enable uprobes-based dynamic events"
> -	depends on ARCH_SUPPORTS_UPROBES
>  	depends on MMU
> -	select UPROBES
> +	depends on UPROBES
>  	select PROBE_EVENTS
>  	select TRACING
>  	default n


This removes the pseudo-recursive dependency.  The downside is that now 
you have to set PERF_EVENTS to use UPROBES, and UPROBES to use 
UPROBE_EVENT, whereas before on x86 you only had to set UPROBE_EVENT (as 
long as PERF_EVENTS stays hardcoded on). I think this could be avoided 
by making each "depends on" a "selects", since they apply transitively. 
  I'm sensing this would be an unpopular idea though.

-dl



On 03/01/14 07:30, Russell King - ARM Linux wrote:

[ ... ]

> Clearly, it doesn't make sense for UPROBES to be enabled with PERF_EVENTS
> disabled - and indeed the Kconfig ensures that this dependency is properly
> expressed:
>
> 	config UPROBES
> 	        bool "Transparent user-space probes (EXPERIMENTAL)"
> 	        depends on UPROBE_EVENT && PERF_EVENTS
> 	        default n
> 	        select PERCPU_RWSEM
>
> but where this all falls down is here:
>
> 	config UPROBE_EVENT
> 	        bool "Enable uprobes-based dynamic events"
> 	        depends on ARCH_SUPPORTS_UPROBES
> 	        depends on MMU
> 	        select UPROBES
> 	        select PROBE_EVENTS
> 	        select TRACING
> 	        default n
>
> Which is yet another brilliant example of why this "select" crap is soo
> evil.  Yes, the failing configuration has:
>
> 	CONFIG_UPROBE_EVENT=y
>
> Ineed, there was a Kconfig warning:
>
> 	warning: (UPROBE_EVENT) selects UPROBES which has unmet direct dependencies (UPROBE_EVENT && PERF_EVENTS)
>
> This is not your fault.  It's the fault of everyone who passed through
> commit f3f096cfedf8113380c56fc855275cc75cd8cf55 without properly reviewing
> it and paying attention to that select crap.  Given how evil "select" is,
> it's something which should always be thoroughly reviewed - with analysis
> of the dependencies.  I believe commits which introduce new select
> statements should document an analysis of why those new select statements
> are appropriate and how they ensure that any dependencies of the selected
> symbol are not violated.
>
> Therefore, I will not take the ARM uprobes code while this kind of
> select abortion is present - it needs to be fixed first to avoid these
> build errors.  Sorry.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/