mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Daniel Zahka" <daniel.zahka@gmail.com>
To: "Todd Freed" <toddfreed@meta.com>, <netdev@vger.kernel.org>
Cc: "Daniel Zahka" <daniel.zahka@gmail.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Willem de Bruijn" <willemdebruijn.kernel@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Simon Horman" <horms@kernel.org>,
	"Andrew Lunn" <andrew+netdev@lunn.ch>,
	"Shuah Khan" <shuah@kernel.org>, "Raed Salem" <raeds@nvidia.com>,
	"Rahul Rameshbabu" <rrameshbabu@nvidia.com>,
	"Cosmin Ratiu" <cratiu@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	"Björn Töpel" <bjorn@kernel.org>
Subject: Re: [PATCH net v2 1/2] psp: fix transport mode encap with ipv6 extension headers
Date: Fri, 23 Oct 2026 07:23:55 -0400	[thread overview]
Message-ID: <DMC6H9OT57RA.30RKA7CO0856R@gmail.com> (raw)
In-Reply-To: <20261022223014.556388-2-toddfreed@meta.com>

On Thu Oct 22, 2026 at 6:30 PM EDT, Todd Freed wrote:
> psp_dev_encapsulate() inserts the udp and psp headers and rewrites the
> l3 next header to udp.
>
> if the packet is an ipv6 packet with extension headers, the transport
> header comes after them, so rewriting the next header of the ipv6 fixed
> header produces a malformed packet.
>
> a psp tcp socket emits such packets if certain socket options are set on
> it (e.g. IPV6_DSTOPTS). with mlx5, these packets also go out
> unencrypted.
>
> fix by walking the extension header chain to rewrite the nexthdr on the
> last one in the chain instead. the walk looks at the extension header
> types only to find their lengths, and to reject these,
>
> - NEXTHDR_FRAGMENT. this could work, but udp+psp would need to come
>   before it, which is different than how the other option types are
>   handled. for now, keep fragments unsupported for transport mode.
>
> - NEXTHDR_ROUTING. the tcp checksum covers the final destination, but
>   drivers seed tso checksums from the daddr in the fixed hdr
>   (mlx5e_psp_handle_tx_skb()), so this would produce bad checksums.
>   could be supported with more work.
>

The stack should seed the tcp check correctly with the sk dst addr. If
mlx5's lso implementation can't take that into account, I'm not sure a
generic helper should be the one rejecting the skb.

> - NEXTHDR_NONE, and types which ipv6_ext_hdr() doesn't know.
>
> on rx, the inverse process is performed in psp_dev_rcv(). transport mode
> psp fragments are rejected on this path as before.
>
> note that the stack doesn't fragment ipv6 tcp packets, so a psp socket
> won't be able to exercise the tx fragment path.
>
> Suggested-by: Daniel Zahka <daniel.zahka@gmail.com>
> Fixes: fc724515741a ("psp: provide encapsulation helper for drivers")
> Fixes: 0eddb8023cee ("psp: provide decapsulation and receive helper for drivers")
> Signed-off-by: Todd Freed <toddfreed@meta.com>
> ---
>  net/psp/psp_main.c | 78 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index c9c1a8826b7f..0c009d7b08c0 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -4,6 +4,7 @@
>  #include <linux/list.h>
>  #include <linux/netdevice.h>
>  #include <linux/xarray.h>
> +#include <net/ipv6.h>
>  #include <net/net_namespace.h>
>  #include <net/psp.h>
>  #include <net/udp.h>
> @@ -185,6 +186,42 @@ unsigned int psp_key_size(u32 version)
>  }
>  EXPORT_SYMBOL(psp_key_size);
>  
> +/**
> + * psp_ipv6_nexthdr_off() - find the offset to the l4 proto nexthdr
> + * @hdr:  ipv6 fixed header
> + * @len:  l3 header total length
> + *
> + * Return: offset from @hdr, or -1 to drop the packet
> + */
> +static int psp_ipv6_nexthdr_off(const u8 *hdr, unsigned int len)
> +{
> +	unsigned int nh = offsetof(struct ipv6hdr, nexthdr);
> +	unsigned int off = sizeof(struct ipv6hdr);
> +	const struct ipv6_opt_hdr *eh;
> +	unsigned int rem;
> +	u8 type;
> +
> +	while (off < len) {
> +		eh = (const void *)(hdr + off);
> +		type = hdr[nh];
> +
> +		if (!ipv6_ext_hdr(type) || type == NEXTHDR_NONE ||
> +		    type == NEXTHDR_FRAGMENT || type == NEXTHDR_ROUTING)
> +			return -1;

return an error code instead of -1?

> +		rem = len - off;
> +		if (rem < sizeof(*eh))
> +			return -1;
> +
> +		nh = off + offsetof(struct ipv6_opt_hdr, nexthdr);
> +		if (type == NEXTHDR_AUTH)
> +			off += ipv6_authlen(eh);

I was expecting auth header to be part of the reject list.

> +		else
> +			off += ipv6_optlen(eh);
> +	}
> +
> +	return off == len ? nh : -1;
> +}
> +
>  static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
>  			      u8 ver, unsigned int udp_len, __be16 sport)
>  {
> @@ -243,7 +280,7 @@ static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
>  }
>  
>  /* Encapsulate a TCP packet with PSP by adding the UDP+PSP headers and filling
> - * them in.
> + * them in. With IPv6, they go after any extension headers.
>   */
>  bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  			 u8 ver, __be16 sport)
> @@ -251,10 +288,17 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  	u32 network_len = skb_network_header_len(skb);
>  	u32 ethr_len = skb_mac_header_len(skb);
>  	u32 bufflen = ethr_len + network_len;
> -
> -	if (skb->protocol != htons(ETH_P_IP) &&
> -	    skb->protocol != htons(ETH_P_IPV6))
> +	int nh_off = 0;
> +
> +	if (skb->protocol == htons(ETH_P_IPV6)) {
> +		/* UDP + PSP after the extension headers, if any */
> +		nh_off = psp_ipv6_nexthdr_off(skb_network_header(skb),
> +					      network_len);
> +		if (nh_off < 0)
> +			return false;
> +	} else if (skb->protocol != htons(ETH_P_IP)) {
>  		return false;
> +	}
>  
>  	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
>  		return false;
> @@ -272,7 +316,7 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  		ip_hdr(skb)->check =
>  			ip_fast_csum((u8 *)ip_hdr(skb), ip_hdr(skb)->ihl);
>  	} else {
> -		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
> +		skb_network_header(skb)[nh_off] = IPPROTO_UDP;
>  		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
>  	}
>  
> @@ -331,8 +375,23 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
>  			return -EINVAL;

