[PATCH net v3 0/4] psp: fix transport mode with ipv6 extension headers

[PATCH net v3 0/4] psp: fix transport mode with ipv6 extension headers

[Todd Freed]

While working on adding support for socket-less psp associations, I
noticed that this case was not being handled correctly. It will be more
relevant with that series, because it adds udp support, for which the
psp nexthdr will be taken from the inner packet.

Since the tx association encodes intent to send encrypted packets, but
because of other configuration (socket options, routes) the packets are
sent unencrypted, sending this change as a fix.

The first patch fixes an mlx5e bug in tso checksum calculation for psp
with ipv6 routing headers.

The second patch fixes the receive path when ipv6 extension headers are
present.

The third patch fixes the send path when ipv6 extension headers are
present.

The fourth patch adds selftests for these cases.

Note that no test case is added for ipv6 fragment headers. As far as
I can tell it's not possible to tx fragments from an ipv6 tcp socket.

All testing was done on connectx-7.

v3:
 - new patch 1 to fix tso checksum seed calculation in mlx5e (Daniel)
 - accept routing headers and reject ah (Daniel)
 - split the psp_dev_rcv() change into its own patch (Daniel)
 - return -EINVAL instead of -1 (Daniel)
 - selftest: one variant per extension header type (Daniel)

v2: https://lore.kernel.org/netdev/20261022223014.556388-1-toddfreed@meta.com
 - walk the extension headers instead of dropping (Daniel)
 - reject fragment headers (Bjorn, Daniel)
 - reject routing headers
 - psp_dev_rcv() handle the rx case as well

v1: https://lore.kernel.org/netdev/20261021210200.788191-1-toddfreed@meta.com

Todd Freed (4):
  net/mlx5e: psp: tso: replace only the length in checksum seed
  psp: support ipv6 extension headers in psp_dev_rcv() decap
  psp: support ipv6 extension headers in psp_dev_encapsulate()
  selftests: drv-net: psp: test ipv6 tcp socket with extension headers

 .../mellanox/mlx5/core/en_accel/psp_rxtx.c    |  21 ++-
 net/psp/psp_main.c                            |  79 +++++++++-
 tools/testing/selftests/drivers/net/config    |   1 +
 tools/testing/selftests/drivers/net/psp.py    | 137 +++++++++++++++++-
 4 files changed, 215 insertions(+), 23 deletions(-)

-- 
2.53.0-Meta

[PATCH net v3 1/4] net/mlx5e: psp: tso: replace only the length in checksum seed

[Todd Freed]

for psp tcp with tso,

 - __tcp_transmit_skb() -> tcp_v6_send_check() or tcp_v4_send_check()
   computes the checksum seed using the addresses on the socket and the
   total length, then

 - mlx5e_psp_handle_tx_skb() recomputes the checksum seed using
   addresses from the packet's l3 header, and the length of one segment

the length difference is expected by the device. however, the
difference in addresses is incorrect in cases where the l3 header daddr
is an intermediate hop (e.g. IPV6_RTHDR). this causes every segment of
the tso packet to go out with a bad tcp checksum, in this case.

the fix is to replace only the length in the seed which tcp had already
computed. this is also what tcp_gso_segment() does.

tested on connectx-7 with the rest of this series applied, with a psp
tcp socket whose segment routing header's first hop isn't the final
destination, and the peer checking tcp checksums in software.

Fixes: e5a1861a298e ("net/mlx5e: Implement PSP Tx data path")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 .../mellanox/mlx5/core/en_accel/psp_rxtx.c    | 21 ++++++++-----------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
index 348fd7a96261..d9c2742b3c24 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
@@ -177,18 +177,15 @@ bool mlx5e_psp_handle_tx_skb(struct net_device *netdev,
 		return false;
 	}
 	if (skb_is_gso(skb)) {
-		int len = skb_shinfo(skb)->gso_size + inner_tcp_hdrlen(skb);
-		struct tcphdr *th = inner_tcp_hdr(skb);
-
-		if (skb->protocol == htons(ETH_P_IP)) {
-			const struct iphdr *ip = ip_hdr(skb);
-
-			th->check = ~tcp_v4_check(len, ip->saddr, ip->daddr, 0);
-		} else {
-			const struct ipv6hdr *ip6 = ipv6_hdr(skb);
-
-			th->check = ~tcp_v6_check(len, &ip6->saddr, &ip6->daddr, 0);
-		}
+		u32 seg_len = skb_shinfo(skb)->gso_size + inner_tcp_hdrlen(skb);
+		u32 len = skb->len - skb_inner_transport_offset(skb);
+
+		/* update just the length component of the checksum seed
+		 * tcp computed the seed with the total length, but mlx5
+		 * devices are expecting the segment length
+		 */
+		inet_proto_csum_replace4(&inner_tcp_hdr(skb)->check, skb,
+					 htonl(len), htonl(seg_len), true);
 	}
 
 	return true;
