diff mbox series

[RFC,bpf-next,1/6] bpf: xfrm: Add bpf_xdp_get_xfrm_state() kfunc

Message ID ee5513e6384696147da9bdccd2e22ea27d690084.1698431765.git.dxu@dxuuu.xyz
State New
Headers
Series Add bpf_xdp_get_xfrm_state() kfunc |

Commit Message

Daniel Xu Oct. 27, 2023, 6:46 p.m. UTC 
  This commit adds an unstable kfunc helper to access internal xfrm_state
associated with an SA. This is intended to be used for the upcoming
IPsec pcpu work to assign special pcpu SAs to a particular CPU. In other
words: for custom software RSS.

That being said, the function that this kfunc wraps is fairly generic
and used for a lot of xfrm tasks. I'm sure people will find uses
elsewhere over time.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/net/xfrm.h        |   9 ++++
 net/xfrm/Makefile         |   1 +
 net/xfrm/xfrm_policy.c    |   2 +
 net/xfrm/xfrm_state_bpf.c | 105 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+)
 create mode 100644 net/xfrm/xfrm_state_bpf.c

Comments

Alexei Starovoitov Oct. 28, 2023, 11:49 p.m. UTC | #1 
On Fri, Oct 27, 2023 at 11:46 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> This commit adds an unstable kfunc helper to access internal xfrm_state
> associated with an SA. This is intended to be used for the upcoming
> IPsec pcpu work to assign special pcpu SAs to a particular CPU. In other
> words: for custom software RSS.
>
> That being said, the function that this kfunc wraps is fairly generic
> and used for a lot of xfrm tasks. I'm sure people will find uses
> elsewhere over time.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  include/net/xfrm.h        |   9 ++++
>  net/xfrm/Makefile         |   1 +
>  net/xfrm/xfrm_policy.c    |   2 +
>  net/xfrm/xfrm_state_bpf.c | 105 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 117 insertions(+)
>  create mode 100644 net/xfrm/xfrm_state_bpf.c
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 98d7aa78adda..ab4cf66480f3 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -2188,4 +2188,13 @@ static inline int register_xfrm_interface_bpf(void)
>
>  #endif
>
> +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF)
> +int register_xfrm_state_bpf(void);
> +#else
> +static inline int register_xfrm_state_bpf(void)
> +{
> +       return 0;
> +}
> +#endif
> +
>  #endif /* _NET_XFRM_H */
> diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
> index cd47f88921f5..547cec77ba03 100644
> --- a/net/xfrm/Makefile
> +++ b/net/xfrm/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
>  obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
>  obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
>  obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
> +obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 5cdd3bca3637..62e64fa7ae5c 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -4267,6 +4267,8 @@ void __init xfrm_init(void)
>  #ifdef CONFIG_XFRM_ESPINTCP
>         espintcp_init();
>  #endif
> +
> +       register_xfrm_state_bpf();
>  }
>
>  #ifdef CONFIG_AUDITSYSCALL
> diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
> new file mode 100644
> index 000000000000..a73a17a6497b
> --- /dev/null
> +++ b/net/xfrm/xfrm_state_bpf.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Unstable XFRM state BPF helpers.
> + *
> + * 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/xdp.h>
> +#include <net/xfrm.h>
> +
> +/* bpf_xfrm_state_opts - Options for XFRM state lookup helpers
> + *
> + * Members:
> + * @error      - Out parameter, set for any errors encountered
> + *              Values:
> + *                -EINVAL - netns_id is less than -1
> + *                -EINVAL - Passed NULL for opts
> + *                -EINVAL - opts__sz isn't BPF_XFRM_STATE_OPTS_SZ
> + *                -ENONET - No network namespace found for netns_id
> + * @netns_id   - Specify the network namespace for lookup
> + *              Values:
> + *                BPF_F_CURRENT_NETNS (-1)
> + *                  Use namespace associated with ctx
> + *                [0, S32_MAX]
> + *                  Network Namespace ID
> + * @mark       - XFRM mark to match on
> + * @daddr      - Destination address to match on
> + * @spi                - Security parameter index to match on
> + * @proto      - L3 protocol to match on
> + * @family     - L3 protocol family to match on
> + */
> +struct bpf_xfrm_state_opts {
> +       s32 error;
> +       s32 netns_id;
> +       u32 mark;
> +       xfrm_address_t daddr;
> +       __be32 spi;
> +       u8 proto;
> +       u16 family;
> +};
> +
> +enum {
> +       BPF_XFRM_STATE_OPTS_SZ = sizeof(struct bpf_xfrm_state_opts),
> +};
> +
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +                 "Global functions as their definitions will be in xfrm_state BTF");
> +
> +/* bpf_xdp_get_xfrm_state - Get XFRM state
> + *
> + * Parameters:
> + * @ctx        - Pointer to ctx (xdp_md) in XDP program
> + *                 Cannot be NULL
> + * @opts       - Options for lookup (documented above)
> + *                 Cannot be NULL
> + * @opts__sz   - Length of the bpf_xfrm_state_opts structure
> + *                 Must be BPF_XFRM_STATE_OPTS_SZ
> + */
> +__bpf_kfunc struct xfrm_state *
> +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts, u32 opts__sz)
> +{
> +       struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> +       struct net *net = dev_net(xdp->rxq->dev);
> +
> +       if (!opts || opts__sz != BPF_XFRM_STATE_OPTS_SZ) {
> +               opts->error = -EINVAL;
> +               return NULL;
> +       }
> +
> +       if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS)) {
> +               opts->error = -EINVAL;
> +               return NULL;
> +       }
> +
> +       if (opts->netns_id >= 0) {
> +               net = get_net_ns_by_id(net, opts->netns_id);
> +               if (unlikely(!net)) {
> +                       opts->error = -ENONET;
> +                       return NULL;
> +               }
> +       }
> +
> +       return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi,
> +                                opts->proto, opts->family);
> +}

