On Tue, May 23, 2023, at 03:05, Alexei Starovoitov wrote:
> On Wed, May 17, 2023 at 5:56 AM Arnd Bergmann <arnd@xxxxxxxxxx> 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
