| Message ID | 20240714202246.1573817-9-chopps@chopps.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add IP-TFS mode to xfrm | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Sun, Jul 14, 2024 at 04:22:36PM -0400, Christian Hopps wrote: > From: Christian Hopps <chopps@labn.net> > > Add a new xfrm mode implementing AggFrag/IP-TFS from RFC9347. > > This utilizes the new xfrm_mode_cbs to implement demand-driven IP-TFS > functionality. This functionality can be used to increase bandwidth > utilization through small packet aggregation, as well as help solve PMTU > issues through it's efficient use of fragmentation. > > Link: https://www.rfc-editor.org/rfc/rfc9347.txt > > Multiple commits follow to build the functionality into xfrm_iptfs.c > > Signed-off-by: Christian Hopps <chopps@labn.net> > --- > net/xfrm/Makefile | 1 + > net/xfrm/xfrm_iptfs.c | 206 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 207 insertions(+) > create mode 100644 net/xfrm/xfrm_iptfs.c > > diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile > index 512e0b2f8514..5a1787587cb3 100644 > --- a/net/xfrm/Makefile > +++ b/net/xfrm/Makefile > @@ -21,5 +21,6 @@ obj-$(CONFIG_XFRM_USER) += xfrm_user.o > 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_IPTFS) += xfrm_iptfs.o > obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o > obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o > diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c > new file mode 100644 > index 000000000000..414035a7a208 > --- /dev/null > +++ b/net/xfrm/xfrm_iptfs.c > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* xfrm_iptfs: IPTFS encapsulation support > + * > + * April 21 2022, Christian Hopps <chopps@labn.net> > + * > + * Copyright (c) 2022, LabN Consulting, L.L.C. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/icmpv6.h> > +#include <net/gro.h> > +#include <net/icmp.h> > +#include <net/ip6_route.h> > +#include <net/inet_ecn.h> > +#include <net/xfrm.h> > + > +#include <crypto/aead.h> > + > +#include "xfrm_inout.h" > + > +struct xfrm_iptfs_config { > + u32 pkt_size; /* outer_packet_size or 0 */ > +}; > + > +struct xfrm_iptfs_data { > + struct xfrm_iptfs_config cfg; > + > + /* Ingress User Input */ > + struct xfrm_state *x; /* owning state */ > + u32 payload_mtu; /* max payload size */ > +}; > + > +/* ========================== */ > +/* State Management Functions */ > +/* ========================== */ > + > +/** > + * iptfs_get_inner_mtu() - return inner MTU with no fragmentation. > + * @x: xfrm state. > + * @outer_mtu: the outer mtu > + */ Hi Christian, Please consider including a "Return:" or "Returns:" section in the Kernel doc for functions that return values. Likewise elsewhere in this patchset. Flagged by: ./scripts/kernel-doc -none -Wall > +static u32 iptfs_get_inner_mtu(struct xfrm_state *x, int outer_mtu) ...
Simon Horman via Devel <devel@linux-ipsec.org> writes: > On Sun, Jul 14, 2024 at 04:22:36PM -0400, Christian Hopps wrote: >> From: Christian Hopps <chopps@labn.net> >> >> Add a new xfrm mode implementing AggFrag/IP-TFS from RFC9347. >> >> This utilizes the new xfrm_mode_cbs to implement demand-driven IP-TFS >> functionality. This functionality can be used to increase bandwidth >> utilization through small packet aggregation, as well as help solve PMTU >> issues through it's efficient use of fragmentation. >> >> Link: https://www.rfc-editor.org/rfc/rfc9347.txt >> >> Multiple commits follow to build the functionality into xfrm_iptfs.c >> >> Signed-off-by: Christian Hopps <chopps@labn.net> >> --- >> net/xfrm/Makefile | 1 + >> net/xfrm/xfrm_iptfs.c | 206 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 207 insertions(+) >> create mode 100644 net/xfrm/xfrm_iptfs.c >> >> diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile >> index 512e0b2f8514..5a1787587cb3 100644 >> --- a/net/xfrm/Makefile >> +++ b/net/xfrm/Makefile >> @@ -21,5 +21,6 @@ obj-$(CONFIG_XFRM_USER) += xfrm_user.o >> 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_IPTFS) += xfrm_iptfs.o >> obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o >> obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o >> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c >> new file mode 100644 >> index 000000000000..414035a7a208 >> --- /dev/null >> +++ b/net/xfrm/xfrm_iptfs.c >> @@ -0,0 +1,206 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* xfrm_iptfs: IPTFS encapsulation support >> + * >> + * April 21 2022, Christian Hopps <chopps@labn.net> >> + * >> + * Copyright (c) 2022, LabN Consulting, L.L.C. >> + * >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/icmpv6.h> >> +#include <net/gro.h> >> +#include <net/icmp.h> >> +#include <net/ip6_route.h> >> +#include <net/inet_ecn.h> >> +#include <net/xfrm.h> >> + >> +#include <crypto/aead.h> >> + >> +#include "xfrm_inout.h" >> + >> +struct xfrm_iptfs_config { >> + u32 pkt_size; /* outer_packet_size or 0 */ >> +}; >> + >> +struct xfrm_iptfs_data { >> + struct xfrm_iptfs_config cfg; >> + >> + /* Ingress User Input */ >> + struct xfrm_state *x; /* owning state */ >> + u32 payload_mtu; /* max payload size */ >> +}; >> + >> +/* ========================== */ >> +/* State Management Functions */ >> +/* ========================== */ >> + >> +/** >> + * iptfs_get_inner_mtu() - return inner MTU with no fragmentation. >> + * @x: xfrm state. >> + * @outer_mtu: the outer mtu >> + */ > > Hi Christian, > > Please consider including a "Return:" or "Returns:" section > in the Kernel doc for functions that return values. > > Likewise elsewhere in this patchset. > > Flagged by: ./scripts/kernel-doc -none -Wall > >> +static u32 iptfs_get_inner_mtu(struct xfrm_state *x, int outer_mtu) > > ... I've updated all the doc comments to include "Return:"s. Thanks! Chris.
Christian Hopps via Devel <devel@linux-ipsec.org> wrote: > +static int iptfs_clone(struct xfrm_state *x, struct xfrm_state *orig) > +{ > + struct xfrm_iptfs_data *xtfs; > + int err; > + > + xtfs = kmemdup(orig->mode_data, sizeof(*xtfs), GFP_KERNEL); > + if (!xtfs) > + return -ENOMEM; > + > + err = __iptfs_init_state(x, xtfs); > + if (err) > + return err; Missing kfree on err? > + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); > + if (!xtfs) > + return -ENOMEM; > + > + err = __iptfs_init_state(x, xtfs); > + if (err) > + return err; Likewise.
Florian Westphal <fw@strlen.de> writes: > Christian Hopps via Devel <devel@linux-ipsec.org> wrote: >> +static int iptfs_clone(struct xfrm_state *x, struct xfrm_state *orig) >> +{ >> + struct xfrm_iptfs_data *xtfs; >> + int err; >> + >> + xtfs = kmemdup(orig->mode_data, sizeof(*xtfs), GFP_KERNEL); >> + if (!xtfs) >> + return -ENOMEM; >> + >> + err = __iptfs_init_state(x, xtfs); >> + if (err) >> + return err; > > Missing kfree on err? > >> + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); >> + if (!xtfs) >> + return -ENOMEM; >> + >> + err = __iptfs_init_state(x, xtfs); >> + if (err) >> + return err; > > Likewise. These originally counted on the fact that the xfrm state is cleaned up on error; however, a later code change did indeed allow for the leak -- added kfree's as suggested. Thanks, Chris.
2024-07-14, 16:22:36 -0400, Christian Hopps wrote: > +struct xfrm_iptfs_config { > + u32 pkt_size; /* outer_packet_size or 0 */ Please convert this to kdoc. > +}; > + > +struct xfrm_iptfs_data { > + struct xfrm_iptfs_config cfg; > + > + /* Ingress User Input */ > + struct xfrm_state *x; /* owning state */ And this too. > + u32 payload_mtu; /* max payload size */ > +}; > +static int iptfs_create_state(struct xfrm_state *x) > +{ > + struct xfrm_iptfs_data *xtfs; > + int err; > + > + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); > + if (!xtfs) > + return -ENOMEM; > + > + err = __iptfs_init_state(x, xtfs); > + if (err) > + return err; BTW, I wrote that this was leaking xtfs in my previous review, back in March :/
Sabrina Dubroca <sd@queasysnail.net> writes: > 2024-07-14, 16:22:36 -0400, Christian Hopps wrote: >> +static int iptfs_create_state(struct xfrm_state *x) >> +{ >> + struct xfrm_iptfs_data *xtfs; >> + int err; >> + >> + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); >> + if (!xtfs) >> + return -ENOMEM; >> + >> + err = __iptfs_init_state(x, xtfs); >> + if (err) >> + return err; > > BTW, I wrote that this was leaking xtfs in my previous review, back in > March :/ You did indeed -- that was the review you asked for the commit split. I've marked unread that previous email now so I can go over it again and verify I didn't miss anything else. Thanks, Chris.
Sabrina Dubroca <sd@queasysnail.net> writes: > 2024-07-14, 16:22:36 -0400, Christian Hopps wrote: >> +struct xfrm_iptfs_config { >> + u32 pkt_size; /* outer_packet_size or 0 */ > > Please convert this to kdoc. Done. >> +}; >> + >> +struct xfrm_iptfs_data { >> + struct xfrm_iptfs_config cfg; >> + >> + /* Ingress User Input */ >> + struct xfrm_state *x; /* owning state */ > > And this too. Done. >> + u32 payload_mtu; /* max payload size */ >> +}; > > >> +static int iptfs_create_state(struct xfrm_state *x) >> +{ >> + struct xfrm_iptfs_data *xtfs; >> + int err; >> + >> + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); >> + if (!xtfs) >> + return -ENOMEM; >> + >> + err = __iptfs_init_state(x, xtfs); >> + if (err) >> + return err; > > BTW, I wrote that this was leaking xtfs in my previous review, back in > March :/ I went back through your earlier review again. The next patch set adds the queue flush/cleanup on state delete, and uses a new MIB direction independent MIB counter for failed skb alloc -- which you identified as well. Thanks, Chris.
diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile index 512e0b2f8514..5a1787587cb3 100644 --- a/net/xfrm/Makefile +++ b/net/xfrm/Makefile @@ -21,5 +21,6 @@ obj-$(CONFIG_XFRM_USER) += xfrm_user.o 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_IPTFS) += xfrm_iptfs.o obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_state_bpf.o diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c new file mode 100644 index 000000000000..414035a7a208 --- /dev/null +++ b/net/xfrm/xfrm_iptfs.c @@ -0,0 +1,206 @@ +// SPDX-License-Identifier: GPL-2.0 +/* xfrm_iptfs: IPTFS encapsulation support + * + * April 21 2022, Christian Hopps <chopps@labn.net> + * + * Copyright (c) 2022, LabN Consulting, L.L.C. + * + */ + +#include <linux/kernel.h> +#include <linux/icmpv6.h> +#include <net/gro.h> +#include <net/icmp.h> +#include <net/ip6_route.h> +#include <net/inet_ecn.h> +#include <net/xfrm.h> + +#include <crypto/aead.h> + +#include "xfrm_inout.h" + +struct xfrm_iptfs_config { + u32 pkt_size; /* outer_packet_size or 0 */ +}; + +struct xfrm_iptfs_data { + struct xfrm_iptfs_config cfg; + + /* Ingress User Input */ + struct xfrm_state *x; /* owning state */ + u32 payload_mtu; /* max payload size */ +}; + +/* ========================== */ +/* State Management Functions */ +/* ========================== */ + +/** + * iptfs_get_inner_mtu() - return inner MTU with no fragmentation. + * @x: xfrm state. + * @outer_mtu: the outer mtu + */ +static u32 iptfs_get_inner_mtu(struct xfrm_state *x, int outer_mtu) +{ + struct crypto_aead *aead; + u32 blksize; + + aead = x->data; + blksize = ALIGN(crypto_aead_blocksize(aead), 4); + return ((outer_mtu - x->props.header_len - crypto_aead_authsize(aead)) & + ~(blksize - 1)) - 2; +} + +/** + * iptfs_user_init() - initialize the SA with IPTFS options from netlink. + * @net: the net data + * @x: xfrm state + * @attrs: netlink attributes + * @extack: extack return data + */ +static int iptfs_user_init(struct net *net, struct xfrm_state *x, + struct nlattr **attrs, + struct netlink_ext_ack *extack) +{ + struct xfrm_iptfs_data *xtfs = x->mode_data; + struct xfrm_iptfs_config *xc; + + xc = &xtfs->cfg; + + if (attrs[XFRMA_IPTFS_PKT_SIZE]) { + xc->pkt_size = nla_get_u32(attrs[XFRMA_IPTFS_PKT_SIZE]); + if (!xc->pkt_size) { + xtfs->payload_mtu = 0; + } else if (xc->pkt_size > x->props.header_len) { + xtfs->payload_mtu = xc->pkt_size - x->props.header_len; + } else { + NL_SET_ERR_MSG(extack, + "Packet size must be 0 or greater than IPTFS/ESP header length"); + return -EINVAL; + } + } + return 0; +} + +static unsigned int iptfs_sa_len(const struct xfrm_state *x) +{ + struct xfrm_iptfs_data *xtfs = x->mode_data; + struct xfrm_iptfs_config *xc = &xtfs->cfg; + unsigned int l = 0; + + if (x->dir == XFRM_SA_DIR_OUT) + l += nla_total_size(sizeof(xc->pkt_size)); + + return l; +} + +static int iptfs_copy_to_user(struct xfrm_state *x, struct sk_buff *skb) +{ + struct xfrm_iptfs_data *xtfs = x->mode_data; + struct xfrm_iptfs_config *xc = &xtfs->cfg; + int ret = 0; + + if (x->dir == XFRM_SA_DIR_OUT) + ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size); + + return ret; +} + +static int __iptfs_init_state(struct xfrm_state *x, + struct xfrm_iptfs_data *xtfs) +{ + /* Modify type (esp) adjustment values */ + + if (x->props.family == AF_INET) + x->props.header_len += sizeof(struct iphdr) + sizeof(struct ip_iptfs_hdr); + else if (x->props.family == AF_INET6) + x->props.header_len += sizeof(struct ipv6hdr) + sizeof(struct ip_iptfs_hdr); + x->props.enc_hdr_len = sizeof(struct ip_iptfs_hdr); + + /* Always have a module reference if x->mode_data is set */ + if (!try_module_get(x->mode_cbs->owner)) + return -EINVAL; + + x->mode_data = xtfs; + xtfs->x = x; + + return 0; +} + +static int iptfs_clone(struct xfrm_state *x, struct xfrm_state *orig) +{ + struct xfrm_iptfs_data *xtfs; + int err; + + xtfs = kmemdup(orig->mode_data, sizeof(*xtfs), GFP_KERNEL); + if (!xtfs) + return -ENOMEM; + + err = __iptfs_init_state(x, xtfs); + if (err) + return err; + + return 0; +} + +static int iptfs_create_state(struct xfrm_state *x) +{ + struct xfrm_iptfs_data *xtfs; + int err; + + xtfs = kzalloc(sizeof(*xtfs), GFP_KERNEL); + if (!xtfs) + return -ENOMEM; + + err = __iptfs_init_state(x, xtfs); + if (err) + return err; + + return 0; +} + +static void iptfs_delete_state(struct xfrm_state *x) +{ + struct xfrm_iptfs_data *xtfs = x->mode_data; + + if (!xtfs) + return; + + kfree_sensitive(xtfs); + + module_put(x->mode_cbs->owner); +} + +static const struct xfrm_mode_cbs iptfs_mode_cbs = { + .owner = THIS_MODULE, + .create_state = iptfs_create_state, + .delete_state = iptfs_delete_state, + .user_init = iptfs_user_init, + .copy_to_user = iptfs_copy_to_user, + .sa_len = iptfs_sa_len, + .clone = iptfs_clone, + .get_inner_mtu = iptfs_get_inner_mtu, +}; + +static int __init xfrm_iptfs_init(void) +{ + int err; + + pr_info("xfrm_iptfs: IPsec IP-TFS tunnel mode module\n"); + + err = xfrm_register_mode_cbs(XFRM_MODE_IPTFS, &iptfs_mode_cbs); + if (err < 0) + pr_info("%s: can't register IP-TFS\n", __func__); + + return err; +} + +static void __exit xfrm_iptfs_fini(void) +{ + xfrm_unregister_mode_cbs(XFRM_MODE_IPTFS); +} + +module_init(xfrm_iptfs_init); +module_exit(xfrm_iptfs_fini); +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("IP-TFS support for xfrm ipsec tunnels");