[PATCH 1/2] bpf: hide unused bpf_patch_call_args

[PATCH 1/2] bpf: hide unused bpf_patch_call_args

[Arnd Bergmann]

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

[PATCH 2/2] bpf: add bpf_probe_read_kernel declaration

[Arnd Bergmann]

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

Re: [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration

[Alexei Starovoitov]

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.

Re: [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration

[Arnd Bergmann]

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

Re: [PATCH 1/2] bpf: hide unused bpf_patch_call_args

[Daniel Borkmann]

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