* [PATCH 1/2] bpf: hide unused bpf_patch_call_args
@ 2023-05-17 12:56 Arnd Bergmann
2023-05-17 12:56 ` [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration Arnd Bergmann
2023-05-22 14:26 ` [PATCH 1/2] bpf: hide unused bpf_patch_call_args Daniel Borkmann
0 siblings, 2 replies; 5+ messages in thread
From: Arnd Bergmann @ 2023-05-17 12:56 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Arnd Bergmann, John Fastabend, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jason A. Donenfeld, Kumar Kartikeya Dwivedi, Delyan Kratunov,
Ilya Leoshkevich, Menglong Dong, Yafang Shao, bpf, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
This function has no callers and no declaration when CONFIG_BPF_JIT_ALWAYS_ON
is enabled:
kernel/bpf/core.c:2075:6: error: no previous prototype for 'bpf_patch_call_args' [-Werror=missing-prototypes]
Hide the definition as well.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
kernel/bpf/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 7421487422d4..6f5ede31e471 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2064,7 +2064,7 @@ EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
};
#undef PROG_NAME_LIST
#define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
-static u64 (*interpreters_args[])(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5,
+static __maybe_unused u64 (*interpreters_args[])(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5,
const struct bpf_insn *insn) = {
EVAL6(PROG_NAME_LIST, 32, 64, 96, 128, 160, 192)
EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
@@ -2072,6 +2072,7 @@ EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
};
#undef PROG_NAME_LIST
+#ifdef CONFIG_BPF_SYSCALL
void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
{
stack_depth = max_t(u32, stack_depth, 1);
@@ -2080,6 +2081,7 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
__bpf_call_base_args;
insn->code = BPF_JMP | BPF_CALL_ARGS;
}
+#endif
#else
static unsigned int __bpf_prog_ret0_warn(const void *ctx,
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration
2023-05-17 12:56 [PATCH 1/2] bpf: hide unused bpf_patch_call_args Arnd Bergmann
@ 2023-05-17 12:56 ` Arnd Bergmann
2023-05-23 1:05 ` Alexei Starovoitov
2023-05-22 14:26 ` [PATCH 1/2] bpf: hide unused bpf_patch_call_args Daniel Borkmann
1 sibling, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2023-05-17 12:56 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Arnd Bergmann, John Fastabend, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jason A. Donenfeld, Kumar Kartikeya Dwivedi, Delyan Kratunov,
Ilya Leoshkevich, Menglong Dong, Yafang Shao, bpf, linux-kernel
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]
Add a prototype directly in front of the function instead to shut
up the warning. Also, to avoid having an incompatible function override
the __weak definition, use an #ifdef to ensure that only one of the
two is ever defined.
I'm not sure what can be done to make the two prototypes match.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
kernel/bpf/core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 6f5ede31e471..38762a784b86 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1635,11 +1635,14 @@ 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)
+u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr);
+#ifndef CONFIG_BPF_EVENTS
+u64 bpf_probe_read_kernel(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
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] bpf: hide unused bpf_patch_call_args
2023-05-17 12:56 [PATCH 1/2] bpf: hide unused bpf_patch_call_args Arnd Bergmann
2023-05-17 12:56 ` [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration Arnd Bergmann
@ 2023-05-22 14:26 ` Daniel Borkmann
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Borkmann @ 2023-05-22 14:26 UTC (permalink / raw
To: Arnd Bergmann, Alexei Starovoitov, Andrii Nakryiko
Cc: Arnd Bergmann, John Fastabend, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jason A. Donenfeld, Kumar Kartikeya Dwivedi, Delyan Kratunov,
Ilya Leoshkevich, Menglong Dong, Yafang Shao, bpf, linux-kernel
On 5/17/23 2:56 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> This function has no callers and no declaration when CONFIG_BPF_JIT_ALWAYS_ON
> is enabled:
>
> kernel/bpf/core.c:2075:6: error: no previous prototype for 'bpf_patch_call_args' [-Werror=missing-prototypes]
>
> Hide the definition as well.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> kernel/bpf/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 7421487422d4..6f5ede31e471 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2064,7 +2064,7 @@ EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
> };
> #undef PROG_NAME_LIST
> #define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
> -static u64 (*interpreters_args[])(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5,
> +static __maybe_unused u64 (*interpreters_args[])(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5,
> const struct bpf_insn *insn) = {
Patch 2 lgtm, small nit above: could you fix up indent?
> EVAL6(PROG_NAME_LIST, 32, 64, 96, 128, 160, 192)
> EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
> @@ -2072,6 +2072,7 @@ EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
> };
> #undef PROG_NAME_LIST
>
> +#ifdef CONFIG_BPF_SYSCALL
> void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
> {
> stack_depth = max_t(u32, stack_depth, 1);
> @@ -2080,6 +2081,7 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
> __bpf_call_base_args;
> insn->code = BPF_JMP | BPF_CALL_ARGS;
> }
> +#endif
>
> #else
> static unsigned int __bpf_prog_ret0_warn(const void *ctx,
Thanks,
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration
2023-05-17 12:56 ` [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration Arnd Bergmann
@ 2023-05-23 1:05 ` Alexei Starovoitov
2023-05-23 13:59 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2023-05-23 1:05 UTC (permalink / raw
To: Arnd Bergmann
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Arnd Bergmann, John Fastabend, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jason A. Donenfeld, Kumar Kartikeya Dwivedi, Delyan Kratunov,
Ilya Leoshkevich, Menglong Dong, Yafang Shao, bpf, LKML
On Wed, May 17, 2023 at 5:56 AM Arnd Bergmann <arnd@kernel.org> 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]
>
> Add a prototype directly in front of the function instead to shut
> up the warning. Also, to avoid having an incompatible function override
> the __weak definition, use an #ifdef to ensure that only one of the
> two is ever defined.
>
> I'm not sure what can be done to make the two prototypes match.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> kernel/bpf/core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 6f5ede31e471..38762a784b86 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1635,11 +1635,14 @@ 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)
> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr);
> +#ifndef CONFIG_BPF_EVENTS
> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> {
> memset(dst, 0, size);
> return -EFAULT;
> }
This is not right, but you've spotted a bug.
bpf_probe_read_kernel
It should be BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
const void *, unsafe_ptr)
here in kernel/bpf/core.c as well otherwise bpf prog won't
pass the arguments correctly on 32-bit arches.
The kconfig without CONFIG_BPF_EVENTS and with BPF_SYSCALL is very odd.
I suspect the progs will likely refuse to load,
but still worth fixing it correctly at least to document the calling convention.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration
2023-05-23 1:05 ` Alexei Starovoitov
@ 2023-05-23 13:59 ` Arnd Bergmann
0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2023-05-23 13:59 UTC (permalink / raw
To: Alexei Starovoitov, Arnd Bergmann
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jason A . Donenfeld, Kumar Kartikeya Dwivedi, Delyan Kratunov,
Ilya Leoshkevich, Menglong Dong, Yafang Shao, bpf, LKML
On Tue, May 23, 2023, at 03:05, Alexei Starovoitov wrote:
> On Wed, May 17, 2023 at 5:56 AM Arnd Bergmann <arnd@kernel.org> wrote:
>> @@ -1635,11 +1635,14 @@ 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)
>> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr);
>> +#ifndef CONFIG_BPF_EVENTS
>> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>> {
>> memset(dst, 0, size);
>> return -EFAULT;
>> }
>
> This is not right, but you've spotted a bug.
> bpf_probe_read_kernel
> It should be BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size,
> const void *, unsafe_ptr)
> here in kernel/bpf/core.c as well otherwise bpf prog won't
> pass the arguments correctly on 32-bit arches.
I tried that before and again now, but could not figure out how
to do this correctly though.
With this patch on top:
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1635,9 +1635,8 @@ bool bpf_opcode_in_insntable(u8 code)
}
#ifndef CONFIG_BPF_JIT_ALWAYS_ON
-u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr);
#ifndef CONFIG_BPF_EVENTS
-u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
+BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size, const void *, unsafe_ptr)
{
memset(dst, 0, size);
return -EFAULT;
I see a ton of other build failures, for every function calling
bpf_probe_read_kernel() from kernel/bpf:
kernel/bpf/core.c: In function '___bpf_prog_run':
kernel/bpf/core.c:1936:39: error: passing argument 1 of 'bpf_probe_read_kernel' makes integer from pointer without a cast [-Werror=int-conversion]
1936 | bpf_probe_read_kernel(&DST, sizeof(SIZE), \
| ^
| |
| u64 * {aka long long unsigned int *}
kernel/bpf/core.c:1937:39: error: passing argument 3 of 'bpf_probe_read_kernel' makes integer from pointer without a cast [-Werror=int-conversion
1937 | (const void *)(long) (SRC + insn->off)); \
Though the code from samples/bpf seems to be able to call this
without problems.
If you have a suggestion for how to do it correctly, can you send
that as a patch yourself? Let me know if you'd like me to run that
through my test builds.
> The kconfig without CONFIG_BPF_EVENTS and with BPF_SYSCALL is very odd.
> I suspect the progs will likely refuse to load, but still worth
> fixing it correctly at least to document the calling convention.
Do you think there should be a change to the Kconfig files as well then?
I see a lot of features depend on BPF_SYSCALL but not BPF_EVENTS:
HID_BPF, BPF_LIRC_MODE2, CGROUP_BPF, BPF_PRELOAD, DEBUG_INFO_BTF,
BPF_STREAM_PARSER, AF_KCM, XDP_SOCKETS and NETFILTER_BPF_LINK.
Right now, these can all be enabled when {KPROBE,UPROBE,PERF,BPF}_EVENTS
are disabled.
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-23 14:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-17 12:56 [PATCH 1/2] bpf: hide unused bpf_patch_call_args Arnd Bergmann
2023-05-17 12:56 ` [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration Arnd Bergmann
2023-05-23 1:05 ` Alexei Starovoitov
2023-05-23 13:59 ` Arnd Bergmann
2023-05-22 14:26 ` [PATCH 1/2] bpf: hide unused bpf_patch_call_args Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).