[PATCH net 0/2] psp: drop packets with extension headers in transport mode

[PATCH net 0/2] psp: drop packets with extension headers in transport mode

[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 drops the packets, the second patch adds selftests for
that case.

Todd Freed (2):
  psp: drop transport mode packets with extension headers
  selftests: drv-net: psp: test ipv6 tcp socket with extension headers

 net/psp/psp_main.c                         |   7 +-
 tools/testing/selftests/drivers/net/config |   1 +
 tools/testing/selftests/drivers/net/psp.py | 120 ++++++++++++++++++++-
 3 files changed, 125 insertions(+), 3 deletions(-)

-- 
2.53.0-Meta

[PATCH net 1/2] psp: drop transport mode packets with extension headers

[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 is wrong.

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.

drop such packets on egress rather than them send malformed.

note that a tcp socket which has such options set stops making progress.
its packets are dropped (SKB_DROP_REASON_PSP_OUTPUT) until the
connection times out.

refusing the combination on the control path, when the options or the tx
association are set, would be helpful but is not implemented here.
extension headers can also come from a route, so the datapath check is
needed either way.

Fixes: fc724515741a ("psp: provide encapsulation helper for drivers")

Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/psp/psp_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
index c9c1a8826b7f..0d276e2aceff 100644
--- a/net/psp/psp_main.c
+++ b/net/psp/psp_main.c
@@ -243,7 +243,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. IPv6 packets with extension headers are rejected.
  */
 bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
 			 u8 ver, __be16 sport)
@@ -256,6 +256,11 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
 	    skb->protocol != htons(ETH_P_IPV6))
 		return false;
 
+	/* extension headers not supported */
+	if (skb->protocol == htons(ETH_P_IPV6) &&
+	    network_len != sizeof(struct ipv6hdr))
+		return false;
+
 	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
 		return false;
 
-- 
2.53.0-Meta

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

[Todd Freed]

send on a psp tcp socket in three phases, checking the device's
tx-packets and what the peer received after each

1/ no socket options - the data is sent and counted.

2/ an extension header set on the socket - assert that the packets are
   dropped, that tx-packets is unchanged, and that nothing reaches the
   peer. the data stays queued.

3/ option removed - the queued data and new data are sent and counted,
   and the peer gets all of it.

phases 2 and 3 run twice, with a destination options header
(IPV6_DSTOPTS) and with a hop-by-hop options header (IPV6_HOPOPTS), each
holding one padn option.

in phase 2, a software filter on the peer also counts udp packets from
the socket's address to the peer's. the well-formed psp packets reach
the peer as decrypted tcp packets, as expected. the malformed packets
show up as udp. if the peer can't take the filter, that check is
skipped.

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

without the fix in the preceding patch:

- with netdevsim, the packets of phase 2 are encapsulated and counted in
  tx-packets, and the test fails on that.
- 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 | 120 ++++++++++++++++++++-
 2 files changed, 119 insertions(+), 2 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..2a109ef61468 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 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,71 @@ def removal_device_bi(cfg):
         _close_conn(cfg, s)
 
 
+def data_send_ipv6_exthdr(cfg):
+    """ Test that IPv6 packets with extension headers are dropped """
+    cfg.require_ipver("6")
+    _init_psp_dev(cfg)
+
+    s = _make_psp_conn(cfg, 0, "6")
+
+    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)
+
+    def wait_outq_empty():
+        for _ in range(100):
+            if _get_outq(s) == 0:
+                break
+            time.sleep(0.5)
+        _check_data_outq(s, 0)
+
+    # 1. No options: sent and counted
+    tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+    total = _send_careful(cfg, s, 20)
+    wait_outq_empty()
+    ksft_gt(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts)
+    _check_data_rx(cfg, total)
+
+    # In happy path, PSP traffic arrives at the remote decrypted as TCP
+    remote_udp = _remote_udp_from(cfg, s.getsockname()[0],
+                                  s.getpeername()[0])
+
+    # An extension header with one PadN option, as destination options and
+    # as hop-by-hop options
+    for name, opt in (("IPV6_DSTOPTS", socket.IPV6_DSTOPTS),
+                      ("IPV6_HOPOPTS", socket.IPV6_HOPOPTS)):
+        # 2. With the option: dropped, not counted, and nothing reaches
+        # the peer. The data stays queued.
+        s.setsockopt(socket.IPPROTO_IPV6, opt,
+                     bytes([0, 0, 1, 4, 0, 0, 0, 0]))
+        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+        udp = remote_udp() if remote_udp else 0
+        queued = _send_careful(cfg, s, 20)
+        _check_data_outq(s, queued, force_wait=True)
+        ksft_eq(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts, comment=name)
+        if remote_udp:
+            ksft_eq(remote_udp(), udp, comment=f"{name}: UDP at the remote")
+        _check_data_rx(cfg, total)
+        total += queued
+
+        # 3. Option removed: the queued data, then new data, sent and
+        # counted
+        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
+        wait_outq_empty()
+        total += _send_careful(cfg, s, 20)
+        wait_outq_empty()
+        ksft_gt(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts, comment=name)
+        _check_data_rx(cfg, total)
+    _close_psp_conn(cfg, s)
+
+
 def _get_psp_ver_ip_variants():
     for ver in range(4):
         for ipv in ("4", "6"):
