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@xxxxxxxxxx> wrote:
>
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
>  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.