If you send a v3, I think you should put the psp_dev_rcv() in a
serparate patch.

>  	} else {
>  		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
> +		u8 nexthdr = ipv6h->nexthdr;
> +		__be16 frag_off;
> +		int off;
>  
> -		is_udp = ipv6h->nexthdr == IPPROTO_UDP;
> +		/* UDP + PSP come after any extension headers. Fragment and
> +		 * routing headers are not supported in transport mode.
> +		 */
> +		off = ipv6_skip_exthdr(skb, l2_hlen + sizeof(*ipv6h), &nexthdr,
> +				       &frag_off);
> +		if (unlikely(off < 0 || frag_off))
> +			return -EINVAL;
> +
> +		is_udp = nexthdr == IPPROTO_UDP;
> +		l3_hlen = off - l2_hlen;
> +		if (l3_hlen != sizeof(struct ipv6hdr) &&
> +		    !pskb_may_pull(skb, l2_hlen + l3_hlen + PSP_ENCAP_HLEN))
> +			return -EINVAL;
>  	}
>  
>  	if (unlikely(!is_udp))
> @@ -386,11 +445,14 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
>  		iph->check = ip_fast_csum((u8 *)iph, iph->ihl);
>  	} else {
>  		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
> +		int nh_off = psp_ipv6_nexthdr_off((u8 *)ipv6h, l3_hlen);
>  
> -		if (unlikely(ntohs(ipv6h->payload_len) < encap))
> +		if (unlikely(nh_off < 0 ||
> +			     ntohs(ipv6h->payload_len) <
> +			     l3_hlen - sizeof(*ipv6h) + encap))
>  			return -EINVAL;
>  
> -		ipv6h->nexthdr = psph->nexthdr;
> +		((u8 *)ipv6h)[nh_off] = psph->nexthdr;
>  		ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) - encap);
>  	}
>  


  reply	other threads:[~2026-10-23 11:23 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-22 22:30 [PATCH net v2 0/2] psp: fix transport mode " Todd Freed
2026-10-22 22:30 ` [PATCH net v2 1/2] psp: fix transport mode encap " Todd Freed
2026-10-23 11:23   ` Daniel Zahka [this message]
2026-10-23 17:20     ` Todd Bert Freed
2026-10-22 22:30 ` [PATCH net v2 2/2] selftests: drv-net: psp: test ipv6 tcp socket with " Todd Freed
2026-10-23 11:44   ` Daniel Zahka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DMC6H9OT57RA.30RKA7CO0856R@gmail.com \
    --to=daniel.zahka@gmail.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=bjorn@kernel.org \
    --cc=cratiu@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@kernel.org \
    --cc=horms@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=raeds@nvidia.com \
    --cc=rrameshbabu@nvidia.com \
    --cc=shuah@kernel.org \
    --cc=toddfreed@meta.com \
    --cc=willemdebruijn.kernel@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®