Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-441-ga3ab13cd6d-fm-20230517.001-ga3ab13cd
Mime-Version: 1.0
Message-Id: <60d76601-de3a-498b-9ebd-e09add11f96f@app.fastmail.com>
In-Reply-To: <CAADnVQL+K22KEMu8fkQbsdmDAMcws1Cja3iM=E9zPLhDfDWmqw@mail.gmail.com>
References: <20230517125617.931437-1-arnd@kernel.org>
 <20230517125617.931437-2-arnd@kernel.org>
 <CAADnVQL+K22KEMu8fkQbsdmDAMcws1Cja3iM=E9zPLhDfDWmqw@mail.gmail.com>
Date:   Tue, 23 May 2023 15:59:27 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
        "Arnd Bergmann" <arnd@kernel.org>
Cc:     "Alexei Starovoitov" <ast@kernel.org>,
        "Daniel Borkmann" <daniel@iogearbox.net>,
        "Andrii Nakryiko" <andrii@kernel.org>,
        "John Fastabend" <john.fastabend@gmail.com>,
        "Martin KaFai Lau" <martin.lau@linux.dev>,
        "Song Liu" <song@kernel.org>, "Yonghong Song" <yhs@fb.com>,
        "KP Singh" <kpsingh@kernel.org>,
        "Stanislav Fomichev" <sdf@google.com>,
        "Hao Luo" <haoluo@google.com>, "Jiri Olsa" <jolsa@kernel.org>,
        "Jason A . Donenfeld" <Jason@zx2c4.com>,
        "Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
        "Delyan Kratunov" <delyank@fb.com>,
        "Ilya Leoshkevich" <iii@linux.ibm.com>,
        "Menglong Dong" <imagedong@tencent.com>,
        "Yafang Shao" <laoar.shao@gmail.com>, bpf <bpf@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] bpf: add bpf_probe_read_kernel declaration
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 23, 2023, at 03:05, Alexei Starovoitov wrote:
> On Wed, May 17, 2023 at 5:56=E2=80=AFAM 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 *un=
safe_ptr)
>> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_pt=
r);
>> +#ifndef CONFIG_BPF_EVENTS
>> +u64 bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_pt=
r)
>>  {
>>         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)
 }
=20
 #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=3Dint-convers=
ion]
 1936 |                 bpf_probe_read_kernel(&DST, sizeof(SIZE),       =
        \
      |                                       ^
      |                                       |
      |                                       u64 * {aka long long unsig=
ned int *}
kernel/bpf/core.c:1937:39: error: passing argument 3 of 'bpf_probe_read_=
kernel' makes integer from pointer without a cast [-Werror=3Dint-convers=
ion
 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