Re: [PATCH RFC ipsec-next] esp: Consolidate esp4 and esp6.

[Sabrina Dubroca <sd@queasysnail.net>]

2025-10-22, 08:03:07 +0200, Steffen Klassert wrote:
> This patch merges common code of esp4.c and esp6.c into
> xfrm_esp.c. This almost halves the size of the ESP
> implementation for the prize of three indirect calls
> on UDP/TCP encapsulation.

If that turns out to be a problem, maybe we can figure out a way to
use the INDIRECT_CALL* helpers here as well (but currently they don't
work for modules, I played with that a while back and could look into
it again).

> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> Tested-by: Tobias Brunner <tobias@strongswan.org>
> ---
>  include/net/esp.h       |    6 +
>  include/net/xfrm.h      |    4 +
>  net/ipv4/esp4.c         | 1067 ++------------------------------------
>  net/ipv6/esp6.c         | 1093 +++------------------------------------
>  net/ipv6/esp6_offload.c |    6 +-
>  net/xfrm/Makefile       |    1 +
>  net/xfrm/xfrm_esp.c     | 1025 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 1156 insertions(+), 2046 deletions(-)

nice!


> -int esp_input_done2(struct sk_buff *skb, int err)
> -{
[...]
> +	if (iph->saddr != x->props.saddr.a4 ||
> +	    source != encap->encap_sport) {
> +		xfrm_address_t ipaddr;
> +
> +		ipaddr.a4 = iph->saddr;
> +		km_new_mapping(x, &ipaddr, source);
> +
> +		/* XXX: perhaps add an extra
> +		 * policy check here, to see
> +		 * if we should allow or
> +		 * reject a packet from a
> +		 * different source
> +		 * address/port.
>  		 */

Maybe we can get rid of those "XXX" comments? Unless you think the
suggestion still makes sense. But the comments (here and in
esp6_input_done2) have been here a long time and it doesn't seem to
bother users.

[...]
>  static int esp_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)

Looking at esp_init_state and esp6_init_state, they also have a lot in
common (pretty much everything except some x->props.header_len
adjustments) that could be extracted into the new file.

[...]
> +static int esp6_input_encap(struct sk_buff *skb, struct xfrm_state *x)
>  {
> +	const struct ipv6hdr *ip6h = ipv6_hdr(skb);
> +	int offset = skb_network_offset(skb) + sizeof(*ip6h);
> +	int hdr_len = skb_network_header_len(skb);

As Simon mentioned, it's set/incremented but not used. The current
code in esp6_input_done2 would then use that value in
skb_set_transport_header, but now the common esp_input_done2 calls
->input_encap and doesn't get that increased value back. I think we'll
need to have esp_input_done2 pass &hdr_len when it calls the
->input_encap to get the correct value and take into account ipv6
exthdrs.


[...]
> +static void esp_output_done(void *data, int err)
> +{
> +	struct sk_buff *skb = data;
> +	struct xfrm_offload *xo = xfrm_offload(skb);
> +	void *tmp;
> +	struct xfrm_state *x;
> +
> +	if (xo && (xo->flags & XFRM_DEV_RESUME)) {
> +		struct sec_path *sp = skb_sec_path(skb);
> +
> +		x = sp->xvec[sp->len - 1];
> +	} else {
> +		x = skb_dst(skb)->xfrm;
> +	}
> +
> +	tmp = ESP_SKB_CB(skb)->tmp;
> +	esp_ssg_unref(x, tmp, skb);
> +	kfree(tmp);
> +
> +	x->type->output_encap_csum(skb);

Since the ipv4 variant doesn't do anything, maybe

if (x->type->output_encap_csum)
	x->type->output_encap_csum(skb);

and don't set it in esp_type?

OTOH it's nice to have a consistent "all CBs are always defined" and
just call them unconditionally.



> +static struct ip_esp_hdr *esp_output_udp_encap(struct sk_buff *skb,
> +					       int encap_type,
> +					       struct esp_info *esp,
> +					       __be16 sport,
> +					       __be16 dport)
> +{
> +	struct udphdr *uh;
> +	unsigned int len;
> +	struct xfrm_offload *xo = xfrm_offload(skb);
> +
> +	len = skb->len + esp->tailen - skb_transport_offset(skb);
> +	if (len + sizeof(struct iphdr) > IP_MAX_MTU)

This is now used by both IPv4 and IPv6, and esp6_output_udp_encap
didn't have any adjustment in this test.

> +		return ERR_PTR(-EMSGSIZE);
> +
> +	uh = (struct udphdr *)esp->esph;
> +	uh->source = sport;
> +	uh->dest = dport;
> +	uh->len = htons(len);
> +	uh->check = 0;
> +
> +	/* For IPv4 ESP with UDP encapsulation, if xo is not null, the skb is in the crypto offload
> +	 * data path, which means that esp_output_udp_encap is called outside of the XFRM stack.
> +	 * In this case, the mac header doesn't point to the IPv4 protocol field, so don't set it.
> +	 */
> +	if (!xo || encap_type != UDP_ENCAP_ESPINUDP)
> +		*skb_mac_header(skb) = IPPROTO_UDP;

That was absent in esp6_output_udp_encap, commit 447bc4b1906f ("xfrm:
Support crypto offload for outbound IPv4 UDP-encapsulated ESP packet")
only took care of IPv4. I'm not sure adding this to the IPv6 code
without adapting the rest of 447bc4b1906f in the esp6_offload code is
correct.

> +
> +	return (struct ip_esp_hdr *)(uh + 1);
> +}

[...]
> + int esp_output(struct xfrm_state *x, struct sk_buff *skb)

nit: ' ' at the start of the line


-- 
Sabrina