Date:   Fri, 4 Nov 2022 19:18:17 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
        haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org
Subject: Re: [RFC bpf-next v2 08/14] bpf: Helper to simplify calling kernel
 routines from unrolled kfuncs

On Fri, Nov 4, 2022 at 5:40 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Nov 03, 2022 at 08:25:26PM -0700, Stanislav Fomichev wrote:
> > When we need to call the kernel function from the unrolled
> > kfunc, we have to take extra care: r6-r9 belong to the callee,
> > not us, so we can't use these registers to stash our r1.
> >
> > We use the same trick we use elsewhere: ask the user
> > to provide extra on-stack storage.
> >
> > Also, note, the program being called has to receive and
> > return the context.
> >
> > Cc: John Fastabend <john.fastabend@...il.com>
> > Cc: David Ahern <dsahern@...il.com>
> > Cc: Martin KaFai Lau <martin.lau@...ux.dev>
> > Cc: Jakub Kicinski <kuba@...nel.org>
> > Cc: Willem de Bruijn <willemb@...gle.com>
> > Cc: Jesper Dangaard Brouer <brouer@...hat.com>
> > Cc: Anatoly Burakov <anatoly.burakov@...el.com>
> > Cc: Alexander Lobakin <alexandr.lobakin@...el.com>
> > Cc: Magnus Karlsson <magnus.karlsson@...il.com>
> > Cc: Maryam Tahhan <mtahhan@...hat.com>
> > Cc: xdp-hints@...-project.net
> > Cc: netdev@...r.kernel.org
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  include/net/xdp.h |  4 ++++
> >  net/core/xdp.c    | 24 +++++++++++++++++++++++-
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 8c97c6996172..09c05d1da69c 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -440,10 +440,14 @@ static inline u32 xdp_metadata_kfunc_id(int id)
> >       return xdp_metadata_kfunc_ids.pairs[id].id;
> >  }
> >  void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch);
> > +void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> > +                               void *kfunc);
> >  #else
> >  #define xdp_metadata_magic 0
> >  static inline u32 xdp_metadata_kfunc_id(int id) { return 0; }
> >  static void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch) { return 0; }
> > +static void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> > +                                      void *kfunc) {}
> >  #endif
> >
> >  #endif /* __LINUX_NET_XDP_H__ */
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 8204fa05c5e9..16dd7850b9b0 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -737,6 +737,7 @@ BTF_SET8_START_GLOBAL(xdp_metadata_kfunc_ids)
> >  XDP_METADATA_KFUNC_xxx
> >  #undef XDP_METADATA_KFUNC
> >  BTF_SET8_END(xdp_metadata_kfunc_ids)
> > +EXPORT_SYMBOL(xdp_metadata_kfunc_ids);
> >
> >  /* Make sure userspace doesn't depend on our layout by using
> >   * different pseudo-generated magic value.
> > @@ -756,7 +757,8 @@ static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
> >   *
> >   * The above also means we _cannot_ easily call any other helper/kfunc
> >   * because there is no place for us to preserve our R1 argument;
> > - * existing R6-R9 belong to the callee.
> > + * existing R6-R9 belong to the callee. For the cases where calling into
> > + * the kernel is the only option, see xdp_kfunc_call_preserving_r1.
> >   */
> >  void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch)
> >  {
> > @@ -832,6 +834,26 @@ void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *p
> >
> >       bpf_patch_resolve_jmp(patch);
> >  }
> > +EXPORT_SYMBOL(xdp_metadata_export_to_skb);
> > +
> > +/* Helper to generate the bytecode that calls the supplied kfunc.
> > + * The kfunc has to accept a pointer to the context and return the
> > + * same pointer back. The user also has to supply an offset within
> > + * the context to store r0.
> > + */
> > +void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> > +                               void *kfunc)
> > +{
> > +     bpf_patch_append(patch,
> > +             /* r0 = kfunc(r1); */
> > +             BPF_EMIT_CALL(kfunc),
> > +             /* r1 = r0; */
> > +             BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> > +             /* r0 = *(r1 + r0_offset); */
> > +             BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, r0_offset),
> > +     );
> > +}
> > +EXPORT_SYMBOL(xdp_kfunc_call_preserving_r1);
>
> That's one twisted logic :)
> I guess it works for preserving r1, but r2-r5 are gone and r6-r9 cannot be touched.
> So it works for the most basic case of returning single value from that additional
> kfunc while preserving single r1.
> I'm afraid that will be very limiting moving forward.
> imo we need push/pop insns. It shouldn't difficult to add to the interpreter and JITs.
> Since this patching is done after verificaiton they will be kernel internal insns.
> Like we have BPF_REG_AX internal reg.

Heh, having push/pop would help a lot, agree :-)
Good suggestion, will look into that, thank you!

Powered by blists - more mailing lists