-- 
2.53.0-Meta

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Daniel Zahka]

On Wed Oct 21, 2026 at 5:01 PM EDT, Todd Freed wrote:
> 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 drops the packets, the second patch adds selftests for
> that case.
>

Why not just fix psp_dev_encapsulate() to put the psp headers in the
right position? With the self test we are going to make PSP + ipv6 ext
header == drop part of the uapi?

> Todd Freed (2):
>   psp: drop transport mode packets with extension headers
>   selftests: drv-net: psp: test ipv6 tcp socket with extension headers
>
>  net/psp/psp_main.c                         |   7 +-
>  tools/testing/selftests/drivers/net/config |   1 +
>  tools/testing/selftests/drivers/net/psp.py | 120 ++++++++++++++++++++-
>  3 files changed, 125 insertions(+), 3 deletions(-)

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Todd Bert Freed]

On Wed, Oct 21, 2026 at 3:07 PM Daniel Zahka <daniel.zahka@gmail.com> wrote:
> >
> > The first patch drops the packets, the second patch adds selftests for
> > that case.
> >
>
> Why not just fix psp_dev_encapsulate() to put the psp headers in the
> right position? With the self test we are going to make PSP + ipv6 ext
> header == drop part of the uapi?

If the original ipv6 packet has extension headers, then the nexthdr on the
fixed ipv6 header identifies the first extension header, and the L4 proto is
in the nexthdr on the last one in the chain.

psp_dev_encapsulate() places the udp+psp headers after all the extension
headers in this case.

The issue is that it rewrites the fixed ipv6 header nexthdr to be UDP,

  ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;

It would need to walk the chain, and set the nexthdr on the final extension
header to UDP, instead, if the correct policy were for udp+psp to come
after the extension headers in all cases.

I'm not sure that's the correct policy though ; for some headers (e.g. the
fragment header) it might make sense to leave them on the inner packet.
Note that the psp spec does not describe this. Perhaps the user should
express the desired policy?

Explicitly dropping ipv6 packets with fragments is consistent with the
current behavior (it already doesn't work).

-Todd

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Björn Töpel]

Todd Bert Freed <toddfreed@meta.com> writes:

> On Wed, Oct 21, 2026 at 3:07 PM Daniel Zahka <daniel.zahka@gmail.com> wrote:
>> >
>> > The first patch drops the packets, the second patch adds selftests for
>> > that case.
>> >
>>
>> Why not just fix psp_dev_encapsulate() to put the psp headers in the
>> right position? With the self test we are going to make PSP + ipv6 ext
>> header == drop part of the uapi?
>
> If the original ipv6 packet has extension headers, then the nexthdr on the
> fixed ipv6 header identifies the first extension header, and the L4 proto is
> in the nexthdr on the last one in the chain.
>
> psp_dev_encapsulate() places the udp+psp headers after all the extension
> headers in this case.
>
> The issue is that it rewrites the fixed ipv6 header nexthdr to be UDP,
>
>   ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
>
> It would need to walk the chain, and set the nexthdr on the final extension
> header to UDP, instead, if the correct policy were for udp+psp to come
> after the extension headers in all cases.
>
> I'm not sure that's the correct policy though ; for some headers (e.g. the
> fragment header) it might make sense to leave them on the inner packet.
> Note that the psp spec does not describe this. Perhaps the user should
> express the desired policy?

