On Wed, 2023-02-22 at 15:43 -0800, Stanislav Fomichev wrote:
> On Wed, Feb 22, 2023 at 2:37 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> 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().
> >
> > Introduce bpf_get_kfunc_addr() instead of exposing both
> > find_kfunc_desc() and struct bpf_kfunc_desc.
> >
> > 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@xxxxxxxxxxxxx>
>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>
> With a nit below (and an unrelated question).
>
> I'll wait a bit for the buildbots to finish until ack'ing the rest.
> But the jit (except sparc quirks) and selftest changes also make
> sense to me.
>
> > ---
> > include/linux/bpf.h | 2 ++
> > kernel/bpf/core.c | 21 ++++++++++--
> > kernel/bpf/verifier.c | 79 +++++++++++++--------------------------
> > ----
> > 3 files changed, 44 insertions(+), 58 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 520b238abd5a..e521eae334ea 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2234,6 +2234,8 @@ bool bpf_prog_has_kfunc_call(const struct
> > bpf_prog *prog);
> > const struct btf_func_model *
> > bpf_jit_find_kfunc_model(const struct bpf_prog *prog,
> > const struct bpf_insn *insn);
> > +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32 func_id,
> > u16 offset,
> > + u8 **func_addr);
> > struct bpf_core_ctx {
> > struct bpf_verifier_log *log;
> > const struct btf *btf;
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 933869983e2a..4d51782f17ab 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1185,10 +1185,12 @@ int bpf_jit_get_func_addr(const struct
> > bpf_prog *prog,
> > {
> > s16 off = insn->off;
> > s32 imm = insn->imm;
>
> [..]
>
> > + bool fixed;
>
> nit: do we really need that extra fixed bool? Why not directly
> *func_addr_fixes = true/false in all the places?
I introduced it in order to avoid touching func_addr_fixed if there
is an error, but actually that's not necessary - it's assigned after
all checks. I will drop it in v4.
> > u8 *addr;
> > + int err;
> >
> > - *func_addr_fixed = insn->src_reg != BPF_PSEUDO_CALL;
> > - if (!*func_addr_fixed) {
> > + switch (insn->src_reg) {
> > + case BPF_PSEUDO_CALL:
> > /* Place-holder address till the last pass has
> > collected
> > * all addresses for JITed subprograms in which
> > case we
> > * can pick them up from prog->aux.
> > @@ -1200,15 +1202,28 @@ int bpf_jit_get_func_addr(const struct
> > bpf_prog *prog,
> > addr = (u8 *)prog->aux->func[off]-
> > >bpf_func;
> > else
> > return -EINVAL;
> > - } else {
> > + fixed = false;
> > + break;
> > + case 0:
> > /* Address of a BPF helper call. Since part of the
> > core
> > * kernel, it's always at a fixed location.
> > __bpf_call_base
> > * and the helper with imm relative to it are both
> > in core
> > * kernel.
> > */
> > addr = (u8 *)__bpf_call_base + imm;
> > + fixed = true;
> > + break;
> > + case BPF_PSEUDO_KFUNC_CALL:
> > + err = bpf_get_kfunc_addr(prog, imm, off, &addr);
> > + if (err)
> > + return err;
> > + fixed = true;
> > + break;
> > + default:
> > + return -EINVAL;
> > }
> >
> > + *func_addr_fixed = fixed;
> > *func_addr = (unsigned long)addr;
> > return 0;
> > }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 574d2dfc6ada..6d4632476c9c 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -2115,8 +2115,8 @@ static int add_subprog(struct
> > bpf_verifier_env *env, int off)
> > struct bpf_kfunc_desc {
> > struct btf_func_model func_model;
> > u32 func_id;
> > - s32 imm;
> > u16 offset;
>
> [..]
>
> > + unsigned long addr;
>
> Do we have some canonical type to store the address? I was using void
> * in bpf_dev_bound_resolve_kfunc, but we are doing ulong here. We
> seem
> to be doing u64/void */unsigned long inconsistently.
IIUC u64 is for BPF progs [1]. I've seen unsigned long in a number of
places, e.g. kallsyms. My personal heuristic is that if we don't
dereference it on the C side, it can be unsigned long. But I don't have
a strong opinion on this.
> Also, maybe move it up a bit? To turn u32+u16+gap+u64 into
> u64+u32+u16+padding ?
You are right, we can do better here w.r.t. space efficiency:
struct bpf_kfunc_desc {
struct btf_func_model func_model; /* 0 27
*/
/* XXX 1 byte hole, try to pack */
u32 func_id; /* 28 4
*/
u16 offset; /* 32 2
*/
/* XXX 6 bytes hole, try to pack */
long unsigned int addr; /* 40 8
*/
[1]
https://lore.kernel.org/bpf/CAEf4BzaQJfB0Qh2Wn5wd9H0ZSURbzWBfKkav8xbkhozqTWXndw@xxxxxxxxxxxxxx/
Best regards,
Ilya