Patch 6 example does little to explain how this kfunc can be used.
Cover letter sounds promising, but no code to demonstrate the result.
The main issue is that this kfunc has to be KF_ACQUIRE,
otherwise bpf prog will keep leaking xfrm_state.
Plenty of red flags in this RFC.
Daniel Xu Oct. 29, 2023, 10:55 p.m. UTC | #2 
Hi Alexei,

On Sat, Oct 28, 2023 at 04:49:45PM -0700, Alexei Starovoitov wrote:
> On Fri, Oct 27, 2023 at 11:46 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > This commit adds an unstable kfunc helper to access internal xfrm_state
> > associated with an SA. This is intended to be used for the upcoming
> > IPsec pcpu work to assign special pcpu SAs to a particular CPU. In other
> > words: for custom software RSS.
> >
> > That being said, the function that this kfunc wraps is fairly generic
> > and used for a lot of xfrm tasks. I'm sure people will find uses
> > elsewhere over time.
> >
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >  include/net/xfrm.h        |   9 ++++
> >  net/xfrm/Makefile         |   1 +
> >  net/xfrm/xfrm_policy.c    |   2 +
> >  net/xfrm/xfrm_state_bpf.c | 105 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 117 insertions(+)
> >  create mode 100644 net/xfrm/xfrm_state_bpf.c
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index 98d7aa78adda..ab4cf66480f3 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -2188,4 +2188,13 @@ static inline int register_xfrm_interface_bpf(void)
> >
> >  #endif
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF)
> > +int register_xfrm_state_bpf(void);
> > +#else
> > +static inline int register_xfrm_state_bpf(void)
> > +{
> > +       return 0;
> > +}
> > +#endif
> > +
> >  #endif /* _NET_XFRM_H */
> > diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
> > index cd47f88921f5..547cec77ba03 100644
> > --- a/net/xfrm/Makefile
> > +++ b/net/xfrm/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
> >  obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
> >  obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
> >  obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
> > +obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 5cdd3bca3637..62e64fa7ae5c 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -4267,6 +4267,8 @@ void __init xfrm_init(void)
> >  #ifdef CONFIG_XFRM_ESPINTCP
> >         espintcp_init();
> >  #endif
> > +
> > +       register_xfrm_state_bpf();
> >  }
> >
> >  #ifdef CONFIG_AUDITSYSCALL
> > diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
> > new file mode 100644
> > index 000000000000..a73a17a6497b
> > --- /dev/null
> > +++ b/net/xfrm/xfrm_state_bpf.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Unstable XFRM state BPF helpers.
> > + *
> > + * 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/xdp.h>
> > +#include <net/xfrm.h>
> > +
> > +/* bpf_xfrm_state_opts - Options for XFRM state lookup helpers
> > + *
> > + * Members:
> > + * @error      - Out parameter, set for any errors encountered
> > + *              Values:
> > + *                -EINVAL - netns_id is less than -1
> > + *                -EINVAL - Passed NULL for opts
> > + *                -EINVAL - opts__sz isn't BPF_XFRM_STATE_OPTS_SZ
> > + *                -ENONET - No network namespace found for netns_id
> > + * @netns_id   - Specify the network namespace for lookup
> > + *              Values:
> > + *                BPF_F_CURRENT_NETNS (-1)
> > + *                  Use namespace associated with ctx
> > + *                [0, S32_MAX]
> > + *                  Network Namespace ID
> > + * @mark       - XFRM mark to match on
> > + * @daddr      - Destination address to match on
> > + * @spi                - Security parameter index to match on
> > + * @proto      - L3 protocol to match on
> > + * @family     - L3 protocol family to match on
> > + */
> > +struct bpf_xfrm_state_opts {
> > +       s32 error;
> > +       s32 netns_id;
> > +       u32 mark;
> > +       xfrm_address_t daddr;
> > +       __be32 spi;
> > +       u8 proto;
> > +       u16 family;
> > +};
> > +
> > +enum {
> > +       BPF_XFRM_STATE_OPTS_SZ = sizeof(struct bpf_xfrm_state_opts),
> > +};
> > +
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +                 "Global functions as their definitions will be in xfrm_state BTF");
> > +
> > +/* bpf_xdp_get_xfrm_state - Get XFRM state
> > + *
> > + * Parameters:
> > + * @ctx        - Pointer to ctx (xdp_md) in XDP program
> > + *                 Cannot be NULL
> > + * @opts       - Options for lookup (documented above)
> > + *                 Cannot be NULL
> > + * @opts__sz   - Length of the bpf_xfrm_state_opts structure
> > + *                 Must be BPF_XFRM_STATE_OPTS_SZ
> > + */
> > +__bpf_kfunc struct xfrm_state *
> > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts, u32 opts__sz)
> > +{
> > +       struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> > +       struct net *net = dev_net(xdp->rxq->dev);
> > +
> > +       if (!opts || opts__sz != BPF_XFRM_STATE_OPTS_SZ) {
> > +               opts->error = -EINVAL;
> > +               return NULL;
> > +       }
> > +
> > +       if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS)) {
> > +               opts->error = -EINVAL;
> > +               return NULL;
> > +       }
> > +
> > +       if (opts->netns_id >= 0) {
> > +               net = get_net_ns_by_id(net, opts->netns_id);
> > +               if (unlikely(!net)) {
> > +                       opts->error = -ENONET;
> > +                       return NULL;
> > +               }
> > +       }
> > +
> > +       return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi,
> > +                                opts->proto, opts->family);
> > +}
> 
> Patch 6 example does little to explain how this kfunc can be used.
> Cover letter sounds promising, but no code to demonstrate the result.

