| Message ID | 0a5dc090a098b911bdd19ed0e63c7e466f7054f6.1698875025.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | Add bpf_xdp_get_xfrm_state() kfunc | expand |
On Wed, Nov 1, 2023 at 2:58 PM 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. > > Co-developed-by: Antony Antony <antony.antony@secunet.com> > Signed-off-by: Antony Antony <antony.antony@secunet.com> > 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 c9bb0f892f55..1d107241b901 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -2190,4 +2190,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 c13dc3ef7910..1b7e75159727 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -4218,6 +4218,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..4aaac134b97a > --- /dev/null > +++ b/net/xfrm/xfrm_state_bpf.c since net/xfrm/xfrm_interface_bpf.c is already there and was meant to be use as a file for interface between xfrm and bpf may be add new kfuncs there instead of new file? > @@ -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); netns is leaking :( > + if (unlikely(!net)) { > + opts->error = -ENONET; > + return NULL; > + } > + } > + > + return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi, > + opts->proto, opts->family); After looking into xfrm internals realized that refcnt inc/dec and KF_ACQUIRE maybe unnecessary overhead. XDP progs run under rcu_read_lock. I think you can make a version of __xfrm_state_lookup() without xfrm_state_hold_rcu() and avoid two atomics per packet, but such xfrm_state may have refcnt==0. Since bpf prog will only read from there and won't pass it anywhere else it might be ok. But considering the rest of ipsec overhead this might be a premature optimization and it's better to stay with clean acquire/release semantics. As far as IETF: https://datatracker.ietf.org/doc/html/draft-ietf-ipsecme-multi-sa-performance-02 it's not clear to me why one Child SA (without new pcpu field) has to be handled by one cpu. Sounds like it's possible to implement differently. At least in SW. In HW, I can see how duplicating multiple crypto state and the rest in a single queue is difficult.
On Wed, Nov 01, 2023 at 06:27:03PM -0700, Alexei Starovoitov wrote: > On Wed, Nov 1, 2023 at 2:58 PM 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. > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com> > > Signed-off-by: Antony Antony <antony.antony@secunet.com> > > 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 c9bb0f892f55..1d107241b901 100644 > > --- a/include/net/xfrm.h > > +++ b/include/net/xfrm.h > > @@ -2190,4 +2190,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 c13dc3ef7910..1b7e75159727 100644 > > --- a/net/xfrm/xfrm_policy.c > > +++ b/net/xfrm/xfrm_policy.c > > @@ -4218,6 +4218,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..4aaac134b97a > > --- /dev/null > > +++ b/net/xfrm/xfrm_state_bpf.c > > since net/xfrm/xfrm_interface_bpf.c is already there and > was meant to be use as a file for interface between xfrm and bpf > may be add new kfuncs there instead of new file? IIUC xfrm_interface_bpf.c is for "xfrm interfaces" [0]. See bpf_xfrm_info:if_id. I could be wrong. But if you or Steffen want it all in one file I don't really mind either way. [0]: https://docs.strongswan.org/docs/5.9/features/routeBasedVpn.html#_xfrm_interfaces_on_linux > > > > @@ -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); > > netns is leaking :( Argh, will fix. > > > + if (unlikely(!net)) { > > + opts->error = -ENONET; > > + return NULL; > > + } > > + } > > + > > + return xfrm_state_lookup(net, opts->mark, &opts->daddr, opts->spi, > > + opts->proto, opts->family); > > After looking into xfrm internals realized that > refcnt inc/dec and KF_ACQUIRE maybe unnecessary overhead. > XDP progs run under rcu_read_lock. > I think you can make a version of __xfrm_state_lookup() > without xfrm_state_hold_rcu() and avoid two atomics per packet, > but such xfrm_state may have refcnt==0. > Since bpf prog will only read from there and won't pass it anywhere > else it might be ok. > > But considering the rest of ipsec overhead this might be > a premature optimization and it's better to stay with clean > acquire/release semantics. Yeah I think acquire/release will be fine for now. I've been doing a lot of profiling on tput tests and xdp barely shows up. There are some lower hanging fruit atm. > As far as IETF: > https://datatracker.ietf.org/doc/html/draft-ietf-ipsecme-multi-sa-performance-02 > it's not clear to me why one Child SA (without new pcpu field) > has to be handled by one cpu. > > Sounds like it's possible to implement differently. At least in SW. > In HW, I can see how duplicating multiple crypto state and the rest > in a single queue is difficult. The child SA without pcpu flag (aka fallback SA) is not pinned to a cpu. It's shared between all of the cpus. I think Steffen can probably explain this better if that's not clear. Thanks, Daniel
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index c9bb0f892f55..1d107241b901 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -2190,4 +2190,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 c13dc3ef7910..1b7e75159727 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -4218,6 +4218,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..4aaac134b97a --- /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 | KF_ACQUIRE) +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); +}