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

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
esp: Consolidate esp4 and esp6

This commit merges the common code from esp4.c and esp6.c into xfrm_esp.c to
reduce duplication. It adapts the input and output encapsulation paths to
use indirect function calls.

> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index 9f75313734f8c..9d6e030510860 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
[ ... ]
> +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);
> +	struct xfrm_encap_tmpl *encap = x->encap;
> +	int hdr_len = skb_network_header_len(skb);
> +	u8 nexthdr = ip6h->nexthdr;
> +	__be16 frag_off, source;
> +	struct udphdr *uh;
> +	struct tcphdr *th;
> +	int ret;
>  
> +	ret = ipv6_skip_exthdr(skb, offset, &nexthdr, &frag_off);
> +	if (ret == -1)
> +		return ret;
>  
> +	ret += hdr_len;
>  
> +	uh = (void *)(skb->data + ret);
> +	th = (void *)(skb->data + ret);

If hdr_len is added to ret before computing the uh and th pointers, does this
cause the pointers to overshoot the transport header by hdr_len bytes?

If uh->source reads from inside the decrypted ESP payload instead of the actual
UDP header, it seems this would almost always mismatch encap->encap_sport.
Could this lead to the kernel unconditionally calling km_new_mapping() on
every received packet, breaking IPv6 NAT-T?

> diff --git a/net/xfrm/xfrm_esp.c b/net/xfrm/xfrm_esp.c
> new file mode 100644
> index 0000000000000..4f75e1ace3bbc
> --- /dev/null
> +++ b/net/xfrm/xfrm_esp.c
[ ... ]
> +#ifdef CONFIG_INET_ESPINTCP
> +static int esp_output_tcp_finish(struct xfrm_state *x, struct sk_buff *skb)
> +{
[ ... ]
> +#else
> +static int esp_output_tail_tcp(struct xfrm_state *x, struct sk_buff *skb)
> +{
> +	WARN_ON(1);
> +	return -EOPNOTSUPP;
> +}
> +#endif
[ ... ]
> +#ifdef CONFIG_INET_ESPINTCP
> +static struct ip_esp_hdr *esp_output_tcp_encap(struct xfrm_state *x,
> +					       struct sk_buff *skb,
> +					       struct esp_info *esp)
[ ... ]
> +#else
> +static struct ip_esp_hdr *esp_output_tcp_encap(struct xfrm_state *x,
> +					       struct sk_buff *skb,
> +					       struct esp_info *esp)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +#endif

Since the TCP encapsulation methods are guarded exclusively by
CONFIG_INET_ESPINTCP, does this break IPv6 ESP-in-TCP if
CONFIG_INET6_ESPINTCP is enabled but the IPv4 equivalent is disabled?

If CONFIG_INET_ESPINTCP is not enabled, these functions fall back to returning
-EOPNOTSUPP. Should these preprocessor guards check for both
CONFIG_INET_ESPINTCP and CONFIG_INET6_ESPINTCP?