Part of the reason for that is this kfunc is intended to be used with a
not-yet-upstreamed xfrm patchset. The other is that the usage is quite
trivial. This is the code the experiments were run with:

https://github.com/danobi/xdp-tools/blob/e89a1c617aba3b50d990f779357d6ce2863ecb27/xdp-bench/xdp_redirect_cpumap.bpf.c#L385-L406

We intend to upstream that cpumap mode to xdp-tools as soon as the xfrm
patches are in. (Note the linked code is a little buggy but the
main idea is there).

Depending on your appetite for complex diagrams, I can also offer you a
sequence diagram that describes how everything fits together:

https://dxuuu.xyz/r/ipsec-pcpu.png

The TLDR is that all the magic comes from xfrm subsystem. This kfunc
just enables software RSS.

> The main issue is that this kfunc has to be KF_ACQUIRE,
> otherwise bpf prog will keep leaking xfrm_state.
> Plenty of red flags in this RFC.

Ack, will check on KF_ACQUIRE.

Thanks,
Daniel
Alexei Starovoitov Oct. 31, 2023, 10:38 p.m. UTC | #3 
On Sun, Oct 29, 2023 at 3:55 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Hi Alexei,
>
> On Sat, Oct 28, 2023 at 04:49:45PM -0700, Alexei Starovoitov wrote:
> > On Fri, Oct 27, 2023 at 11:46 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > This commit adds an unstable kfunc helper to access internal xfrm_state
> > > associated with an SA. This is intended to be used for the upcoming
> > > IPsec pcpu work to assign special pcpu SAs to a particular CPU. In other
> > > words: for custom software RSS.
> > >
> > > That being said, the function that this kfunc wraps is fairly generic
> > > and used for a lot of xfrm tasks. I'm sure people will find uses
> > > elsewhere over time.
> > >
> > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > ---
> > >  include/net/xfrm.h        |   9 ++++
> > >  net/xfrm/Makefile         |   1 +
> > >  net/xfrm/xfrm_policy.c    |   2 +
> > >  net/xfrm/xfrm_state_bpf.c | 105 ++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 117 insertions(+)
> > >  create mode 100644 net/xfrm/xfrm_state_bpf.c
> > >
> > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > index 98d7aa78adda..ab4cf66480f3 100644
> > > --- a/include/net/xfrm.h
> > > +++ b/include/net/xfrm.h
> > > @@ -2188,4 +2188,13 @@ static inline int register_xfrm_interface_bpf(void)
> > >
> > >  #endif
> > >
> > > +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF)
> > > +int register_xfrm_state_bpf(void);
> > > +#else
> > > +static inline int register_xfrm_state_bpf(void)
> > > +{
> > > +       return 0;
> > > +}
> > > +#endif
> > > +
> > >  #endif /* _NET_XFRM_H */
> > > diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
> > > index cd47f88921f5..547cec77ba03 100644
> > > --- a/net/xfrm/Makefile
> > > +++ b/net/xfrm/Makefile
> > > @@ -21,3 +21,4 @@ obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
> > >  obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
> > >  obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
> > >  obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
> > > +obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o
> > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > > index 5cdd3bca3637..62e64fa7ae5c 100644
> > > --- a/net/xfrm/xfrm_policy.c
> > > +++ b/net/xfrm/xfrm_policy.c
> > > @@ -4267,6 +4267,8 @@ void __init xfrm_init(void)
> > >  #ifdef CONFIG_XFRM_ESPINTCP
> > >         espintcp_init();
> > >  #endif
> > > +
> > > +       register_xfrm_state_bpf();
> > >  }
> > >
> > >  #ifdef CONFIG_AUDITSYSCALL
> > > diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
> > > new file mode 100644
> > > index 000000000000..a73a17a6497b
> > > --- /dev/null
> > > +++ b/net/xfrm/xfrm_state_bpf.c
> > > @@ -0,0 +1,105 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Unstable XFRM state BPF helpers.
> > > + *
> > > + * 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/xdp.h>
> > > +#include <net/xfrm.h>
> > > +
> > > +/* bpf_xfrm_state_opts - Options for XFRM state lookup helpers
> > > + *
> > > + * Members:
> > > + * @error      - Out parameter, set for any errors encountered
> > > + *              Values:
> > > + *                -EINVAL - netns_id is less than -1
> > > + *                -EINVAL - Passed NULL for opts
> > > + *                -EINVAL - opts__sz isn't BPF_XFRM_STATE_OPTS_SZ
> > > + *                -ENONET - No network namespace found for netns_id
> > > + * @netns_id   - Specify the network namespace for lookup
> > > + *              Values:
> > > + *                BPF_F_CURRENT_NETNS (-1)
> > > + *                  Use namespace associated with ctx
> > > + *                [0, S32_MAX]
> > > + *                  Network Namespace ID
> > > + * @mark       - XFRM mark to match on
> > > + * @daddr      - Destination address to match on
> > > + * @spi                - Security parameter index to match on
> > > + * @proto      - L3 protocol to match on
> > > + * @family     - L3 protocol family to match on
> > > + */
> > > +struct bpf_xfrm_state_opts {
> > > +       s32 error;
> > > +       s32 netns_id;
> > > +       u32 mark;
> > > +       xfrm_address_t daddr;
> > > +       __be32 spi;
> > > +       u8 proto;
> > > +       u16 family;
> > > +};
> > > +
> > > +enum {
> > > +       BPF_XFRM_STATE_OPTS_SZ = sizeof(struct bpf_xfrm_state_opts),
> > > +};
> > > +
> > > +__diag_push();
> > > +__diag_ignore_all("-Wmissing-prototypes",
> > > +                 "Global functions as their definitions will be in xfrm_state BTF");
> > > +
> > > +/* bpf_xdp_get_xfrm_state - Get XFRM state
> > > + *
> > > + * Parameters:
> > > + * @ctx        - Pointer to ctx (xdp_md) in XDP program
> > > + *                 Cannot be NULL
> > > + * @opts       - Options for lookup (documented above)
> > > + *                 Cannot be NULL
> > > + * @opts__sz   - Length of the bpf_xfrm_state_opts structure
> > > + *                 Must be BPF_XFRM_STATE_OPTS_SZ
> > > + */
> > > +__bpf_kfunc struct xfrm_state *
> > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts, u32 opts__sz)
> > > +{
> > > +       struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> > > +       struct net *net = dev_net(xdp->rxq->dev);
> > > +
> > > +       if (!opts || opts__sz != BPF_XFRM_STATE_OPTS_SZ) {
> > > +               opts->error = -EINVAL;
> > > +               return NULL;
> > > +       }
> > > +
> > > +       if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS)) {
> > > +               opts->error = -EINVAL;
> > > +               return NULL;
> > > +       }
> > > +
> > > +       if (opts->netns_id >= 0) {
> > > +               net = get_net_ns_by_id(net, opts->netns_id);
> > > +               if (unlikely(!net)) {
> > > +                       opts->error = -ENONET;
> > > +                       return NULL;
> > > +               }
> > > +       }
> > > +
> > > +       return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi,
> > > +                                opts->proto, opts->family);
> > > +}
> >
> > Patch 6 example does little to explain how this kfunc can be used.
> > Cover letter sounds promising, but no code to demonstrate the result.
>
> Part of the reason for that is this kfunc is intended to be used with a
> not-yet-upstreamed xfrm patchset. The other is that the usage is quite
> trivial. This is the code the experiments were run with:
>
> https://github.com/danobi/xdp-tools/blob/e89a1c617aba3b50d990f779357d6ce2863ecb27/xdp-bench/xdp_redirect_cpumap.bpf.c#L385-L406
>
> We intend to upstream that cpumap mode to xdp-tools as soon as the xfrm
> patches are in. (Note the linked code is a little buggy but the
> main idea is there).