Fragment extension header would be weird to keep in transport mode...
I'd say drop is the right call.


Björn

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Daniel Zahka]

On Wed Oct 21, 2026 at 7:16 PM EDT, Todd Bert Freed wrote:
> On Wed, Oct 21, 2026 at 3:07 PM Daniel Zahka <daniel.zahka@gmail.com> wrote:
>> >
>> > The first patch drops the packets, the second patch adds selftests for
>> > that case.
>> >
>>
>> Why not just fix psp_dev_encapsulate() to put the psp headers in the
>> right position? With the self test we are going to make PSP + ipv6 ext
>> header == drop part of the uapi?
>
> If the original ipv6 packet has extension headers, then the nexthdr on the
> fixed ipv6 header identifies the first extension header, and the L4 proto is
> in the nexthdr on the last one in the chain.
>
> psp_dev_encapsulate() places the udp+psp headers after all the extension
> headers in this case.
>
> The issue is that it rewrites the fixed ipv6 header nexthdr to be UDP,
>
>   ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
>
> It would need to walk the chain, and set the nexthdr on the final extension
> header to UDP, instead, if the correct policy were for udp+psp to come
> after the extension headers in all cases.
>
> I'm not sure that's the correct policy though ; for some headers (e.g. the
> fragment header) it might make sense to leave them on the inner packet.
> Note that the psp spec does not describe this. Perhaps the user should
> express the desired policy?
>
> Explicitly dropping ipv6 packets with fragments is consistent with the
> current behavior (it already doesn't work).
>
> -Todd

I think walking the extension headers and inserting before the transport
header is better than dropping. I agree with Bjorn that rejecting
fragment header makes sense because the receiver can't decrypt the
fragmented PSP payload.

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Willem de Bruijn]

Todd Freed wrote:
> 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.

Just curious: what is the plan for replay protection for UDP?

As PSP depends on the inner L4 for replay protection today.

FWIW agreed on the ext_hdr conclusion in this thread.

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Todd Bert Freed]

> Todd Freed wrote:
> > 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.
>
> Just curious: what is the plan for replay protection for UDP?
>
> As PSP depends on the inner L4 for replay protection today.
>
> FWIW agreed on the ext_hdr conclusion in this thread.

To clarify, socket-less psp associations will apply tunnel-mode psp encap.
So it is in this sense that UDP will be supported (not e.g. adding support
for udp sockets with transport mode psp encap).

That being said, UDP which is PSP encrypted (transport or tunnel mode)
would be susceptible to replay attacks, just like regular UDP. Except in
the sense that after enough time has passed, the packet would not
decrypt if received again (operator has rotated the keys, etc).

Not sure if you had some other kind of replay protection in mind.

-Todd

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Willem de Bruijn]

Todd Bert Freed wrote:
> > Todd Freed wrote:
> > > 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.
> >
> > Just curious: what is the plan for replay protection for UDP?
> >
> > As PSP depends on the inner L4 for replay protection today.
> >
> > FWIW agreed on the ext_hdr conclusion in this thread.
> 
> To clarify, socket-less psp associations will apply tunnel-mode psp encap.
> So it is in this sense that UDP will be supported (not e.g. adding support
> for udp sockets with transport mode psp encap).
>
> That being said, UDP which is PSP encrypted (transport or tunnel mode)
> would be susceptible to replay attacks, just like regular UDP. Except in
> the sense that after enough time has passed, the packet would not
> decrypt if received again (operator has rotated the keys, etc).
> 
> Not sure if you had some other kind of replay protection in mind.

Got it, thanks.

No. I'm just slightly concerned that casual users of such a feature
might assume that the security protocol handles replay protection.

Re: [PATCH net 0/2] psp: drop packets with extension headers in transport mode

[Todd Bert Freed]

