Re: [PATCH RFC bpf-next 1/1] bpf: Support 64-bit pointers to kfuncs

[Stanislav Fomichev <sdf@google.com>]

On 02/15, Ilya Leoshkevich wrote:
> On Wed, 2023-02-15 at 09:43 -0800, Stanislav Fomichev wrote:
> > On 02/15, Ilya Leoshkevich wrote:
> > > On Tue, 2023-02-14 at 15:58 -0800, Stanislav Fomichev wrote:
> > > > On 02/14, Ilya Leoshkevich wrote:
> > > > > test_ksyms_module fails to emit a kfunc call targeting a module
> > > > > on
> > > > > s390x, because the verifier stores the difference between kfunc
> > > > > address and __bpf_call_base in bpf_insn.imm, which is s32, and
> > > > > modules
> > > > > are roughly (1 << 42) bytes away from the kernel on s390x.
> > > >
> > > > > Fix by keeping BTF id in bpf_insn.imm for
> > > > > BPF_PSEUDO_KFUNC_CALLs,
> > > > > and storing the absolute address in bpf_kfunc_desc, which JITs
> > > > > retrieve
> > > > > as usual by calling bpf_jit_get_func_addr().
> > > >
> > > > > This also fixes the problem with XDP metadata functions
> > > > > outlined in
> > > > > the description of commit 63d7b53ab59f ("s390/bpf: Implement
> > > > > bpf_jit_supports_kfunc_call()") by replacing address lookups
> > > > > with
> > > > > BTF
> > > > > id lookups. This eliminates the inconsistency between
> > > > > "abstract"
> > > > > XDP
> > > > > metadata functions' BTF ids and their concrete addresses.
> > > >
> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > ---
> > > > > � include/linux/bpf.h�� |� 2 ++
> > > > > � kernel/bpf/core.c���� | 23 ++++++++++---
> > > > > � kernel/bpf/verifier.c | 79 +++++++++++++---------------------
> > > > > ----
> > > > > -----
> > > > > � 3 files changed, 45 insertions(+), 59 deletions(-)
> > >

> [...]

> > > > > +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32
> > > > > func_id,
> > > > > u16�
> > > > > offset,
> > > > > +��������������������� u8 **func_addr)
> > > > > +{
> > > > > +�������const struct bpf_kfunc_desc *desc;
> > > > > +
> > > > > +�������desc = find_kfunc_desc(prog, func_id, offset);
> > > > > +�������if (WARN_ON_ONCE(!desc))
> > > > > +���������������return -EINVAL;
> > > > > +
> > > > > +�������*func_addr = (u8 *)desc->addr;
> > > > > +�������return 0;
> > > > > +}
> > > >
> > > > This function isn't doing much and has a single caller. Should we
> > > > just
> > > > export find_kfunc_desc?
> >
> > > We would have to export struct bpf_kfunc_desc as well; I thought
> > > it's
> > > better to add an extra function so that we could keep hiding the
> > > struct.
> >
> > Ah, good point. In this case seems ok to have this extra wrapper.
> > On that note: what's the purpose of WARN_ON_ONCE here?

> We can hit this only due to an internal verifier/JIT error, so it would
> be good to get some indication of this happening. In verifier.c we have
> verbose() function for that, but this function is called during JITing.

> [...]

 From my point of view, reading the code, it makes it a bit confusing. If  
there
is a WARN_ON_ONCE, I'm assuming it's guarding against some kind of internal
inconsistency that can happen.

What kind of inconsistency is it guarding against here? We seem to have
find_kfunc_desc in fixup_kfunc_call that checks the same insn->imm
and returns early.