I don't understand how it survives anything, but sanity check.
To measure perf gains it needs to be under traffic for some time,
but
x = bpf_xdp_get_xfrm_state(ctx, &opts, sizeof(opts));
will keep refcnt++ that state for every packet.
Minimum -> memory leak or refcnt overflow.
Daniel Xu Nov. 1, 2023, 5:51 p.m. UTC | #4 
On Tue, Oct 31, 2023 at 03:38:26PM -0700, Alexei Starovoitov wrote:
> On Sun, Oct 29, 2023 at 3:55 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > Hi Alexei,
> >
> > On Sat, Oct 28, 2023 at 04:49:45PM -0700, Alexei Starovoitov wrote:
> > > On Fri, Oct 27, 2023 at 11:46 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > >
> > > > This commit adds an unstable kfunc helper to access internal xfrm_state
> > > > associated with an SA. This is intended to be used for the upcoming
> > > > IPsec pcpu work to assign special pcpu SAs to a particular CPU. In other
> > > > words: for custom software RSS.
> > > >
> > > > That being said, the function that this kfunc wraps is fairly generic
> > > > and used for a lot of xfrm tasks. I'm sure people will find uses
> > > > elsewhere over time.
> > > >
> > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > ---
> > > >  include/net/xfrm.h        |   9 ++++
> > > >  net/xfrm/Makefile         |   1 +
> > > >  net/xfrm/xfrm_policy.c    |   2 +
> > > >  net/xfrm/xfrm_state_bpf.c | 105 ++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 117 insertions(+)
> > > >  create mode 100644 net/xfrm/xfrm_state_bpf.c
> > > >
> > > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > > index 98d7aa78adda..ab4cf66480f3 100644
> > > > --- a/include/net/xfrm.h
> > > > +++ b/include/net/xfrm.h
> > > > @@ -2188,4 +2188,13 @@ static inline int register_xfrm_interface_bpf(void)
> > > >
> > > >  #endif
> > > >
> > > > +#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF)
> > > > +int register_xfrm_state_bpf(void);
> > > > +#else
> > > > +static inline int register_xfrm_state_bpf(void)
> > > > +{
> > > > +       return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > >  #endif /* _NET_XFRM_H */
> > > > diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
> > > > index cd47f88921f5..547cec77ba03 100644
> > > > --- a/net/xfrm/Makefile
> > > > +++ b/net/xfrm/Makefile
> > > > @@ -21,3 +21,4 @@ obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
> > > >  obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
> > > >  obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
> > > >  obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
> > > > +obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o
> > > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > > > index 5cdd3bca3637..62e64fa7ae5c 100644
> > > > --- a/net/xfrm/xfrm_policy.c
> > > > +++ b/net/xfrm/xfrm_policy.c
> > > > @@ -4267,6 +4267,8 @@ void __init xfrm_init(void)
> > > >  #ifdef CONFIG_XFRM_ESPINTCP
> > > >         espintcp_init();
> > > >  #endif
> > > > +
> > > > +       register_xfrm_state_bpf();
> > > >  }
> > > >
> > > >  #ifdef CONFIG_AUDITSYSCALL
> > > > diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
> > > > new file mode 100644
> > > > index 000000000000..a73a17a6497b
> > > > --- /dev/null
> > > > +++ b/net/xfrm/xfrm_state_bpf.c
> > > > @@ -0,0 +1,105 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/* Unstable XFRM state BPF helpers.
> > > > + *
> > > > + * 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/xdp.h>
> > > > +#include <net/xfrm.h>
> > > > +
> > > > +/* bpf_xfrm_state_opts - Options for XFRM state lookup helpers
> > > > + *
> > > > + * Members:
> > > > + * @error      - Out parameter, set for any errors encountered
> > > > + *              Values:
> > > > + *                -EINVAL - netns_id is less than -1
> > > > + *                -EINVAL - Passed NULL for opts
> > > > + *                -EINVAL - opts__sz isn't BPF_XFRM_STATE_OPTS_SZ
> > > > + *                -ENONET - No network namespace found for netns_id
> > > > + * @netns_id   - Specify the network namespace for lookup
> > > > + *              Values:
> > > > + *                BPF_F_CURRENT_NETNS (-1)
> > > > + *                  Use namespace associated with ctx
> > > > + *                [0, S32_MAX]
> > > > + *                  Network Namespace ID
> > > > + * @mark       - XFRM mark to match on
> > > > + * @daddr      - Destination address to match on
> > > > + * @spi                - Security parameter index to match on
> > > > + * @proto      - L3 protocol to match on
> > > > + * @family     - L3 protocol family to match on
> > > > + */
> > > > +struct bpf_xfrm_state_opts {
> > > > +       s32 error;
> > > > +       s32 netns_id;
> > > > +       u32 mark;
> > > > +       xfrm_address_t daddr;
> > > > +       __be32 spi;
> > > > +       u8 proto;
> > > > +       u16 family;
> > > > +};
> > > > +
> > > > +enum {
> > > > +       BPF_XFRM_STATE_OPTS_SZ = sizeof(struct bpf_xfrm_state_opts),
> > > > +};
> > > > +
> > > > +__diag_push();
> > > > +__diag_ignore_all("-Wmissing-prototypes",
> > > > +                 "Global functions as their definitions will be in xfrm_state BTF");
> > > > +
> > > > +/* bpf_xdp_get_xfrm_state - Get XFRM state
> > > > + *
> > > > + * Parameters:
> > > > + * @ctx        - Pointer to ctx (xdp_md) in XDP program
> > > > + *                 Cannot be NULL
> > > > + * @opts       - Options for lookup (documented above)
> > > > + *                 Cannot be NULL
> > > > + * @opts__sz   - Length of the bpf_xfrm_state_opts structure
> > > > + *                 Must be BPF_XFRM_STATE_OPTS_SZ
> > > > + */
> > > > +__bpf_kfunc struct xfrm_state *
> > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts, u32 opts__sz)
> > > > +{
> > > > +       struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> > > > +       struct net *net = dev_net(xdp->rxq->dev);
> > > > +
> > > > +       if (!opts || opts__sz != BPF_XFRM_STATE_OPTS_SZ) {
> > > > +               opts->error = -EINVAL;
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS)) {
> > > > +               opts->error = -EINVAL;
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       if (opts->netns_id >= 0) {
> > > > +               net = get_net_ns_by_id(net, opts->netns_id);
> > > > +               if (unlikely(!net)) {
> > > > +                       opts->error = -ENONET;
> > > > +                       return NULL;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi,
> > > > +                                opts->proto, opts->family);
> > > > +}
> > >
> > > Patch 6 example does little to explain how this kfunc can be used.
> > > Cover letter sounds promising, but no code to demonstrate the result.
> >
> > Part of the reason for that is this kfunc is intended to be used with a
> > not-yet-upstreamed xfrm patchset. The other is that the usage is quite
> > trivial. This is the code the experiments were run with:
> >
> > https://github.com/danobi/xdp-tools/blob/e89a1c617aba3b50d990f779357d6ce2863ecb27/xdp-bench/xdp_redirect_cpumap.bpf.c#L385-L406
> >
> > We intend to upstream that cpumap mode to xdp-tools as soon as the xfrm
> > patches are in. (Note the linked code is a little buggy but the
> > main idea is there).
> 
> I don't understand how it survives anything, but sanity check.
> To measure perf gains it needs to be under traffic for some time,
> but
> x = bpf_xdp_get_xfrm_state(ctx, &opts, sizeof(opts));
> will keep refcnt++ that state for every packet.
> Minimum -> memory leak or refcnt overflow.

Yeah, I agree the code in this patchset is not correct. I have the fix
(a KF_RELEASE wrapper around xfrm_state_put()) ready to send. I think
Steffen was gonna chat w/ you about this at IETF next week. But I can
send it now if you'd like.

To answer your question why it doesn't blow up immediately:

* The test system only has ~33 inbound SAs and the test doesn't try to
  delete any. So leak is not noticed in the test. Oddly enough I recall
  `ip x s flush` working correctly... Could be misremembering.

* Refcnt overflow will indeed happen, but some rough math shows it'll
  take about 12 hrs receiving at 100Gbps for that to happen. 100Gbps =
  12.5 GB/s. 12.5GB / (32 CPUs) / (9000B) = 43k pps for each pcpu SA.
  INT_MAX = 2 billion. 2B / 4k = 46k. 46k seconds to hours is ~12 hrs.
  And I was only running traffic for ~1 hour.

At least I think that math is right.

Thanks,
Daniel
Alexei Starovoitov Nov. 1, 2023, 6:51 p.m. UTC | #5 
On Wed, Nov 1, 2023 at 10:51 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Yeah, I agree the code in this patchset is not correct. I have the fix
> (a KF_RELEASE wrapper around xfrm_state_put()) ready to send. I think
> Steffen was gonna chat w/ you about this at IETF next week. But I can
> send it now if you'd like.

I say send a new version with all issues addressed now, since
it might help to frame the discussion at IETF.

>
> To answer your question why it doesn't blow up immediately:
>
> * The test system only has ~33 inbound SAs and the test doesn't try to
>   delete any. So leak is not noticed in the test. Oddly enough I recall
>   `ip x s flush` working correctly... Could be misremembering.
>
> * Refcnt overflow will indeed happen, but some rough math shows it'll
>   take about 12 hrs receiving at 100Gbps for that to happen. 100Gbps =
>   12.5 GB/s. 12.5GB / (32 CPUs) / (9000B) = 43k pps for each pcpu SA.
>   INT_MAX = 2 billion. 2B / 4k = 46k. 46k seconds to hours is ~12 hrs.
>   And I was only running traffic for ~1 hour.
>
> At least I think that math is right.

Makes sense.
diff mbox series

Patch

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 98d7aa78adda..ab4cf66480f3 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -2188,4 +2188,13 @@  static inline int register_xfrm_interface_bpf(void)
 
 #endif
 
+#if IS_ENABLED(CONFIG_DEBUG_INFO_BTF)
+int register_xfrm_state_bpf(void);
+#else
+static inline int register_xfrm_state_bpf(void)
+{
+	return 0;
+}
+#endif
+
 #endif	/* _NET_XFRM_H */
diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
index cd47f88921f5..547cec77ba03 100644
--- a/net/xfrm/Makefile
+++ b/net/xfrm/Makefile
@@ -21,3 +21,4 @@  obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
 obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
 obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
 obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
+obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 5cdd3bca3637..62e64fa7ae5c 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4267,6 +4267,8 @@  void __init xfrm_init(void)
 #ifdef CONFIG_XFRM_ESPINTCP
 	espintcp_init();
 #endif
+
+	register_xfrm_state_bpf();
 }
 
 #ifdef CONFIG_AUDITSYSCALL
diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
new file mode 100644
index 000000000000..a73a17a6497b
--- /dev/null
+++ b/net/xfrm/xfrm_state_bpf.c
@@ -0,0 +1,105 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Unstable XFRM state BPF helpers.
+ *
+ * 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/xdp.h>
+#include <net/xfrm.h>
+
+/* bpf_xfrm_state_opts - Options for XFRM state lookup helpers
+ *
+ * Members:
+ * @error      - Out parameter, set for any errors encountered
+ *		 Values:
+ *		   -EINVAL - netns_id is less than -1
+ *		   -EINVAL - Passed NULL for opts
+ *		   -EINVAL - opts__sz isn't BPF_XFRM_STATE_OPTS_SZ
+ *		   -ENONET - No network namespace found for netns_id
+ * @netns_id	- Specify the network namespace for lookup
+ *		 Values:
+ *		   BPF_F_CURRENT_NETNS (-1)
+ *		     Use namespace associated with ctx
+ *		   [0, S32_MAX]
+ *		     Network Namespace ID
+ * @mark	- XFRM mark to match on
+ * @daddr	- Destination address to match on
+ * @spi		- Security parameter index to match on
+ * @proto	- L3 protocol to match on
+ * @family	- L3 protocol family to match on
+ */
+struct bpf_xfrm_state_opts {
+	s32 error;
+	s32 netns_id;
+	u32 mark;
+	xfrm_address_t daddr;
+	__be32 spi;
+	u8 proto;
+	u16 family;
+};
+
+enum {
+	BPF_XFRM_STATE_OPTS_SZ = sizeof(struct bpf_xfrm_state_opts),
+};
+
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "Global functions as their definitions will be in xfrm_state BTF");
+
+/* bpf_xdp_get_xfrm_state - Get XFRM state
+ *
+ * Parameters:
+ * @ctx 	- Pointer to ctx (xdp_md) in XDP program
+ *		    Cannot be NULL
+ * @opts	- Options for lookup (documented above)
+ *		    Cannot be NULL
+ * @opts__sz	- Length of the bpf_xfrm_state_opts structure
+ *		    Must be BPF_XFRM_STATE_OPTS_SZ
+ */
+__bpf_kfunc struct xfrm_state *
+bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts, u32 opts__sz)
+{
+	struct xdp_buff *xdp = (struct xdp_buff *)ctx;
+	struct net *net = dev_net(xdp->rxq->dev);
+
+	if (!opts || opts__sz != BPF_XFRM_STATE_OPTS_SZ) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+
+	if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS)) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+
+	if (opts->netns_id >= 0) {
+		net = get_net_ns_by_id(net, opts->netns_id);
+		if (unlikely(!net)) {
+			opts->error = -ENONET;
+			return NULL;
+		}
+	}
+
+	return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi,
+				 opts->proto, opts->family);
+}
+
+__diag_pop()
+
+BTF_SET8_START(xfrm_state_kfunc_set)
+BTF_ID_FLAGS(func, bpf_xdp_get_xfrm_state, KF_RET_NULL)
+BTF_SET8_END(xfrm_state_kfunc_set)
+
+static const struct btf_kfunc_id_set xfrm_state_xdp_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set   = &xfrm_state_kfunc_set,
+};
+
+int __init register_xfrm_state_bpf(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP,
+					 &xfrm_state_xdp_kfunc_set);
+}