-- 
2.53.0-Meta

[PATCH net v3 2/4] psp: support ipv6 extension headers in psp_dev_rcv() decap

[Todd Freed]

psp_dev_rcv() expects the udp header to immediately follow the fixed
ipv6 header, so packets with ipv6 extension headers are dropped by the
is_udp check. additionally, on decap it writes the inner nexthdr into
the fixed header instead of into the last extension header.

fix by skipping the extension headers to find the udp header, and on
decap, setting the inner nexthdr on the last one.

fragment and authentication headers (ah) are not supported in psp
transport mode, so such packets are dropped.

Fixes: 0eddb8023cee ("psp: provide decapsulation and receive helper for drivers")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/psp/psp_main.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
index c9c1a8826b7f..946047440ed3 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,39 @@ 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 a negative value 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_AUTH)
+			return -EINVAL;
+		rem = len - off;
+		if (rem < sizeof(*eh))
+			return -EINVAL;
+
+		nh = off + offsetof(struct ipv6_opt_hdr, nexthdr);
+		off += ipv6_optlen(eh);
+	}
+
+	return off == len ? nh : -EINVAL;
+}
+
 static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
 			      u8 ver, unsigned int udp_len, __be16 sport)
 {
@@ -303,6 +337,7 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
 	struct psphdr *psph;
 	struct ethhdr *eth;
 	struct udphdr *uh;
+	int nh_off = 0;
 	__be16 proto;
 	bool is_udp;
 
@@ -331,8 +366,28 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
 			return -EINVAL;
 	} else {
 		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
+		u8 nexthdr = ipv6h->nexthdr;
+		__be16 frag_off;
+		int off;
+
+		/* UDP + PSP come after any extension headers */
+		off = ipv6_skip_exthdr(skb, l2_hlen + sizeof(*ipv6h), &nexthdr,
+				       &frag_off);
+		if (unlikely(off < 0))
+			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;
 
-		is_udp = ipv6h->nexthdr == IPPROTO_UDP;
+		/* fragment and authentication headers are not supported in
+		 * transport mode
+		 */
+		nh_off = psp_ipv6_nexthdr_off(skb->data + l2_hlen, l3_hlen);
+		if (unlikely(nh_off < 0))
+			return -EINVAL;
 	}
 
 	if (unlikely(!is_udp))
@@ -387,10 +442,11 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
 	} else {
 		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
 
-		if (unlikely(ntohs(ipv6h->payload_len) < encap))
+		if (unlikely(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);
 	}
 
-- 
2.53.0-Meta

[PATCH net v3 3/4] psp: support ipv6 extension headers in psp_dev_encapsulate()

[Todd Freed]

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 also rejects packets with
extension headers that aren't supported in transport mode,

- 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_AUTH. changing what follows would break the AH ICV. Also, AH
  adds little incremental value when used with psp, whose ICV already
  authenticates the payload. AH remains unsupported for psp transport
  mode.

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")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/psp/psp_main.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
index 946047440ed3..e6d2136cb9a7 100644
--- a/net/psp/psp_main.c
+++ b/net/psp/psp_main.c
@@ -277,7 +277,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)
@@ -285,10 +285,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;
+	int nh_off = 0;
 
-	if (skb->protocol != htons(ETH_P_IP) &&
-	    skb->protocol != htons(ETH_P_IPV6))
+	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;
@@ -306,7 +313,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);
 	}
 
-- 
2.53.0-Meta

[PATCH net v3 4/4] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[Todd Freed]

send on a psp tcp socket in three phases, and after each, check that the
data was sent, counted in the device's tx-packets, and received by the
peer. phases:

 1/ no socket options.

 2/ an extension header set on the socket.

 3/ option removed.

