lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 29 Nov 2022 10:36:31 +0200
From:   Eyal Birger <eyal.birger@...il.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kselftest@...r.kernel.org, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        andrii@...nel.org, daniel@...earbox.net, nicolas.dichtel@...nd.com,
        razor@...ckwall.org, mykolal@...com, ast@...nel.org,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org, shuah@...nel.org
Subject: Re: [PATCH ipsec-next 2/3] xfrm: interface: Add unstable helpers for
 setting/getting XFRM metadata from TC-BPF

(sent again in plain text, sorry for the noise).

Hi Martin.

On Tue, Nov 29, 2022 at 3:58 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 11/28/22 8:05 AM, Eyal Birger wrote:
> > This change adds xfrm metadata helpers using the unstable kfunc call
> > interface for the TC-BPF hooks. This allows steering traffic towards
> > different IPsec connections based on logic implemented in bpf programs.
> >
> > This object is built based on the availabilty of BTF debug info.
> >
> > The metadata percpu dsts used on TX take ownership of the original skb
> > dsts so that they may be used as part of the xfrm transmittion logic -
> > e.g.  for MTU calculations.
>
> A few quick comments and questions:

Thanks for your comments!

>
> >
> > Signed-off-by: Eyal Birger <eyal.birger@...il.com>
> > ---
> >   include/net/dst_metadata.h     |  1 +
> >   include/net/xfrm.h             | 20 ++++++++
> >   net/core/dst.c                 |  4 ++
> >   net/xfrm/Makefile              |  6 +++
> >   net/xfrm/xfrm_interface_bpf.c  | 92 ++++++++++++++++++++++++++++++++++
>
> Please tag for bpf-next
Sure. I wasn't totally sure which tree this belongs to.

>
> >   net/xfrm/xfrm_interface_core.c | 15 ++++++
> >   6 files changed, 138 insertions(+)
> >   create mode 100644 net/xfrm/xfrm_interface_bpf.c
> >
> > diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> > index a454cf4327fe..1b7fae4c6b24 100644
> > --- a/include/net/dst_metadata.h
> > +++ b/include/net/dst_metadata.h
> > @@ -26,6 +26,7 @@ struct macsec_info {
> >   struct xfrm_md_info {
> >       u32 if_id;
> >       int link;
> > +     struct dst_entry *dst_orig;
> >   };
> >
> >   struct metadata_dst {
>
> [ ... ]
>
> > diff --git a/net/core/dst.c b/net/core/dst.c
> > index bc9c9be4e080..4c2eb7e56dab 100644
> > --- a/net/core/dst.c
> > +++ b/net/core/dst.c
> > @@ -315,6 +315,8 @@ void metadata_dst_free(struct metadata_dst *md_dst)
> >   #ifdef CONFIG_DST_CACHE
> >       if (md_dst->type == METADATA_IP_TUNNEL)
> >               dst_cache_destroy(&md_dst->u.tun_info.dst_cache);
> > +     else if (md_dst->type == METADATA_XFRM)
> > +             dst_release(md_dst->u.xfrm_info.dst_orig);
>
> Why only release dst_orig under CONFIG_DST_CACHE?
It's a relic from a previous version where I'd used dst cache.
Will move out of this ifdef.

>
> >   #endif
> >       kfree(md_dst);
> >   }
> > @@ -348,6 +350,8 @@ void metadata_dst_free_percpu(struct metadata_dst __percpu *md_dst)
> >
> >               if (one_md_dst->type == METADATA_IP_TUNNEL)
> >                       dst_cache_destroy(&one_md_dst->u.tun_info.dst_cache);
> > +             else if (one_md_dst->type == METADATA_XFRM)
> > +                     dst_release(one_md_dst->u.xfrm_info.dst_orig);
>
> Same here.

Likewise.

>
> [ ... ]
>
> > diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c
> > new file mode 100644
> > index 000000000000..d3997ab7cc28
> > --- /dev/null
> > +++ b/net/xfrm/xfrm_interface_bpf.c
> > @@ -0,0 +1,92 @@
> > +// 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 {
> > +     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)
> > +{
> > +     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)
> > +{
> > +     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));
> > +
> > +     info->if_id = from->if_id;
> > +     info->link = from->link;
> > +     info->dst_orig = skb_dst(skb);
> However, the dst_orig init is not done under CONFIG_DST_CACHE though...
>
> Also, is it possible that skb->_skb_refdst has SKB_DST_NOREF set and later below
> ... (contd)
Nice catch! will force dst is refcounted.

>
> > +
> > +     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)
> > +{
> > +     xfrm_md_dst = metadata_dst_alloc_percpu(0, METADATA_XFRM,
> > +                                             GFP_KERNEL);
> > +     if (!xfrm_md_dst)
> > +             return -ENOMEM;
> > +     return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS,
> > +                                      &xfrm_interface_kfunc_set);
>
> Will cleanup_xfrm_interface_bpf() be called during error ?
No. Will fix in v2.

Thanks!
Eyal.

Powered by blists - more mailing lists