Re: [PATCH v13 bpf-next 10/10] selftests/bpf: tests for using dynptrs to parse skb and xdp buffers

From: Ilya Leoshkevich <iii@linux.ibm.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: "Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
	bpf <bpf@vger.kernel.org>,
	"Martin KaFai Lau" <martin.lau@kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Network Development" <netdev@vger.kernel.org>,
	"Toke Høiland-Jørgensen" <toke@kernel.org>
Subject: Re: [PATCH v13 bpf-next 10/10] selftests/bpf: tests for using dynptrs to parse skb and xdp buffers
Date: Wed, 08 Mar 2023 15:24:35 +0100	[thread overview]
Message-ID: <c27727cfabced2b9207eabbba71bed158ca35eec.camel@linux.ibm.com> (raw)
In-Reply-To: <CAJnrk1Y1ONmEJpwDqGzCUmyrkDf9s_HpDhR5mW=6fNKM6PiXew@mail.gmail.com>

On Tue, 2023-03-07 at 23:22 -0800, Joanne Koong wrote:
> On Tue, Mar 7, 2023 at 5:55 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Wed, Mar 01, 2023 at 08:28:40PM -0800, Joanne Koong wrote:
> > > On Wed, Mar 1, 2023 at 10:08 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > 
> > > > On Wed, Mar 1, 2023 at 7:51 AM Joanne Koong
> > > > <joannelkoong@gmail.com> wrote:
> > > > > 
> > > > > 5) progs/dynptr_success.c
> > > > >    * Add test case "test_skb_readonly" for testing attempts
> > > > > at writes
> > > > >      on a prog type with read-only skb ctx.
> > > > >    * Add "test_dynptr_skb_data" for testing that
> > > > > bpf_dynptr_data isn't
> > > > >      supported for skb progs.
> > > > 
> > > > I added
> > > > +dynptr/test_dynptr_skb_data
> > > > +dynptr/test_skb_readonly
> > > > to DENYLIST.s390x and applied.
> > > 
> > > Thanks, I'm still not sure why s390x cannot load these programs.
> > > It is
> > > being loaded in the same way as other tests like
> > > test_parse_tcp_hdr_opt() are loading programs. I will keep
> > > looking
> > > some more into this
> > 
> > Hi,
> > 
> > I believe the culprit is:
> > 
> >     insn->imm = BPF_CALL_IMM(bpf_dynptr_from_skb_rdonly);
> > 
> > s390x needs to know the kfunc model in order to emit the call (like
> > i386), but after this assignment it's no longer possible to look it
> > up in kfunc_tab by insn->imm. x86_64 does not need this, because
> > its
> > ABI is exactly the same as BPF ABI.
> > 
> > The simplest solution seems to be adding an artificial kfunc_desc
> > like this:
> > 
> >     {
> >         .func_model = desc->func_model,  /* model must be
> > compatible */
> >         .func_id = 0,                    /* unused at this point */
> >         .imm = insn->imm,                /* new target */
> >         .offset = 0,                     /* unused at this point */
> >     }
> > 
> > here and also after this assignment:
> > 
> >     insn->imm = BPF_CALL_IMM(xdp_kfunc);
> > 
> > What do you think?
> 
> Ohh interesting! This makes sense to me. In particular, you're
> referring to the bpf_jit_find_kfunc_model() call in bpf_jit_insn()
> (in
> arch/s390/net/bpf_jit_comp.c) as the one that fails out whenever
> insn->imm gets set, correct?

Precisely.

> I like your proposed solution, I agree that this looks like the
> simplest, though maybe we should replace the existing kfunc_desc
> instead of adding it so we don't have to deal with the edge case of
> reaching MAX_KFUNC_DESCS? To get the func model of the new insn->imm,

I wonder whether replacement is safe? This would depend on the
following functions returning the same value for the same inputs:

- may_access_direct_pkt_data() - this looks ok;
- bpf_dev_bound_resolve_kfunc() - I'm not so sure, any insights?

If it's not, then MAX_KFUNC_DESCS indeed becomes a concern.

> it seems pretty straightforward, it looks like we can just use
> btf_distill_func_proto(). or call add_kfunc_call() directly, which
> would do everything needed, but adds an additional unnecessary sort
> and more overhead for replacing (eg we'd need to first swap the old
> kfunc_desc with the last tab->descs[tab->nr_descs] entry and then
> delete the old kfunc_desc before adding the new one). What are your
> thoughts?

Is there a way to find BTF by function pointer?
IIUC bpf_dev_bound_resolve_kfunc() can return many different things,
and btf_distill_func_proto() and add_kfunc_call() need BTF.
A straightforward way that immediately comes to mind is to do kallsyms
lookup and then resolve by name, but this sounds clumsy.

I've been looking into this in context of fixing (kfunc 
__bpf_call_base) not fitting into 32 bits on s390x. A solution that
would solve both problems that I'm currently thinking about is to
associate

struct {
    struct btf_func_model *m;
    unsigned long addr;
} kfunc_callee;

with every insn - during verification it could live in
bpf_insn_aux_data, during jiting in bpf_prog, and afterwards it can
be freed. Any thoughts about this?