the test has a variant for each supported extension header type. for
the routing header, the test enables seg6_enabled on the peer, which
otherwise drops packets with a segment routing header.

a software filter on the peer also counts udp packets from the socket's
address to the peer's. the psp packets reach the peer as decrypted tcp
packets, as expected. malformed or unencrypted packets show up as udp,
and the test asserts there are none. if the peer can't take the filter,
the test is skipped.

tx-packets is read after it has quiesced, because some devices refresh
counters periodically (mlx5 takes about a second).

without the fixes in the preceding patches:

- with netdevsim, the peer receives none of the data of phase 2. it
  stays in the send queue, and the test fails after waiting for it to
  drain.
- with mlx5 (connectx-7), the packets go out unencrypted and aren't
  counted in tx-packets. the test fails on the udp packets which reach
  the peer. capturing on the peer,

    tcpdump -i <dev> -nn -A 'ip6 and src host <sender> and not tcp'

  shows them as udp, with the test's payload readable

Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 tools/testing/selftests/drivers/net/config |   1 +
 tools/testing/selftests/drivers/net/psp.py | 137 ++++++++++++++++++++-
 2 files changed, 135 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
index 4838adf27fa1..409f9a96a102 100644
--- a/tools/testing/selftests/drivers/net/config
+++ b/tools/testing/selftests/drivers/net/config
@@ -4,6 +4,7 @@ CONFIG_DEBUG_INFO_BTF_MODULES=n
 CONFIG_INET_PSP=y
 CONFIG_IPV6=y
 CONFIG_MACSEC=m
+CONFIG_NET_ACT_GACT=m
 CONFIG_NET_ACT_SKBEDIT=m
 CONFIG_NET_CLS_ACT=y
 CONFIG_NET_CLS_BPF=y
diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index a5b1e14f120f..2a56a7bf5d24 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -16,12 +16,12 @@ from lib.py import ksft_run, ksft_exit, ksft_pr
 from lib.py import ksft_true, ksft_eq, ksft_ne, ksft_gt, ksft_raises
 from lib.py import ksft_not_none
 from lib.py import ksft_variants, KsftNamedVariant
-from lib.py import KsftSkipEx, KsftFailEx
+from lib.py import KsftSkipEx, KsftFailEx, CmdExitFailure
 from lib.py import NetDrvEpEnv, NetDrvContEnv
 from lib.py import Netlink, NlError, PSPFamily, RtnlFamily
 from lib.py import NetNSEnter
 from lib.py import bkg, rand_port, wait_port_listen
-from lib.py import ip
+from lib.py import cmd, ip, tc
 
 TCP_ULP = 31
 
@@ -120,6 +120,57 @@ def _check_data_outq(s, exp_len, force_wait=False):
 def _get_stat(cfg, key):
     return cfg.pspnl.get_stats({'dev-id': cfg.psp_dev_id})[key]
 
+
+def _get_quiesced_stat(cfg, key, timeout=10):
+    """
+    Like _get_stat(), but wait till the value stops changing. Note
+    that some devices refresh flow counters periodically, e.g. once
+    per second with mlx5.
+    """
+    val = _get_stat(cfg, key)
+    end = time.monotonic() + timeout
+    while True:
+        time.sleep(1.5)
+        new = _get_stat(cfg, key)
+        if new == val:
+            return val
+        val = new
+        if time.monotonic() >= end:
+            ksft_pr(f"{key} still changing after {timeout}s: {val}")
+            return val
+
+
+def _remote_udp_from(cfg, src, dst):
+    """
+    Count what reaches the remote from @src to @dst as UDP, with a software
+    filter on its ingress (offloaded filter counters are refreshed
+    periodically). Returns a function reading the count, or None if the
+    filter can't be installed.
+    """
+    flt = (f"dev {cfg.remote_ifname} ingress protocol ipv6 pref 1 "
+           f"handle 0x5053 flower")
+    try:
+        qdiscs = tc(f"qdisc show dev {cfg.remote_ifname}", json=True,
+                    host=cfg.remote)
+        if not any(q['kind'] == 'clsact' for q in qdiscs):
+            tc(f"qdisc add dev {cfg.remote_ifname} clsact", host=cfg.remote)
+            defer(tc, f"qdisc del dev {cfg.remote_ifname} clsact",
+                  host=cfg.remote)
+        tc(f"filter add {flt} skip_hw src_ip {src} dst_ip {dst} "
+           f"ip_proto udp action drop", host=cfg.remote)
+    except CmdExitFailure as e:
+        ksft_pr(f"Can't count UDP on the remote: {e}")
+        return None
+    defer(tc, f"filter del {flt}", host=cfg.remote)
+
+    def count():
+        filters = tc(f"-s filter get {flt}", json=True, host=cfg.remote)
+        for f in filters:
+            for act in f.get("options", {}).get("actions", []):
+                return act["stats"]["packets"]
+        raise KsftFailEx("no action on the remote filter")
+    return count
+
 #
 # Test case boiler plate
 #