> > Explicitly dropping ipv6 packets with fragments is consistent with the
> > current behavior (it already doesn't work).
> >
> > -Todd
>
> I think walking the extension headers and inserting before the transport
> header is better than dropping. I agree with Bjorn that rejecting
> fragment header makes sense because the receiver can't decrypt the
> fragmented PSP payload.

Sounds good. Please see v2, where I have implemented this. After doing
more research today, I discovered reasons why routing headers will also
not cleanly work, so dropping those as well.

-Todd

Re: [PATCH net 1/2] psp: drop transport mode packets with extension headers

[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

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] The new check in psp_dev_encapsulate()
  (net/psp/psp_main.c:260-262) only looks for IPv6 extension headers…

--

psp: drop transport mode packets with extension headers

This adds a check to psp_dev_encapsulate() that rejects IPv6 packets
whose network header length is not sizeof(struct ipv6hdr). Without it,
the nexthdr of the fixed IPv6 header gets rewritten to UDP even when
extension headers sit between it and the TCP header. That produced
malformed packets, and on mlx5 unencrypted ones.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index c9c1a8826b7f9..0d276e2aceff4 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> @@ -256,6 +256,11 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  	    skb->protocol != htons(ETH_P_IPV6))
>  		return false;
>  
> +	/* extension headers not supported */
> +	if (skb->protocol == htons(ETH_P_IPV6) &&
> +	    network_len != sizeof(struct ipv6hdr))
> +		return false;
> +

[Severity: High]
Does this check catch the case where a route inserts the extension
headers? The commit message gives that case as the reason for a datapath
check:

  "extension headers can also come from a route, so the datapath check
  is needed either way."

network_len comes from skb_network_header_len(), which is only
transport_header - network_header. The lwtunnel encapsulators that add
IPv6 extension headers on egress insert the extension header and then set
the transport header to the offset right after the fixed header:

net/ipv6/seg6_iptunnel.c:seg6_do_srh() {
    ...
	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
	nf_reset_ct(skb);
    ...
}

rpl_do_srh_inline() in net/ipv6/rpl_iptunnel.c does the same. So do the
ioam6 inline and encap helpers in net/ipv6/ioam6_iptunnel.c, which call
skb_set_transport_header(skb, sizeof(*hdr)).

Take a PSP TCP flow that leaves the PSP netdev through a seg6, rpl or
ioam6 lwtunnel route. Its network_len is exactly sizeof(struct ipv6hdr),
even though a Routing or Hop-by-Hop header sits right after the fixed
header:

seg6_output_core()
  seg6_do_srh()
    skb_set_transport_header(skb, sizeof(struct ipv6hdr))
dev_queue_xmit()
  psp_validate_xmit()
  mlx5e_psp_handle_tx_skb() (or nsim_do_psp())
    psp_dev_encapsulate()
      network_len == sizeof(struct ipv6hdr), so the new check passes

After that, psp_dev_encapsulate() still does:

		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;

This overwrites NEXTHDR_ROUTING or NEXTHDR_HOP. The UDP and PSP headers
end up in front of the extension header. psp_write_headers() also sets
psph->nexthdr = IPPROTO_TCP, but the next bytes are an SRH or HBH header.

Wouldn't this produce the same kind of malformed packet that this patch
is meant to drop?

The follow-up selftest patch in the series doesn't touch psp_main.c, so
this path still seems reachable at the end of the series.

Could the check look at the header chain itself instead? For example,
rejecting the packet when ipv6_hdr(skb)->nexthdr != IPPROTO_TCP would
cover both the socket option case and the route case.

>  	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
>  		return false;
>

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

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

[netdev-bot+sashiko]

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

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

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] On hardware PSP devices such as mlx5, data_send_ipv6_exthdr()
  in tools/testing/selftests/drivers/net/psp.py passes whether or not…
- [Low] The docstring of _remote_udp_from() in
  tools/testing/selftests/drivers/net/psp.py describes the filter as…
- [Low] data_send_ipv6_exthdr() in
  tools/testing/selftests/drivers/net/psp.py opens the PSP connection at…

--

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

This adds data_send_ipv6_exthdr() to the psp.py selftest. It sends on an
IPv6 PSP TCP socket without options, then with IPV6_DSTOPTS or
IPV6_HOPOPTS set, then with the option removed. After each phase it
checks tx-packets and what the peer received. It also adds the
_get_quiesced_stat() and _remote_udp_from() helpers and enables
CONFIG_NET_ACT_GACT in the config fragment.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index a5b1e14f120f0..2a109ef61468a 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> @@ -120,6 +120,57 @@ def _check_data_outq(s, exp_len, force_wait=False):

[ ... ]

> +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)

