* Re: [PATCH 2/2] [v3] bpf: fix bpf_probe_read_kernel prototype mismatch
2023-06-02 13:50 ` [PATCH 2/2] [v3] bpf: fix bpf_probe_read_kernel prototype mismatch Arnd Bergmann
@ 2023-06-06 22:49 ` Yonghong Song
2023-06-12 17:04 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Yonghong Song @ 2023-06-06 22:49 UTC (permalink / raw)
To: Arnd Bergmann, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Steven Rostedt, Masami Hiramatsu
Cc: Arnd Bergmann, stable, John Fastabend, Martin KaFai Lau,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Kumar Kartikeya Dwivedi, Dave Marchevsky, Delyan Kratunov,
Joanne Koong, Peter Zijlstra, Jason A. Donenfeld, Roberto Sassu,
bpf, linux-kernel, linux-trace-kernel
On 6/2/23 6:50 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> bpf_probe_read_kernel() has a __weak definition in core.c and another
> definition with an incompatible prototype in kernel/trace/bpf_trace.c,
> when CONFIG_BPF_EVENTS is enabled.
>
> Since the two are incompatible, there cannot be a shared declaration in
> a header file, but the lack of a prototype causes a W=1 warning:
>
> kernel/bpf/core.c:1638:12: error: no previous prototype for 'bpf_probe_read_kernel' [-Werror=missing-prototypes]
>
> On 32-bit architectures, the local prototype
>
> u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>
> passes arguments in other registers as the one in bpf_trace.c
>
> BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
> const void *, unsafe_ptr)
>
> which uses 64-bit arguments in pairs of registers.
>
> Change the core.c file to only reference the inner
> bpf_probe_read_kernel_common() helper and provide a prototype for that,
> to ensure this is compatible with both definitions.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] [v3] bpf: fix bpf_probe_read_kernel prototype mismatch
2023-06-02 13:50 ` [PATCH 2/2] [v3] bpf: fix bpf_probe_read_kernel prototype mismatch Arnd Bergmann
2023-06-06 22:49 ` Yonghong Song
@ 2023-06-12 17:04 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2023-06-12 17:04 UTC (permalink / raw)
To: Arnd Bergmann, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Song Liu, Steven Rostedt, Masami Hiramatsu
Cc: Arnd Bergmann, stable, John Fastabend, Martin KaFai Lau,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Kumar Kartikeya Dwivedi, Dave Marchevsky, Delyan Kratunov,
Joanne Koong, Peter Zijlstra, Jason A. Donenfeld, Roberto Sassu,
bpf, linux-kernel, linux-trace-kernel
On 6/2/23 3:50 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> bpf_probe_read_kernel() has a __weak definition in core.c and another
> definition with an incompatible prototype in kernel/trace/bpf_trace.c,
> when CONFIG_BPF_EVENTS is enabled.
>
> Since the two are incompatible, there cannot be a shared declaration in
> a header file, but the lack of a prototype causes a W=1 warning:
>
> kernel/bpf/core.c:1638:12: error: no previous prototype for 'bpf_probe_read_kernel' [-Werror=missing-prototypes]
>
> On 32-bit architectures, the local prototype
>
> u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>
> passes arguments in other registers as the one in bpf_trace.c
>
> BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
> const void *, unsafe_ptr)
>
> which uses 64-bit arguments in pairs of registers.
>
> Change the core.c file to only reference the inner
> bpf_probe_read_kernel_common() helper and provide a prototype for that,
> to ensure this is compatible with both definitions.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Sorry for the delay. I just took in patch 1/2, thanks!
Small comment below:
> --
> v3: clarify changelog text further.
> v2: rewrite completely to fix the mismatch.
> ---
> include/linux/bpf.h | 2 ++
> kernel/bpf/core.c | 9 ++++++---
> kernel/trace/bpf_trace.c | 3 +--
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f58895830adae..55826398acfba 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2619,6 +2619,8 @@ static inline void bpf_dynptr_set_rdonly(struct bpf_dynptr_kern *ptr)
> }
> #endif /* CONFIG_BPF_SYSCALL */
>
> +int bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr);
> +
> void __bpf_free_used_btfs(struct bpf_prog_aux *aux,
> struct btf_mod_pair *used_btfs, u32 len);
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 0926714641eb5..565ef6950c7a8 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -35,6 +35,7 @@
> #include <linux/bpf_verifier.h>
> #include <linux/nodemask.h>
> #include <linux/nospec.h>
> +#include <linux/bpf.h>
> #include <linux/bpf_mem_alloc.h>
> #include <linux/memcontrol.h>
>
> @@ -1635,11 +1636,13 @@ bool bpf_opcode_in_insntable(u8 code)
> }
>
> #ifndef CONFIG_BPF_JIT_ALWAYS_ON
> -u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> +#ifndef CONFIG_BPF_EVENTS
> +int bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr)
> {
> memset(dst, 0, size);
> return -EFAULT;
> }
> +#endif
>
> /**
> * ___bpf_prog_run - run eBPF program on a given context
> @@ -1931,8 +1934,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
> DST = *(SIZE *)(unsigned long) (SRC + insn->off); \
> CONT; \
> LDX_PROBE_MEM_##SIZEOP: \
> - bpf_probe_read_kernel(&DST, sizeof(SIZE), \
> - (const void *)(long) (SRC + insn->off)); \
> + bpf_probe_read_kernel_common(&DST, sizeof(SIZE), \
> + (const void *)(long) (SRC + insn->off)); \
> DST = *((SIZE *)&DST); \
> CONT;
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 2bc41e6ac9fe0..290fdce2ce535 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -223,8 +223,7 @@ const struct bpf_func_proto bpf_probe_read_user_str_proto = {
> .arg3_type = ARG_ANYTHING,
> };
>
> -static __always_inline int
> -bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr)
> +int bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr)
> {
> int ret;
Given this is critical fast-path, please find a solution where we don't need
to penalize bpf_probe_read_kernel_common.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 3+ messages in thread