@@ -624,6 +675,85 @@ def removal_device_bi(cfg):
         _close_conn(cfg, s)
 
 
+def _ipv6_exthdr_variants():
+    padn = bytes([0, 0, 1, 4, 0, 0, 0, 0])  # one PadN option
+
+    def srh(peer):
+        """ segment routing header, with @peer as its only segment """
+        return (bytes([0, 2, 4, 0, 0, 0, 0, 0]) +
+                socket.inet_pton(socket.AF_INET6, peer))
+
+    yield KsftNamedVariant("dstopts", socket.IPV6_DSTOPTS, lambda peer: padn)
+    yield KsftNamedVariant("hopopts", socket.IPV6_HOPOPTS, lambda peer: padn)
+    yield KsftNamedVariant("rthdr", socket.IPV6_RTHDR, srh)
+
+
+def _remote_seg6_enable(cfg):
+    """ Make the remote accept segment routing headers """
+    for conf in ("all", cfg.remote_ifname):
+        # slashes, so that sysctl doesn't split a dotted ifname
+        key = f"net/ipv6/conf/{conf}/seg6_enabled"
+        old = cmd(f"sysctl -n {key}", host=cfg.remote).stdout.strip()
+        cmd(f"sysctl -qw {key}=1", host=cfg.remote)
+        defer(cmd, f"sysctl -qw {key}={old}", host=cfg.remote)
+
+
+@ksft_variants(_ipv6_exthdr_variants())
+def data_send_ipv6_exthdr(cfg, opt, val):
+    """ Test PSP on IPv6 packets with an extension header """
+    cfg.require_ipver("6")
+    _init_psp_dev(cfg)
+    if opt == socket.IPV6_RTHDR:
+        _remote_seg6_enable(cfg)
+
+    s = _make_psp_conn(cfg, 0, "6")
+    try:
+        # In happy path, PSP traffic arrives at the remote decrypted as TCP
+        remote_udp = _remote_udp_from(cfg, s.getsockname()[0],
+                                      s.getpeername()[0])
+        if not remote_udp:
+            raise KsftSkipEx("can't count UDP on the remote")
+
+        rx_assoc = cfg.pspnl.rx_assoc({"version": 0,
+                                       "dev-id": cfg.psp_dev_id,
+                                       "sock-fd": s.fileno()})
+        tx = _spi_xchg(s, rx_assoc['rx-key'])
+        cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
+                            "version": 0,
+                            "tx-key": tx,
+                            "sock-fd": s.fileno()})
+        _check_data_outq(s, 0)
+
+        total = 0
+
+        def send(phase):
+            """ Sent encrypted, counted, and received. Nothing reaches the
+            peer as plain UDP.
+            """
+            nonlocal total
+            tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+            udp = remote_udp()
+            total += _send_careful(cfg, s, 20)
+            for _ in range(100):
+                if _get_outq(s) == 0:
+                    break
+                time.sleep(0.5)
+            _check_data_outq(s, 0)
+            ksft_gt(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts,
+                    comment=phase)
+            ksft_eq(remote_udp(), udp, comment=f"{phase}: UDP at the remote")
+            _check_data_rx(cfg, total)
+
+        send("no option")
+        # UDP + PSP go after the extension header
+        s.setsockopt(socket.IPPROTO_IPV6, opt, val(s.getpeername()[0]))
+        send("with the option")
+        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
+        send("option removed")
+    finally:
+        _close_psp_conn(cfg, s)
+
+
 def _get_psp_ver_ip_variants():
     for ver in range(4):
         for ipv in ("4", "6"):
@@ -1019,7 +1149,8 @@ def main() -> None:
                                                           cfg.comm_port),
                                                          timeout=1)
 