[Severity: Low]
This isn't a bug, but the docstring says the filter counts UDP from @src
to @dst. The filter is installed with "action drop", though, so matching
packets are also dropped while it is in place.

Could the docstring mention the drop? Alternatively, "action pass" would
keep the filter a pure counter.

> +    except CmdExitFailure as e:
> +        ksft_pr(f"Can't count UDP on the remote: {e}")
> +        return None

[ ... ]

> @@ -624,6 +675,71 @@ def removal_device_bi(cfg):
>          _close_conn(cfg, s)
>  
>  
> +def data_send_ipv6_exthdr(cfg):
> +    """ Test that IPv6 packets with extension headers are dropped """
> +    cfg.require_ipver("6")
> +    _init_psp_dev(cfg)
> +
> +    s = _make_psp_conn(cfg, 0, "6")

[ ... ]

> +    for name, opt in (("IPV6_DSTOPTS", socket.IPV6_DSTOPTS),
> +                      ("IPV6_HOPOPTS", socket.IPV6_HOPOPTS)):
> +        # 2. With the option: dropped, not counted, and nothing reaches
> +        # the peer. The data stays queued.
> +        s.setsockopt(socket.IPPROTO_IPV6, opt,
> +                     bytes([0, 0, 1, 4, 0, 0, 0, 0]))
> +        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
> +        udp = remote_udp() if remote_udp else 0
> +        queued = _send_careful(cfg, s, 20)
> +        _check_data_outq(s, queued, force_wait=True)
> +        ksft_eq(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts, comment=name)
> +        if remote_udp:
> +            ksft_eq(remote_udp(), udp, comment=f"{name}: UDP at the remote")

[Severity: Medium]
On mlx5, is this the only check that can tell a fixed kernel from an
unfixed one?

The commit message says that without the fix, mlx5 sends the phase 2
packets unencrypted and doesn't count them in tx-packets. The driver
takes tx_packets from a hardware counter:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c:mlx5e_psp_get_stats() {
    ...
	stats->tx_packets = nstats.psp_tx_pkts;
	stats->tx_bytes = nstats.psp_tx_bytes;
	stats->tx_error = atomic_read(&priv->psp->tx_drop);
    ...
}

So the ksft_eq() on tx-packets holds with or without the fix.

The leaked packets reach the peer as UDP, not TCP, so _check_data_rx()
also passes. Phase 3 retransmits without the option, so it passes too.

_remote_udp_from() returns None from its except block on any
CmdExitFailure. Examples: no tc, flower or gact on the remote, no
permission, or an existing filter at pref 1 / handle 0x5053. In that
case udp is set to 0, this check is skipped, and the test reports PASS.

The commit message does say "if the peer can't take the filter, that
check is skipped". Even so, could the test raise KsftXfailEx or
KsftSkipEx when remote_udp is None on a device other than netdevsim? That
way CI could see that the case wasn't covered.

Another option is a check that doesn't depend on the remote, such as
tx-error. mlx5 reports it from priv->psp->tx_drop, which increments when
psp_dev_encapsulate() fails. netdevsim doesn't report tx-error, so this
check would have to be per device.

> +        _check_data_rx(cfg, total)
> +        total += queued

[ ... ]

> +        _check_data_rx(cfg, total)
> +    _close_psp_conn(cfg, s)

[Severity: Low]
Should the code from _make_psp_conn() up to here be wrapped in
try/finally? _data_mss_adjust(), data_stale_key(), removal_device_rx()
and removal_device_bi() all close the connection from a finally block.

ksft_eq() and ksft_gt() don't raise, but several calls in between can:

- _send_careful() raises RuntimeError after 10 EAGAIN retries. This is
  more likely in phase 2, where nothing gets acked.
- The count() closure raises KsftFailEx. Its "tc -s filter get" call can
  also raise CmdExitFailure, since it is outside the try/except in
  _remote_udp_from().
- s.setsockopt() can raise OSError.
- The rx_assoc and tx_assoc calls can raise NlError.

If any of these raises, "data close" is never sent. The responder keeps
its data socket until the next accept, where it prints "WARN: new data
sock but old one still here".

The local socket is only closed once it's garbage collected. If the
exception happens in phase 2, the socket still has the extension header
option and queued data. It then keeps retransmitting packets that
psp_dev_encapsulate() drops, until it times out.

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