Hi Martin,
On Wed, Nov 30, 2022 at 8:15 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 11/29/22 5:20 AM, Eyal Birger wrote:
> > diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c
> > new file mode 100644
> > index 000000000000..757e15857dbf
> > --- /dev/null
> > +++ b/net/xfrm/xfrm_interface_bpf.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Unstable XFRM Helpers for TC-BPF hook
> > + *
> > + * These are called from SCHED_CLS BPF programs. Note that it is
> > + * allowed to break compatibility for these functions since the interface they
> > + * are exposed through to BPF programs is explicitly unstable.
> > + */
> > +
> > +#include <linux/bpf.h>
> > +#include <linux/btf_ids.h>
> > +
> > +#include <net/dst_metadata.h>
> > +#include <net/xfrm.h>
> > +
> > +struct bpf_xfrm_info {
> No need to introduce a bpf variant of the "struct xfrm_md_info" (more on this
> later).
>
> > + u32 if_id;
> > + int link;
> > +};
> > +
> > +static struct metadata_dst __percpu *xfrm_md_dst;
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > + "Global functions as their definitions will be in xfrm_interface BTF");
> > +
> > +__used noinline
> > +int bpf_skb_get_xfrm_info(struct __sk_buff *skb_ctx, struct bpf_xfrm_info *to)
>
> This kfunc is not needed. It only reads the skb->_skb_refdst. The new kfunc
> bpf_rdonly_cast() can be used. Take a look at the bpf_rdonly_cast() usages in
> the selftests/bpf/progs/type_cast.c. It was in bpf-next only but should also be
> in net-next now.
I'm somewhat concerned with this approach.
Indeed it would remove the kfunc, and the API is declared "unstable", but
still the implementation as dst isn't relevant to the user and would make
the programs less readable.
Also note that the helper can be also used as it is to get the xfrm info at
egress from an lwt route (which stores the xfrm_info in the dst lwstate).
>
> > +{
> > + struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> > + struct xfrm_md_info *info;
> > +
> > + memset(to, 0, sizeof(*to));
> > +
> > + info = skb_xfrm_md_info(skb);
> > + if (!info)
> > + return -EINVAL;
> > +
> > + to->if_id = info->if_id;
> > + to->link = info->link;
> > + return 0;
> > +}
> > +
> > +__used noinline
> > +int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx,
> > + const struct bpf_xfrm_info *from)
>
> Directly use "const struct xfrm_md_info *from" instead. This kfunc can check
> from->dst_orig != NULL and return -EINVAL. It will then have a consistent API
> with the bpf_rdonly_cast() mentioned above.
See above.
>
> > +{
> > + struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> > + struct metadata_dst *md_dst;
> > + struct xfrm_md_info *info;
> > +
> > + if (unlikely(skb_metadata_dst(skb)))
> > + return -EINVAL;
> > +
> > + md_dst = this_cpu_ptr(xfrm_md_dst);
> > +
> > + info = &md_dst->u.xfrm_info;
> > + memset(info, 0, sizeof(*info));
>
> Unnecessary memset here. Everything should have been initialized below.
> bpf_skb_set_tunnel_key() needs memset but not here.
Ok.
>
> > +
> > + info->if_id = from->if_id;
> > + info->link = from->link;
> > + skb_dst_force(skb);
> > + info->dst_orig = skb_dst(skb);
> > +
> > + dst_hold((struct dst_entry *)md_dst);
> > + skb_dst_set(skb, (struct dst_entry *)md_dst);
> > + return 0;
> > +}
> > +
> > +__diag_pop()
> > +
> > +BTF_SET8_START(xfrm_ifc_kfunc_set)
> > +BTF_ID_FLAGS(func, bpf_skb_get_xfrm_info)
> > +BTF_ID_FLAGS(func, bpf_skb_set_xfrm_info)
> > +BTF_SET8_END(xfrm_ifc_kfunc_set)
> > +
> > +static const struct btf_kfunc_id_set xfrm_interface_kfunc_set = {
> > + .owner = THIS_MODULE,
> > + .set = &xfrm_ifc_kfunc_set,
> > +};
> > +
> > +int __init register_xfrm_interface_bpf(void)
> > +{
> > + int err;
> > +
> > + xfrm_md_dst = metadata_dst_alloc_percpu(0, METADATA_XFRM,
> > + GFP_KERNEL);
>
> May be DEFINE_PER_CPU() instead?
Are you suggesting duplicating the dst init/cleanup logic here?
Personally given that this happens once at module load, I think it's best to
use the existing infrastructure, but am willing to add this here if you think
it's better.
Thanks for the review!
Eyal.