-                cases = [data_basic_send, data_mss_adjust]
+                cases = [data_basic_send, data_mss_adjust,
+                         data_send_ipv6_exthdr]
 
                 if has_cont:
                     cases += [
-- 
2.53.0-Meta

Re: [PATCH net v3 1/4] net/mlx5e: psp: tso: replace only the length in checksum seed

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message dependency.

--

net/mlx5e: psp: tso: replace only the length in checksum seed

This patch changes mlx5e_psp_handle_tx_skb() so that, for PSP TSO packets,
it adjusts only the length part of the TCP checksum seed instead of
recomputing the seed from the l3 header addresses. The goal is to avoid
bad checksums when the l3 header daddr is an intermediate hop.

> the length difference is expected by the device. however, the
> difference in addresses is incorrect in cases where the l3 header daddr
> is an intermediate hop (e.g. IPV6_RTHDR). this causes every segment of
> the tso packet to go out with a bad tcp checksum, in this case.

[Severity: Low]
The code change in mlx5e_psp_handle_tx_skb() looks correct. However, is
IPV6_RTHDR the right example for this patch on its own?

At this point in the series, psp_dev_encapsulate() still writes the
nexthdr of the base IPv6 header unconditionally:

net/psp/psp_main.c:psp_dev_encapsulate() {
    ...
	} else {
		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
	}
    ...
}

With a routing header present, this overwrites NEXTHDR_ROUTING in the base
header, while the UDP+PSP headers go in after the routing header. That
means the packet is already malformed before the TCP checksum seed
matters. So this patch alone doesn't appear to fix the IPv6 RTHDR case.

The IPv6 case seems to be fixed later in the series by "psp: support ipv6
extension headers in psp_dev_encapsulate()", which has its own Fixes: tag.
The only mention of that dependency here is the testing note:

> tested on connectx-7 with the rest of this series applied, with a psp
> tcp socket whose segment routing header's first hop isn't the final
> destination, and the peer checking tcp checksums in software.

The case this patch fixes by itself looks like IPv4 source routing
(IP_OPTIONS with SRR/LSRR). There, ip_hdr(skb)->daddr is the first hop,
but tcp_v4_send_check() seeded the checksum with inet->inet_daddr.

Could the commit message mention the IPv4 SRR/LSRR case? Could it also
say that the IPv6 RTHDR case depends on the psp_dev_encapsulate() patch?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com

Re: [PATCH net v3 2/4] psp: support ipv6 extension headers in psp_dev_rcv() decap

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] psp_dev_rcv() now decrypts and decaps PSP packets that carry
  IPv6 extension headers.

Pre-existing issues:
- [Low] TX/RX asymmetry for IPv6 extension headers.

--

psp: support ipv6 extension headers in psp_dev_rcv() decap

This patch makes psp_dev_rcv() skip IPv6 extension headers to find the
UDP and PSP headers. On decap it writes the inner nexthdr into the last
extension header instead of the fixed header. Packets that carry fragment
or authentication headers are dropped.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 273b010d2355c..206ab7c3e3b51 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> @@ -188,6 +189,39 @@ unsigned int psp_key_size(u32 version)
>  }
>  EXPORT_SYMBOL(psp_key_size);
>  

[ ... ]

> +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_AUTH)
> +			return -EINVAL;

[Severity: High]
This filter only rejects NEXTHDR_FRAGMENT, NEXTHDR_AUTH and NEXTHDR_NONE.
That means routing headers of any type and any segments_left value are
accepted. So are hop-by-hop and destination options headers with any
option content. Could this expose decrypted plaintext through ICMPv6
errors?

The PSP ICV doesn't cover the outer IPv6 header or its extension headers.
An on-path attacker could take a captured PSP packet, insert one of these
headers, and set saddr to an address they control. This assumes the
device authenticates such packets, which this patch's premise implies.

Once psp_dev_rcv() succeeds, the driver marks the skb as decrypted.
For example, mlx5e_psp_offload_handle_rx_skb() sets skb->decrypted = 1.
The IPv6 stack then processes the extension headers before tcp_v6_rcv()
reaches psp_sk_rx_policy_check().

Several of those handlers reply with an ICMPv6 error that quotes the
packet. With default sysctls, this happens for an unrecognized routing
header type (type 0, for example) with segments_left != 0:

net/ipv6/exthdrs.c:ipv6_rthdr_rcv() {
    ...
unknown_rh:
	__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
	icmpv6_param_prob(skb, ICMPV6_HDR_FIELD,
			  (&hdr->type) - skb_network_header(skb));
    ...
}

It also happens for an unknown destination or hop-by-hop option whose
action bits request a parameter problem, through
ip6_parse_tlv()->ip6_tlvopt_unknown():

	case 2: /* send ICMP PARM PROB regardless and drop packet */
		icmpv6_param_prob_reason(skb, ICMPV6_UNK_OPTION, optoff,
					 SKB_DROP_REASON_UNHANDLED_PROTO);

When seg6_enabled is set, ipv6_srh_rcv() calls icmpv6_param_prob() if
segments_left >= (hdrlen >> 1). It also sends ICMPV6_TIME_EXCEED in the
hop_limit <= 1 loopback case.

icmp6_send() copies up to 1232 bytes of the original packet, starting at
the network header, into a new skb. That limit is IPV6_MIN_MTU minus the
IPv6 and ICMPv6 headers. The new skb doesn't have decrypted set, so
sk_validate_xmit_skb() doesn't drop it. It is sent in cleartext to the
attacker-chosen saddr, carrying the decrypted TCP header and payload.

Before this patch, all of these packets were dropped at the is_udp check.
Forwarding the decrypted skb itself looks blocked by
sk_validate_xmit_skb() ("unencrypted skb with no associated socket -
dropping"). That leaves the ICMP error path as the open channel.

Should psp_ipv6_nexthdr_off() be stricter about which extension headers
and options it accepts? Rejecting only routing headers with
segments_left != 0 would not cover the destination or hop-by-hop options
case. Later patches in the series don't change psp_ipv6_nexthdr_off() or
psp_dev_rcv().

[ ... ]

> @@ -390,10 +445,11 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
>  	} else {
>  		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
>  
> -		if (unlikely(ntohs(ipv6h->payload_len) < encap))
> +		if (unlikely(ntohs(ipv6h->payload_len) <
> +			     l3_hlen - sizeof(*ipv6h) + encap))
>  			return -EINVAL;
>  
> -		ipv6h->nexthdr = psph->nexthdr;
> +		((u8 *)ipv6h)[nh_off] = psph->nexthdr;

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The TX
side in psp_dev_encapsulate() still writes IPPROTO_UDP into the fixed
header:

	} else {
		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
	}

The UDP and PSP headers are inserted after skb_network_header_len()
bytes, which include any extension headers. A TCPv6 socket with
IPV6_HOPOPTS, IPV6_DSTOPTS or IPV6_RTHDR set would therefore emit:

  fixed(nexthdr=UDP) | EXT(nexthdr=TCP) | UDP | PSP | TCP

The new psp_dev_rcv() code expects the UDP nexthdr in the last extension
header. Here it would treat the first extension header as the UDP header
and drop the packet on the dport check.

In netdevsim, nsim_do_psp() calls psp_dev_encapsulate() and then
psp_dev_rcv(). So at this commit, the new RX path can't be tested end to
end with extension headers.

The next patch in the series, "psp: support ipv6 extension headers in
psp_dev_encapsulate()", seems to fix this. It reuses
psp_ipv6_nexthdr_off() and writes IPPROTO_UDP at nh_off. Would it make
sense to land the TX change before this one, or together with it?

>  		ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) - encap);
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com

Re: [PATCH net v3 4/4] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In data_send_ipv6_exthdr()
  (tools/testing/selftests/drivers/net/psp.py), a missing remote UDP…
- [Low] The new test does not exercise two code paths that earlier
  patches in this series fix, so a regression in either would still pass.

--

selftests: drv-net: psp: test ipv6 tcp socket with extension headers

