Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1354820 > unrolled thread

[PATCH] bpf: make bpf_stackmap_copy conditionally called

Back to article view | Back to linux.kernel

Contents

  [PATCH] bpf: make bpf_stackmap_copy conditionally called Arnd Bergmann <arnd@arndb.de> - 2016-03-10 04:30 +0100
    Re: [PATCH] bpf: make bpf_stackmap_copy conditionally called Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-03-10 04:40 +0100
      Re: [PATCH] bpf: make bpf_stackmap_copy conditionally called Arnd Bergmann <arnd@arndb.de> - 2016-03-10 10:20 +0100

#1354820 — [PATCH] bpf: make bpf_stackmap_copy conditionally called

Changing the bpf syscall to use the new bpf_stackmap_copy() helper for
BPF_MAP_TYPE_STACK_TRACE causes a link error when CONFIG_PERF_EVENTS
is disabled:

kernel/built-in.o: In function `map_lookup_elem':
:(.text+0x7fca4): undefined reference to `bpf_stackmap_copy'

This patch simply avoids handling that case, which may or may not
be the correct answer here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 557c0c6e7df8 ("bpf: convert stackmap to pre-allocation")
---
 kernel/bpf/syscall.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2978d0d08869..b014b64b6116 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -290,7 +290,8 @@ static int map_lookup_elem(union bpf_attr *attr)
 		err = bpf_percpu_hash_copy(map, key, value);
 	} else if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
 		err = bpf_percpu_array_copy(map, key, value);
-	} else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE) {
+	} else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE &&
+		   IS_ENABLED(CONFIG_PERF_EVENTS)) {
 		err = bpf_stackmap_copy(map, key, value);
 	} else {
 		rcu_read_lock();
-- 
2.7.0

[toc] | [next] | [standalone]

#1354824

On Thu, Mar 10, 2016 at 02:43:42AM +0100, Arnd Bergmann wrote:
> Changing the bpf syscall to use the new bpf_stackmap_copy() helper for
> BPF_MAP_TYPE_STACK_TRACE causes a link error when CONFIG_PERF_EVENTS
> is disabled:
> 
> kernel/built-in.o: In function `map_lookup_elem':
> :(.text+0x7fca4): undefined reference to `bpf_stackmap_copy'
> 
> This patch simply avoids handling that case, which may or may not
> be the correct answer here.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 557c0c6e7df8 ("bpf: convert stackmap to pre-allocation")
...
> -	} else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE) {
> +	} else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE &&
> +		   IS_ENABLED(CONFIG_PERF_EVENTS)) {
>  		err = bpf_stackmap_copy(map, key, value);
>  	} else {

yes. this is also ok-ish fix.
I've sent different version already:
http://patchwork.ozlabs.org/patch/595617/

I considered the option like yours but it's relying on gcc doing
dead code elimination of 'if (false) {}' branch and though kernel
is never compiled with -O0. I didn't want to take the risk.
I'm fine with either approach though.

[toc] | [prev] | [next] | [standalone]

#1354938

On Thursday 10 March 2016, Alexei Starovoitov wrote:
> yes. this is also ok-ish fix.
> I've sent different version already:
> http://patchwork.ozlabs.org/patch/595617/
> 
> I considered the option like yours but it's relying on gcc doing
> dead code elimination of 'if (false) {}' branch and though kernel
> is never compiled with -O0. I didn't want to take the risk.
> I'm fine with either approach though.

Ok, and I see yours is already applied, so that's fine.

In general, I don't like __weak symbols in the kernel as they
make it less clear what is actually getting called, and I think my
version would have been safe, we rely on building with -O2 or -Os
in a lot of places because of similar things.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web