This adds a data_send_ipv6_exthdr test to psp.py. It sends on a PSP TCP
socket in three phases: with no option, with an IPv6 extension header set
via setsockopt, and with the option removed. After each phase it checks
tx-packets, the received data, and that no plain UDP reaches the peer.
There is a variant each for dstopts, hopopts and rthdr, and the test is
meant to skip if the peer can't install the tc flower filter used for
counting UDP.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index 473500901879b..ddf69c0397da1 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> @@ -658,6 +709,85 @@ def removal_device_bi(cfg):
>          _close_conn(cfg, s)
>  
>  
> +def _ipv6_exthdr_variants():
> +    padn = bytes([0, 0, 1, 4, 0, 0, 0, 0])  # one PadN option
> +
> +    def srh(peer):
> +        """ segment routing header, with @peer as its only segment """
> +        return (bytes([0, 2, 4, 0, 0, 0, 0, 0]) +
> +                socket.inet_pton(socket.AF_INET6, peer))
> +
> +    yield KsftNamedVariant("dstopts", socket.IPV6_DSTOPTS, lambda peer: padn)
> +    yield KsftNamedVariant("hopopts", socket.IPV6_HOPOPTS, lambda peer: padn)
> +    yield KsftNamedVariant("rthdr", socket.IPV6_RTHDR, srh)

[Severity: Low]
This isn't a bug, but two code paths fixed earlier in the series are not
exercised by these variants.

Each variant sets exactly one sticky option, so this loop in
psp_ipv6_nexthdr_off() in net/psp/psp_main.c only ever runs once:

    while (off < len) {

A version that rewrote only the first extension header's nexthdr would
still pass all three variants. A TCP socket can emit two headers here, for
example IPV6_HOPOPTS plus IPV6_DSTOPTS. ip6_xmit() pushes those through
ipv6_push_nfrag_opts() and ipv6_push_frag_opts().

Also, srh() builds an SRH with segments_left=0 and the peer as its only
segment. In ipv6_push_rthdr4():

    sr_phdr->segments[0] = **addr_p;
    *addr_p = &sr_ihdr->segments[sr_ihdr->segments_left];

So the L3 daddr ends up equal to the final destination. As a result, the
intermediate-hop case fixed in mlx5e_psp_handle_tx_skb() by "net/mlx5e:
psp: tso: replace only the length in checksum seed" never comes up here.

Would it be worth adding a variant that sets two sticky options? The
intermediate-hop case probably needs more topology setup.

> +
> +
> +def _remote_seg6_enable(cfg):

[ ... ]

> +@ksft_variants(_ipv6_exthdr_variants())
> +def data_send_ipv6_exthdr(cfg, opt, val):
> +    """ Test PSP on IPv6 packets with an extension header """
> +    cfg.require_ipver("6")
> +    _init_psp_dev(cfg)
> +    if opt == socket.IPV6_RTHDR:
> +        _remote_seg6_enable(cfg)
> +
> +    s = _make_psp_conn(cfg, 0, "6")
> +    try:
> +        # In happy path, PSP traffic arrives at the remote decrypted as TCP
> +        remote_udp = _remote_udp_from(cfg, s.getsockname()[0],
> +                                      s.getpeername()[0])
> +        if not remote_udp:
> +            raise KsftSkipEx("can't count UDP on the remote")

[Severity: Medium]
Can this skip path end up reported as a failure?

_make_psp_conn() has already opened the data connection by the time
_remote_udp_from() runs. psp_responder then runs conn_setup_psp() inline
from run_session(). It sends its key and then blocks waiting for the
client's key:

tools/testing/selftests/drivers/net/psp_responder.c:conn_setup_psp() {
    ...
	sz = recv(data_sock, info, sz, MSG_WAITALL);
    ...
}

The tc filter install can fail, for example when cls_flower or act_gact
is missing on the remote. In that case KsftSkipEx is raised before
_spi_xchg() sends the client key, and the finally block runs:

_close_psp_conn()
  _close_conn()
    _send_with_ack(cfg, b'data close\0')

The responder is still blocked in conn_setup_psp() and never reads the
'data close' command. The recv(4) on comm_sock, which was created with
timeout=1, then raises TimeoutError. That replaces the KsftSkipEx, and
ksft_run() records a FAIL instead of a SKIP.

In _close_conn(), s.close() comes after _send_with_ack(), so the data
socket is not closed explicitly either.

What happens when the socket is eventually closed? The responder might
see the data socket EOF before the queued 'data close', hit the
race_close path with data_sock == -1, and leave a stale 'err' reply on
the control channel. Later tests, including the next variants that take
the same skip path, could then fail with "Unexpected server response".

Could the remote filter be installed before _make_psp_conn() is called?
Another option would be to finish the key exchange before any early exit.

> +
> +        rx_assoc = cfg.pspnl.rx_assoc({"version": 0,

[ ... ]

> +    finally:
> +        _close_psp_conn(cfg, s)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com