[PATCH net-next 0/6] netdevsim: psp: implement real crypto operations from the PSP spec

[PATCH net-next 0/6] netdevsim: psp: implement real crypto operations from the PSP spec

[Daniel Zahka]

My motivation for this series is to enable packetdrill [1] testing to work
with netdevsim the same way it would work against a real NIC + driver
stack. That means being able to write already enapsulated and
encrypted packets from userspace into a packet socket (skips normal
psp tx path), yet still trigger the psp rx path (decapsulation and
metadata creation) just based on parsing the bytes "on the wire".

I will add that I believe this also has the benefit of making the
netdevsim code higher fidelity by removing the fake authentication
hack used by data_send_bad_key testcase in psp.py, and replacing with
true authentication from aes-gcm.

The header parsing in nsim_poll() may have some side effects on
non-psp paths from the pskb_may_pull'ing in nsim_psp_handle_rx(), but
that can be avoided by not configuring psp.

As for the code, I believe it is fairly straightforward. It implements
what is described in the psp spec. It preserves passing behavior of
the psp.py tests, and I have tested the crypto interoperability with
an nvidia cx7 card.

[1]: https://github.com/google/packetdrill/pull/100

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
Daniel Zahka (6):
      netdevsim: psp: reset spi on key rotation and check for exhaustion on alloc
      netdevsim: psp: remove unnecessary UDP checksum computation
      netdevsim: psp: move rx processing into nsim_poll()
      netdevsim: psp: implement kdf from psp spec
      netdevsim: psp: add real aes-gcm encryption and decryption
      netdevsim: psp: count rx authentication and length errors

 drivers/net/Kconfig               |   2 +
 drivers/net/netdevsim/netdev.c    |  18 +--
 drivers/net/netdevsim/netdevsim.h |  19 ++-
 drivers/net/netdevsim/psp.c       | 333 ++++++++++++++++++++++++++++----------
 4 files changed, 272 insertions(+), 100 deletions(-)
---
base-commit: 6a4c4656b0d2d4056a1f0c35442db4e8a5cf8021
change-id: 20260430-nsim-psp-crypto-03110ff293f1

Best regards,
-- 
Daniel Zahka <daniel.zahka@gmail.com>

[PATCH net-next 1/6] netdevsim: psp: reset spi on key rotation and check for exhaustion on alloc

[Daniel Zahka]

The PSP spec states that the lower 31b of the SPI need to be
non-zero. Though not in the spec, I think it is reasonable to reset
the lower 31b of the spi space after a key rotation, and to also
decline to generate session keys when the lower 31b saturate.

Assisted-by: Claude:claude-opus-4.7
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/netdevsim/psp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 6936ecb8173e..5073bda60883 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -132,14 +132,14 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
 		  struct netlink_ext_ack *extack)
 {
 	struct netdevsim *ns = psd->drv_priv;
-	unsigned int new;
 	int i;
 
-	new = ++ns->psp.spi & PSP_SPI_KEY_ID;
-	if (psd->generation & 1)
-		new |= PSP_SPI_KEY_PHASE;
+	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
+		NL_SET_ERR_MSG(extack, "SPI space exhausted");
+		return -ENOSPC;
+	}
 
-	assoc->spi = cpu_to_be32(new);
+	assoc->spi = cpu_to_be32(++ns->psp.spi);
 	assoc->key[0] = psd->generation;
 	for (i = 1; i < PSP_MAX_KEY; i++)
 		assoc->key[i] = ns->psp.spi + i;
@@ -162,6 +162,10 @@ static int nsim_assoc_add(struct psp_dev *psd, struct psp_assoc *pas,
 
 static int nsim_key_rotate(struct psp_dev *psd, struct netlink_ext_ack *extack)
 {
+	struct netdevsim *ns = psd->drv_priv;
+
+	ns->psp.spi = (ns->psp.spi & PSP_SPI_KEY_PHASE) ^ PSP_SPI_KEY_PHASE;
+
 	return 0;
 }
 

-- 
2.52.0

[PATCH net-next 2/6] netdevsim: psp: remove unnecessary UDP checksum computation

[Daniel Zahka]

The PSP spec requires the implementations accept 0 checksum in psp-udp
header. Let's take advantage of that to trim netdevsim's psp code
down. psp_dev_encapsulate() already sets uh->check to 0.

Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/netdevsim/psp.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 5073bda60883..75740e2a731f 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-#include <linux/ip.h>
 #include <linux/skbuff.h>
-#include <net/ip6_checksum.h>
 #include <net/psp.h>
 #include <net/sock.h>
 
@@ -81,36 +79,6 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
 			      skb->len - skb_inner_transport_offset(skb));
 		u64_stats_update_end(&ns->psp.syncp);
 	} else {
-		struct ipv6hdr *ip6h __maybe_unused;
-		struct iphdr *iph;
-		struct udphdr *uh;
-		__wsum csum;
-
-		/* Do not decapsulate. Receive the skb with the udp and psp
-		 * headers still there as if this is a normal udp packet.
-		 * psp_dev_encapsulate() sets udp checksum to 0, so we need to
-		 * provide a valid checksum here, so the skb isn't dropped.
-		 */
-		uh = udp_hdr(skb);
-		csum = skb_checksum(skb, skb_transport_offset(skb),
-				    ntohs(uh->len), 0);
-
-		switch (skb->protocol) {
-		case htons(ETH_P_IP):
-			iph = ip_hdr(skb);
-			uh->check = udp_v4_check(ntohs(uh->len), iph->saddr,
-						 iph->daddr, csum);
-			break;
-#if IS_ENABLED(CONFIG_IPV6)
-		case htons(ETH_P_IPV6):
-			ip6h = ipv6_hdr(skb);
-			uh->check = udp_v6_check(ntohs(uh->len), &ip6h->saddr,
-						 &ip6h->daddr, csum);
-			break;
-#endif
-		}
-
-		uh->check	= uh->check ?: CSUM_MANGLED_0;
 		skb->ip_summed	= CHECKSUM_NONE;
 	}
 

-- 
2.52.0

[PATCH net-next 3/6] netdevsim: psp: move rx processing into nsim_poll()

[Daniel Zahka]

nsim_do_psp() does PSP decap and skb extension creation in the tx
path. This has the slightly undesirable property of not allowing the
psp rx code to run on PSP packets cooked up in userspace and
transmitted on a packet socket from the peer dev (e.g. packetdrill).

This commit instead triggers the psp rx path just based on parsing the
received skb. The current code relies on a bit of a hack to simulate
authentication with the proper key: the peer's psd->generation was
placed into the tx key, and during decap used to fill out the
extension the packet before being sent up the psp rx path. This commit
removes that hack, which creates a transient break in psp.py test
cases that rely on this behavior (e.g. data_send_bad_key). Subsequent
commits which introduce real aes-gcm crypto will restore the correct
behavior.

Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/netdevsim/netdev.c    |  18 ++---
 drivers/net/netdevsim/netdevsim.h |  14 ++--
 drivers/net/netdevsim/psp.c       | 143 ++++++++++++++++++++++++++------------
 3 files changed, 113 insertions(+), 62 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index a750768912b5..9c0db7b91fd6 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -103,19 +103,13 @@ static int nsim_napi_rx(struct net_device *tx_dev, struct net_device *rx_dev,
 static int nsim_forward_skb(struct net_device *tx_dev,
 			    struct net_device *rx_dev,
 			    struct sk_buff *skb,
-			    struct nsim_rq *rq,
-			    struct skb_ext *psp_ext)
+			    struct nsim_rq *rq)
 {
 	int ret;
 
 	ret = __dev_forward_skb(rx_dev, skb);
-	if (ret) {
-		if (psp_ext)
-			__skb_ext_put(psp_ext);
+	if (ret)
 		return ret;
-	}
-
-	nsim_psp_handle_ext(skb, psp_ext);
 
 	return nsim_napi_rx(tx_dev, rx_dev, rq, skb);
 }
@@ -123,7 +117,6 @@ static int nsim_forward_skb(struct net_device *tx_dev,
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
-	struct skb_ext *psp_ext = NULL;
 	struct net_device *peer_dev;
 	unsigned int len = skb->len;
 	struct netdevsim *peer_ns;
@@ -147,7 +140,7 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		peer_dev = peer_ns->netdev;
 	}
 
-	dr = nsim_do_psp(skb, ns, peer_ns, &psp_ext);
+	dr = nsim_psp_handle_tx(skb, ns);
 	if (dr)
 		goto out_drop_free;
 
@@ -165,7 +158,7 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb_tx_timestamp(skb);
 	if (unlikely(nsim_forward_skb(dev, peer_dev,
-				      skb, rq, psp_ext) == NET_RX_DROP))
+				      skb, rq) == NET_RX_DROP))
 		goto out_drop_cnt;
 
 	if (!hrtimer_active(&rq->napi_timer))
@@ -379,6 +372,9 @@ static int nsim_rcv(struct nsim_rq *rq, int budget)
 
 		skb = skb_dequeue(&rq->skb_queue);
 
+		if (nsim_psp_handle_rx(ns, skb))
+			continue;
+
 		if (xdp_prog) {
 			/* skb might be freed directly by XDP, save the len */
 			skblen = skb->len;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index d909c4160ea1..dcea76429bac 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -451,22 +451,22 @@ static inline void nsim_macsec_teardown(struct netdevsim *ns)
 #if IS_ENABLED(CONFIG_INET_PSP)
 int nsim_psp_init(struct netdevsim *ns);
 void nsim_psp_uninit(struct netdevsim *ns);
-void nsim_psp_handle_ext(struct sk_buff *skb, struct skb_ext *psp_ext);
 enum skb_drop_reason
-nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
-	    struct netdevsim *peer_ns, struct skb_ext **psp_ext);
+nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns);
+bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb);
 #else
 static inline int nsim_psp_init(struct netdevsim *ns) { return 0; }
 static inline void nsim_psp_uninit(struct netdevsim *ns) {}
 static inline enum skb_drop_reason
-nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
-	    struct netdevsim *peer_ns, struct skb_ext **psp_ext)
+nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 {
 	return 0;
 }
 
-static inline void
-nsim_psp_handle_ext(struct sk_buff *skb, struct skb_ext *psp_ext) {}
+static inline bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
+{
+	return false;
+}
 #endif
 
 int nsim_setup_tc(struct net_device *dev, enum tc_setup_type type,
diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 75740e2a731f..e8831d4bf394 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -6,18 +6,10 @@
 
 #include "netdevsim.h"
 
-void nsim_psp_handle_ext(struct sk_buff *skb, struct skb_ext *psp_ext)
-{
-	if (psp_ext)
-		__skb_ext_set(skb, SKB_EXT_PSP, psp_ext);
-}
-
 enum skb_drop_reason
-nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
-	    struct netdevsim *peer_ns, struct skb_ext **psp_ext)
+nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 {
 	enum skb_drop_reason rc = 0;
-	struct psp_dev *peer_psd;
 	struct psp_assoc *pas;
 	struct net *net;
 	void **ptr;
@@ -46,47 +38,110 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
 		goto out_unlock;
 	}
 
-	/* Now pretend we just received this frame */
-	peer_psd = rcu_dereference(peer_ns->psp.dev);
-	if (peer_psd && peer_psd->config.versions & (1 << pas->version)) {
-		bool strip_icv = false;
-		u8 generation;
-
-		/* We cheat a bit and put the generation in the key.
-		 * In real life if generation was too old, then decryption would
-		 * fail. Here, we just make it so a bad key causes a bad
-		 * generation too, and psp_sk_rx_policy_check() will fail.
-		 */
-		generation = pas->tx.key[0];
-
-		skb_ext_reset(skb);
-		skb->mac_len = ETH_HLEN;
-		if (psp_dev_rcv(skb, peer_psd->id, generation, strip_icv)) {
-			rc = SKB_DROP_REASON_PSP_OUTPUT;
-			goto out_unlock;
-		}
-
-		*psp_ext = skb->extensions;
-		refcount_inc(&(*psp_ext)->refcnt);
-		skb->decrypted = 1;
-
-		u64_stats_update_begin(&ns->psp.syncp);
-		u64_stats_inc(&ns->psp.tx_packets);
-		u64_stats_inc(&ns->psp.rx_packets);
-		u64_stats_add(&ns->psp.tx_bytes,
-			      skb->len - skb_inner_transport_offset(skb));
-		u64_stats_add(&ns->psp.rx_bytes,
-			      skb->len - skb_inner_transport_offset(skb));
-		u64_stats_update_end(&ns->psp.syncp);
-	} else {
-		skb->ip_summed	= CHECKSUM_NONE;
-	}
+	skb->decrypted = 0;
 
+	u64_stats_update_begin(&ns->psp.syncp);
+	u64_stats_inc(&ns->psp.tx_packets);
+	u64_stats_add(&ns->psp.tx_bytes,
+		      skb->len - skb_inner_transport_offset(skb));
+	u64_stats_update_end(&ns->psp.syncp);
 out_unlock:
 	rcu_read_unlock();
 	return rc;
 }
 
+/* Returns true if skb was consumed, false otherwise. */
+bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
+{
+	struct psp_dev *psd;
+	struct psphdr *psph;
+	struct udphdr *uh;
+	int payload_len;
+	u32 versions;
+	int psp_off;
+	bool is_udp;
+	int l3_hlen;
+	u8 version;
+	u32 psd_id;
+	int err;
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		struct iphdr *iph;
+
+		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+			return false;
+
+		iph = (struct iphdr *)skb->data;
+		if (iph->ihl < 5)
+			return false;
+
+		is_udp = iph->protocol == IPPROTO_UDP;
+		l3_hlen = iph->ihl * 4;
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		struct ipv6hdr *ip6h;
+
+		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
+			return false;
+		ip6h = (struct ipv6hdr *)skb->data;
+		is_udp = ip6h->nexthdr == IPPROTO_UDP;
+		l3_hlen = sizeof(struct ipv6hdr);
+	} else {
+		return false;
+	}
+
+	if (!is_udp)
+		return false;
+
+	if (!pskb_may_pull(skb, l3_hlen + sizeof(struct udphdr) + PSP_HDR_SIZE))
+		return false;
+
+	uh = (struct udphdr *)(skb->data + l3_hlen);
+	if (uh->dest != htons(PSP_DEFAULT_UDP_PORT))
+		return false;
+
+	psph = (struct psphdr *)(uh + 1);
+	version = FIELD_GET(PSPHDR_VERFL_VERSION, psph->verfl);
+
+	rcu_read_lock();
+	psd = rcu_dereference(ns->psp.dev);
+	if (psd) {
+		versions = READ_ONCE(psd->config.versions);
+		psd_id = psd->id;
+	}
+	rcu_read_unlock();
+
+	if (!psd || !(versions & (1 << version))) {
+		skb->ip_summed = CHECKSUM_NONE;
+		return false;
+	}
+
+	psp_off = l3_hlen + sizeof(struct udphdr);
+	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
+	if (payload_len < 0)
+		goto drop;
+
+	skb_push(skb, ETH_HLEN);
+	skb->mac_len = ETH_HLEN;
+	err = psp_dev_rcv(skb, psd_id, 0, false);
+	if (err)
+		goto drop;
+
+	skb_reset_mac_header(skb);
+	skb_pull(skb, ETH_HLEN);
+	skb->decrypted = 1;
+
+	u64_stats_update_begin(&ns->psp.syncp);
+	u64_stats_inc(&ns->psp.rx_packets);
+	u64_stats_add(&ns->psp.rx_bytes, payload_len);
+	u64_stats_update_end(&ns->psp.syncp);
+
+	return false;
+
+drop:
+	kfree_skb_reason(skb, SKB_DROP_REASON_PSP_INPUT);
+	return true;
+}
+
 static int
 nsim_psp_set_config(struct psp_dev *psd, struct psp_dev_config *conf,
 		    struct netlink_ext_ack *extack)

-- 
2.52.0

[PATCH net-next 4/6] netdevsim: psp: implement kdf from psp spec

[Daniel Zahka]

Implement the PSP key derivation function (KDF) per the PSP
Architecture Spec.

The kdf is used to generate spi + session key pairs, and will also be
used in the rx path to re-derive the tx key used by the peer.

Also, remove support for psd->generation, as it is not needed for
netdevsim after removing the fake authentication hack.

Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/Kconfig               |  1 +
 drivers/net/netdevsim/netdevsim.h |  3 +++
 drivers/net/netdevsim/psp.c       | 55 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index ff79c466712d..44a220c05536 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -610,6 +610,7 @@ config NETDEVSIM
 	select NET_DEVLINK
 	select PAGE_POOL
 	select NET_SHAPER
+	select CRYPTO_LIB_AES_CBC_MACS if INET_PSP
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index dcea76429bac..112fe1ee10bc 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -38,6 +38,7 @@
 #define NSIM_IPSEC_VALID		BIT(31)
 #define NSIM_UDP_TUNNEL_N_PORTS		4
 
+#define NSIM_PSP_DEV_KEY_SIZE		32
 #define NSIM_HDS_THRESHOLD_MAX		1024
 
 struct nsim_sa {
@@ -123,6 +124,8 @@ struct netdevsim {
 		struct psp_dev __rcu *dev;
 		struct dentry *rereg;
 		struct mutex rereg_lock;
+		spinlock_t dev_keys_lock;
+		u8 dev_keys[2][NSIM_PSP_DEV_KEY_SIZE];
 		u32 spi;
 		u32 assoc_cnt;
 	} psp;
diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index e8831d4bf394..8cdb88b1e232 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -1,11 +1,45 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <crypto/aes-cbc-macs.h>
+#include <linux/random.h>
 #include <linux/skbuff.h>
+#include <linux/unaligned.h>
 #include <net/psp.h>
 #include <net/sock.h>
 
 #include "netdevsim.h"
 
+/* Session key derivation from device key per PSP Architecture Spec */
+static void nsim_psp_derive_key(const u8 *dev_key, __be32 spi, u32 version,
+				u8 *derived_key)
+{
+	unsigned int key_size = psp_key_size(version);
+	struct aes_cmac_key key;
+	u8 block[16];
+
+	aes_cmac_preparekey(&key, dev_key, NSIM_PSP_DEV_KEY_SIZE);
+
+	block[0]  = 0x00;
+	block[1]  = 0x00;
+	block[2]  = 0x00;
+	block[3]  = 0x01;		/* counter */
+	block[4]  = 0x50;		/* 'P' */
+	block[5]  = 0x76;		/* 'v' */
+	block[6]  = 0x30 | version;	/* '0' + version */
+	block[7]  = 0x00;
+	memcpy(&block[8], &spi, sizeof(spi));
+	put_unaligned_be32(key_size * 8, &block[12]);
+
+	aes_cmac(&key, block, sizeof(block), derived_key);
+
+	if (key_size > 16) {
+		block[3] = 0x02;
+		aes_cmac(&key, block, sizeof(block), derived_key + 16);
+	}
+
+	memzero_explicit(&key, sizeof(key));
+}
+
 enum skb_drop_reason
 nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 {
@@ -155,7 +189,7 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
 		  struct netlink_ext_ack *extack)
 {
 	struct netdevsim *ns = psd->drv_priv;
-	int i;
+	unsigned int phase;
 
 	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
 		NL_SET_ERR_MSG(extack, "SPI space exhausted");
@@ -163,9 +197,11 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
 	}
 
 	assoc->spi = cpu_to_be32(++ns->psp.spi);
-	assoc->key[0] = psd->generation;
-	for (i = 1; i < PSP_MAX_KEY; i++)
-		assoc->key[i] = ns->psp.spi + i;
+	phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
+
+	/* dev_keys_lock not needed because of psd->lock */
+	nsim_psp_derive_key(ns->psp.dev_keys[phase], assoc->spi, version,
+			    assoc->key);
 
 	return 0;
 }
@@ -186,8 +222,15 @@ static int nsim_assoc_add(struct psp_dev *psd, struct psp_assoc *pas,
 static int nsim_key_rotate(struct psp_dev *psd, struct netlink_ext_ack *extack)
 {
 	struct netdevsim *ns = psd->drv_priv;
+	unsigned int next_phase;
 
+	psd->generation = 0;
 	ns->psp.spi = (ns->psp.spi & PSP_SPI_KEY_PHASE) ^ PSP_SPI_KEY_PHASE;
+	next_phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
+
+	spin_lock_bh(&ns->psp.dev_keys_lock);
+	get_random_bytes(ns->psp.dev_keys[next_phase], NSIM_PSP_DEV_KEY_SIZE);
+	spin_unlock_bh(&ns->psp.dev_keys_lock);
 
 	return 0;
 }
@@ -295,6 +338,10 @@ int nsim_psp_init(struct netdevsim *ns)
 	struct dentry *ddir = ns->nsim_dev_port->ddir;
 	struct psp_dev *psd;
 
+	spin_lock_init(&ns->psp.dev_keys_lock);
+	get_random_bytes(ns->psp.dev_keys[0], NSIM_PSP_DEV_KEY_SIZE);
+	get_random_bytes(ns->psp.dev_keys[1], NSIM_PSP_DEV_KEY_SIZE);
+
 	psd = psp_dev_create(ns->netdev, &nsim_psp_ops, &nsim_psp_caps, ns);
 	if (IS_ERR(psd))
 		return PTR_ERR(psd);

-- 
2.52.0

[PATCH net-next 5/6] netdevsim: psp: add real aes-gcm encryption and decryption

[Daniel Zahka]

Implement real AES-GCM-128/256 encryption and decryption for PSP
packets in the netdevsim driver, and remove gmac from supported
versions.

We now have to add and remove the PSP ICV trailer from packets. We
linearize skb's because the aesgcm crypto library does not work on
non-linear buffers.

Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/Kconfig         |  1 +
 drivers/net/netdevsim/psp.c | 98 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 44a220c05536..2d21ba13de15 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -611,6 +611,7 @@ config NETDEVSIM
 	select PAGE_POOL
 	select NET_SHAPER
 	select CRYPTO_LIB_AES_CBC_MACS if INET_PSP
+	select CRYPTO_LIB_AESGCM if INET_PSP
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 8cdb88b1e232..4945973d523d 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <crypto/aes-cbc-macs.h>
+#include <crypto/gcm.h>
+#include <linux/ip.h>
 #include <linux/random.h>
 #include <linux/skbuff.h>
+#include <linux/timekeeping.h>
 #include <linux/unaligned.h>
 #include <net/psp.h>
 #include <net/sock.h>
@@ -44,9 +47,17 @@ enum skb_drop_reason
 nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 {
 	enum skb_drop_reason rc = 0;
+	u8 iv[GCM_AES_IV_SIZE];
+	struct aesgcm_ctx ctx;
 	struct psp_assoc *pas;
+	unsigned int key_size;
+	struct psphdr *psph;
+	int payload_len;
 	struct net *net;
+	u8 *authtag;
+	int psp_off;
 	void **ptr;
+	int err;
 
 	rcu_read_lock();
 	pas = psp_skb_get_assoc_rcu(skb);
@@ -72,12 +83,52 @@ nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 		goto out_unlock;
 	}
 
+	key_size = psp_key_size(pas->version);
+	err = aesgcm_expandkey(&ctx, pas->tx.key, key_size, PSP_TRL_SIZE);
+	if (err) {
+		rc = SKB_DROP_REASON_PSP_OUTPUT;
+		goto out_unlock;
+	}
+
+	if (skb_linearize_cow(skb) ||
+	    (skb_tailroom(skb) < PSP_TRL_SIZE &&
+	     pskb_expand_head(skb, 0, PSP_TRL_SIZE, GFP_ATOMIC))) {
+		rc = SKB_DROP_REASON_PSP_OUTPUT;
+		goto out_unlock;
+	}
+	skb_put(skb, PSP_TRL_SIZE);
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		be16_add_cpu(&ip_hdr(skb)->tot_len, PSP_TRL_SIZE);
+		ip_send_check(ip_hdr(skb));
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_TRL_SIZE);
+	}
+	be16_add_cpu(&udp_hdr(skb)->len, PSP_TRL_SIZE);
+
+	psph = (struct psphdr *)(skb_transport_header(skb) +
+				sizeof(struct udphdr));
+
+	/* Real impl needs to guarantee IV isn't reused on the same key */
+	psph->iv = cpu_to_be64(ktime_get_mono_fast_ns());
+	memcpy(iv, &psph->spi, sizeof(psph->spi));
+	memcpy(iv + sizeof(psph->spi), &psph->iv, sizeof(psph->iv));
+	psp_off = skb_transport_offset(skb) + sizeof(struct udphdr);
+	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
+	authtag = skb->data + skb->len - PSP_TRL_SIZE;
+
+	aesgcm_encrypt(&ctx,
+		       skb->data + psp_off + PSP_HDR_SIZE,
+		       skb->data + psp_off + PSP_HDR_SIZE,
+		       payload_len, (u8 *)psph, PSP_HDR_SIZE,
+		       iv, authtag);
+	memzero_explicit(&ctx, sizeof(ctx));
+
 	skb->decrypted = 0;
 
 	u64_stats_update_begin(&ns->psp.syncp);
 	u64_stats_inc(&ns->psp.tx_packets);
-	u64_stats_add(&ns->psp.tx_bytes,
-		      skb->len - skb_inner_transport_offset(skb));
+	u64_stats_add(&ns->psp.tx_bytes, payload_len);
 	u64_stats_update_end(&ns->psp.syncp);
 out_unlock:
 	rcu_read_unlock();
@@ -87,12 +138,17 @@ nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
 /* Returns true if skb was consumed, false otherwise. */
 bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
 {
+	u8 iv[GCM_AES_IV_SIZE];
+	struct aesgcm_ctx ctx;
 	struct psp_dev *psd;
+	u8 key[PSP_MAX_KEY];
 	struct psphdr *psph;
+	unsigned int phase;
 	struct udphdr *uh;
 	int payload_len;
 	u32 versions;
 	int psp_off;
+	u8 *authtag;
 	bool is_udp;
 	int l3_hlen;
 	u8 version;
@@ -154,9 +210,41 @@ bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
 	if (payload_len < 0)
 		goto drop;
 
+	if (FIELD_GET(PSPHDR_CRYPT_OFFSET, psph->crypt_offset))
+		goto drop;
+
+	if (skb_linearize_cow(skb))
+		goto drop;
+
+	psph = (struct psphdr *)(skb->data + psp_off);
+	phase = !!(ntohl(psph->spi) & PSP_SPI_KEY_PHASE);
+
+	spin_lock_bh(&ns->psp.dev_keys_lock);
+	nsim_psp_derive_key(ns->psp.dev_keys[phase], psph->spi, version, key);
+	spin_unlock_bh(&ns->psp.dev_keys_lock);
+
+	err = aesgcm_expandkey(&ctx, key, psp_key_size(version), PSP_TRL_SIZE);
+	memzero_explicit(key, sizeof(key));
+	if (err)
+		goto drop;
+
+	memcpy(iv, &psph->spi, sizeof(psph->spi));
+	memcpy(iv + sizeof(psph->spi), &psph->iv, sizeof(psph->iv));
+	authtag = skb->data + skb->len - PSP_TRL_SIZE;
+
+	if (!aesgcm_decrypt(&ctx,
+			    skb->data + psp_off + PSP_HDR_SIZE,
+			    skb->data + psp_off + PSP_HDR_SIZE,
+			    payload_len, (u8 *)psph, PSP_HDR_SIZE,
+			    iv, authtag)) {
+		memzero_explicit(&ctx, sizeof(ctx));
+		goto drop;
+	}
+	memzero_explicit(&ctx, sizeof(ctx));
+
 	skb_push(skb, ETH_HLEN);
 	skb->mac_len = ETH_HLEN;
-	err = psp_dev_rcv(skb, psd_id, 0, false);
+	err = psp_dev_rcv(skb, psd_id, 0, true);
 	if (err)
 		goto drop;
 
@@ -274,9 +362,7 @@ static struct psp_dev_ops nsim_psp_ops = {
 
 static struct psp_dev_caps nsim_psp_caps = {
 	.versions = 1 << PSP_VERSION_HDR0_AES_GCM_128 |
-		    1 << PSP_VERSION_HDR0_AES_GMAC_128 |
-		    1 << PSP_VERSION_HDR0_AES_GCM_256 |
-		    1 << PSP_VERSION_HDR0_AES_GMAC_256,
+		    1 << PSP_VERSION_HDR0_AES_GCM_256,
 	.assoc_drv_spc = sizeof(void *),
 };
 

-- 
2.52.0

[PATCH net-next 6/6] netdevsim: psp: count rx authentication and length errors

[Daniel Zahka]

Now that netdevsim does psp parsing and aes-gcm decryption in rx, we
can report authentication and length errors in the psp stats api.

Assisted-by: Claude:claude-opus-4.7
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/netdevsim/netdevsim.h |  2 ++
 drivers/net/netdevsim/psp.c       | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 112fe1ee10bc..5f0f638ee893 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -118,6 +118,8 @@ struct netdevsim {
 	struct {
 		u64_stats_t rx_packets;
 		u64_stats_t rx_bytes;
+		u64_stats_t rx_auth_fail;
+		u64_stats_t rx_error;
 		u64_stats_t tx_packets;
 		u64_stats_t tx_bytes;
 		struct u64_stats_sync syncp;
diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 4945973d523d..053c01cc310d 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -207,8 +207,12 @@ bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
 
 	psp_off = l3_hlen + sizeof(struct udphdr);
 	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
-	if (payload_len < 0)
+	if (payload_len < 0) {
+		u64_stats_update_begin(&ns->psp.syncp);
+		u64_stats_inc(&ns->psp.rx_error);
+		u64_stats_update_end(&ns->psp.syncp);
 		goto drop;
+	}
 
 	if (FIELD_GET(PSPHDR_CRYPT_OFFSET, psph->crypt_offset))
 		goto drop;
@@ -238,6 +242,9 @@ bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
 			    payload_len, (u8 *)psph, PSP_HDR_SIZE,
 			    iv, authtag)) {
 		memzero_explicit(&ctx, sizeof(ctx));
+		u64_stats_update_begin(&ns->psp.syncp);
+		u64_stats_inc(&ns->psp.rx_auth_fail);
+		u64_stats_update_end(&ns->psp.syncp);
 		goto drop;
 	}
 	memzero_explicit(&ctx, sizeof(ctx));
@@ -346,6 +353,8 @@ static void nsim_get_stats(struct psp_dev *psd, struct psp_dev_stats *stats)
 		start = u64_stats_fetch_begin(&ns->psp.syncp);
 		stats->rx_bytes = u64_stats_read(&ns->psp.rx_bytes);
 		stats->rx_packets = u64_stats_read(&ns->psp.rx_packets);
+		stats->rx_auth_fail = u64_stats_read(&ns->psp.rx_auth_fail);
+		stats->rx_error = u64_stats_read(&ns->psp.rx_error);
 		stats->tx_bytes = u64_stats_read(&ns->psp.tx_bytes);
 		stats->tx_packets = u64_stats_read(&ns->psp.tx_packets);
 	} while (u64_stats_fetch_retry(&ns->psp.syncp, start));

-- 
2.52.0

Re: [PATCH net-next 1/6] netdevsim: psp: reset spi on key rotation and check for exhaustion on alloc

[Willem de Bruijn]

Daniel Zahka wrote:
> The PSP spec states that the lower 31b of the SPI need to be
> non-zero. Though not in the spec, I think it is reasonable to reset
> the lower 31b of the spi space after a key rotation, and to also
> decline to generate session keys when the lower 31b saturate.

Since this is already a 6 patch series, these two independent changes
could be separate patches.

Agreed on both points btw.
 
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
>  drivers/net/netdevsim/psp.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
> index 6936ecb8173e..5073bda60883 100644
> --- a/drivers/net/netdevsim/psp.c
> +++ b/drivers/net/netdevsim/psp.c
> @@ -132,14 +132,14 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>  		  struct netlink_ext_ack *extack)
>  {
>  	struct netdevsim *ns = psd->drv_priv;
> -	unsigned int new;
>  	int i;
>  
> -	new = ++ns->psp.spi & PSP_SPI_KEY_ID;
> -	if (psd->generation & 1)
> -		new |= PSP_SPI_KEY_PHASE;
> +	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
> +		NL_SET_ERR_MSG(extack, "SPI space exhausted");
> +		return -ENOSPC;
> +	}

Can this all be more readable without the use of XORs? This is not hot
path code that needs to be optimized.

        if (ns->psp.spi & PSP_SPI_KEY_ID == INT32_MAX) {
                NL_SET_ERR_MSG(extack, "SPI space exhausted");
                return -ENOSPC;
        }
  
> -	assoc->spi = cpu_to_be32(new);
> +	assoc->spi = cpu_to_be32(++ns->psp.spi);
>  	assoc->key[0] = psd->generation;
>  	for (i = 1; i < PSP_MAX_KEY; i++)
>  		assoc->key[i] = ns->psp.spi + i;
> @@ -162,6 +162,10 @@ static int nsim_assoc_add(struct psp_dev *psd, struct psp_assoc *pas,
>  
>  static int nsim_key_rotate(struct psp_dev *psd, struct netlink_ext_ack *extack)
>  {
> +	struct netdevsim *ns = psd->drv_priv;
> +
> +	ns->psp.spi = (ns->psp.spi & PSP_SPI_KEY_PHASE) ^ PSP_SPI_KEY_PHASE;
> +

        /* Flip key phase and reset SPI to 0 within that space
         * (will be pre-incremented, as 0 is an invalid SPI)
         */
        if (ns->psp.spi & PSP_SPI_KEY_PHASE)
                ns->psp.spi = 0;
        else
                ns->psp.spi = PSP_SPI_KEY_PHASE;

Even while making the code more self evident I still feel it needs a
comment to explain all that is going on.

Maybe it can be more self-evident.

Or maybe you find the XOR just as readable already.

Your call. A comment might be helpful either way.


>  	return 0;
>  }
>  
> 
> -- 
> 2.52.0
> 

Re: [PATCH net-next 2/6] netdevsim: psp: remove unnecessary UDP checksum computation

[Willem de Bruijn]

Daniel Zahka wrote:
> The PSP spec requires the implementations accept 0 checksum in psp-udp
> header. Let's take advantage of that to trim netdevsim's psp code
> down. psp_dev_encapsulate() already sets uh->check to 0.
> 
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
>  drivers/net/netdevsim/psp.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
> index 5073bda60883..75740e2a731f 100644
> --- a/drivers/net/netdevsim/psp.c
> +++ b/drivers/net/netdevsim/psp.c
> @@ -1,8 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> -#include <linux/ip.h>
>  #include <linux/skbuff.h>
> -#include <net/ip6_checksum.h>
>  #include <net/psp.h>
>  #include <net/sock.h>
>  
> @@ -81,36 +79,6 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
>  			      skb->len - skb_inner_transport_offset(skb));
>  		u64_stats_update_end(&ns->psp.syncp);
>  	} else {
> -		struct ipv6hdr *ip6h __maybe_unused;
> -		struct iphdr *iph;
> -		struct udphdr *uh;
> -		__wsum csum;
> -
> -		/* Do not decapsulate. Receive the skb with the udp and psp
> -		 * headers still there as if this is a normal udp packet.
> -		 * psp_dev_encapsulate() sets udp checksum to 0, so we need to
> -		 * provide a valid checksum here, so the skb isn't dropped.
> -		 */

Perhaps this was here as IPv6 does not allow zero checksums except for
tunneling in specific cases (RFC 6936)?

It seems benign enough to keep the code. Do you have a specific reason
to remove it, beyond reducing LoC?

Re: [PATCH net-next 2/6] netdevsim: psp: remove unnecessary UDP checksum computation

[Daniel Zahka]

On 5/11/26 1:01 PM, Willem de Bruijn wrote:
> Daniel Zahka wrote:
>> The PSP spec requires the implementations accept 0 checksum in psp-udp
>> header. Let's take advantage of that to trim netdevsim's psp code
>> down. psp_dev_encapsulate() already sets uh->check to 0.
>>
>> Assisted-by: Claude:claude-opus-4.6
>> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
>> ---
>>   drivers/net/netdevsim/psp.c | 32 --------------------------------
>>   1 file changed, 32 deletions(-)
>>
>> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
>> index 5073bda60883..75740e2a731f 100644
>> --- a/drivers/net/netdevsim/psp.c
>> +++ b/drivers/net/netdevsim/psp.c
>> @@ -1,8 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   
>> -#include <linux/ip.h>
>>   #include <linux/skbuff.h>
>> -#include <net/ip6_checksum.h>
>>   #include <net/psp.h>
>>   #include <net/sock.h>
>>   
>> @@ -81,36 +79,6 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
>>   			      skb->len - skb_inner_transport_offset(skb));
>>   		u64_stats_update_end(&ns->psp.syncp);
>>   	} else {
>> -		struct ipv6hdr *ip6h __maybe_unused;
>> -		struct iphdr *iph;
>> -		struct udphdr *uh;
>> -		__wsum csum;
>> -
>> -		/* Do not decapsulate. Receive the skb with the udp and psp
>> -		 * headers still there as if this is a normal udp packet.
>> -		 * psp_dev_encapsulate() sets udp checksum to 0, so we need to
>> -		 * provide a valid checksum here, so the skb isn't dropped.
>> -		 */
> Perhaps this was here as IPv6 does not allow zero checksums except for
> tunneling in specific cases (RFC 6936)?


Yes it was originally here for IPv6. It was needed to make a test case 
pass that ultimately never got upstreamed (yet). The test basically used 
the psp_dev_ops::set_config() function to turn psp rx off midflow, and 
then tried to catch packets with a udp socket listening on port 1000. At 
the time I didn't realize that I could probably make the test work by 
opting the socket into a setting for RFC 6936 with setsockopt().


> It seems benign enough to keep the code. Do you have a specific reason
> to remove it, beyond reducing LoC?


It was just to reduce LoC for the series :) I can put it back in if you 
like, though as I mentioned, none of the current tests cover that the 
psp-udp csum is correct.

Re: [PATCH net-next 2/6] netdevsim: psp: remove unnecessary UDP checksum computation

[Willem de Bruijn]

Daniel Zahka wrote:
> 
> On 5/11/26 1:01 PM, Willem de Bruijn wrote:
> > Daniel Zahka wrote:
> >> The PSP spec requires the implementations accept 0 checksum in psp-udp
> >> header. Let's take advantage of that to trim netdevsim's psp code
> >> down. psp_dev_encapsulate() already sets uh->check to 0.
> >>
> >> Assisted-by: Claude:claude-opus-4.6
> >> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> >> ---
> >>   drivers/net/netdevsim/psp.c | 32 --------------------------------
> >>   1 file changed, 32 deletions(-)
> >>
> >> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
> >> index 5073bda60883..75740e2a731f 100644
> >> --- a/drivers/net/netdevsim/psp.c
> >> +++ b/drivers/net/netdevsim/psp.c
> >> @@ -1,8 +1,6 @@
> >>   // SPDX-License-Identifier: GPL-2.0
> >>   
> >> -#include <linux/ip.h>
> >>   #include <linux/skbuff.h>
> >> -#include <net/ip6_checksum.h>
> >>   #include <net/psp.h>
> >>   #include <net/sock.h>
> >>   
> >> @@ -81,36 +79,6 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
> >>   			      skb->len - skb_inner_transport_offset(skb));
> >>   		u64_stats_update_end(&ns->psp.syncp);
> >>   	} else {
> >> -		struct ipv6hdr *ip6h __maybe_unused;
> >> -		struct iphdr *iph;
> >> -		struct udphdr *uh;
> >> -		__wsum csum;
> >> -
> >> -		/* Do not decapsulate. Receive the skb with the udp and psp
> >> -		 * headers still there as if this is a normal udp packet.
> >> -		 * psp_dev_encapsulate() sets udp checksum to 0, so we need to
> >> -		 * provide a valid checksum here, so the skb isn't dropped.
> >> -		 */
> > Perhaps this was here as IPv6 does not allow zero checksums except for
> > tunneling in specific cases (RFC 6936)?
> 
> 
> Yes it was originally here for IPv6. It was needed to make a test case 
> pass that ultimately never got upstreamed (yet). The test basically used 
> the psp_dev_ops::set_config() function to turn psp rx off midflow, and 
> then tried to catch packets with a udp socket listening on port 1000. At 
> the time I didn't realize that I could probably make the test work by 
> opting the socket into a setting for RFC 6936 with setsockopt().

Just curious: which setsockopt is this?

> 
> > It seems benign enough to keep the code. Do you have a specific reason
> > to remove it, beyond reducing LoC?
> 
> 
> It was just to reduce LoC for the series :) I can put it back in if you 
> like,

No, fine to remove then

> though as I mentioned, none of the current tests cover that the 
> psp-udp csum is correct.

.. actually based on this, preferable even

Re: [PATCH net-next 2/6] netdevsim: psp: remove unnecessary UDP checksum computation

[Daniel Zahka]

On 5/11/26 3:01 PM, Willem de Bruijn wrote:
> Daniel Zahka wrote:
>    
> @@ -81,36 +79,6 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
>    			      skb->len - skb_inner_transport_offset(skb));
>    		u64_stats_update_end(&ns->psp.syncp);
>    	} else {
> -		struct ipv6hdr *ip6h __maybe_unused;
> -		struct iphdr *iph;
> -		struct udphdr *uh;
> -		__wsum csum;
> -
> -		/* Do not decapsulate. Receive the skb with the udp and psp
> -		 * headers still there as if this is a normal udp packet.
> -		 * psp_dev_encapsulate() sets udp checksum to 0, so we need to
> -		 * provide a valid checksum here, so the skb isn't dropped.
> -		 */
>>> Perhaps this was here as IPv6 does not allow zero checksums except for
>>> tunneling in specific cases (RFC 6936)?
>>
>> Yes it was originally here for IPv6. It was needed to make a test case
>> pass that ultimately never got upstreamed (yet). The test basically used
>> the psp_dev_ops::set_config() function to turn psp rx off midflow, and
>> then tried to catch packets with a udp socket listening on port 1000. At
>> the time I didn't realize that I could probably make the test work by
>> opting the socket into a setting for RFC 6936 with setsockopt().
> Just curious: which setsockopt is this?

I was thinking of UDP_NO_CHECK6_RX, though I probably shouldn't have 
characterized it as relating to RFC 6936. I haven't dug through the code 
or history to figure out if that was added for tunnels or to implement 
that rfc. I can see that tunnel driver callers of udp_sock_create() can 
configure the 0 checksum behavior, which seems to actually implement 
what's described in the rfc.

>> though as I mentioned, none of the current tests cover that the
>> psp-udp csum is correct.
> .. actually based on this, preferable even


I agree. My plan would be to add the psp-udp checksum calculation back 
when we have a test that actually depends on it to pass.

Re: [PATCH net-next 4/6] netdevsim: psp: implement kdf from psp spec

[Willem de Bruijn]

Daniel Zahka wrote:
> Implement the PSP key derivation function (KDF) per the PSP
> Architecture Spec.
> 
> The kdf is used to generate spi + session key pairs, and will also be

Text is a bit ambiguous here: the kdf does not generate the spi. It
derives a session key from the master key and spi.

> used in the rx path to re-derive the tx key used by the peer.
> 
> Also, remove support for psd->generation, as it is not needed for
> netdevsim after removing the fake authentication hack.

Is psd->generation only used inside driver code, not by the core PSP
stack? Else it should be set to !!(ns->psp.spi & PSP_SPI_KEY_PHASE) on
key rotation. If only used by the driver, no need to reset it on each
rotation.

> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

>  enum skb_drop_reason
>  nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
>  {
> @@ -155,7 +189,7 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>  		  struct netlink_ext_ack *extack)
>  {
>  	struct netdevsim *ns = psd->drv_priv;
> -	int i;
> +	unsigned int phase;
>  
>  	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
>  		NL_SET_ERR_MSG(extack, "SPI space exhausted");
> @@ -163,9 +197,11 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>  	}
>  
>  	assoc->spi = cpu_to_be32(++ns->psp.spi);
> -	assoc->key[0] = psd->generation;
> -	for (i = 1; i < PSP_MAX_KEY; i++)
> -		assoc->key[i] = ns->psp.spi + i;
> +	phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
> +
> +	/* dev_keys_lock not needed because of psd->lock */

Can you elaborate a bit?

Is dev_keys_lock only used to synchronize the writers, then? Which after
device init would only be concurrent invocations of nsim_key_rotate. But
that operation correctly also holds the device lock using
psp_device_get_locked.

> +	nsim_psp_derive_key(ns->psp.dev_keys[phase], assoc->spi, version,
> +			    assoc->key);
>  
>  	return 0;
>  }
> @@ -186,8 +222,15 @@ static int nsim_assoc_add(struct psp_dev *psd, struct psp_assoc *pas,
>  static int nsim_key_rotate(struct psp_dev *psd, struct netlink_ext_ack *extack)
>  {
>  	struct netdevsim *ns = psd->drv_priv;
> +	unsigned int next_phase;
>  
> +	psd->generation = 0;
>  	ns->psp.spi = (ns->psp.spi & PSP_SPI_KEY_PHASE) ^ PSP_SPI_KEY_PHASE;
> +	next_phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
> +
> +	spin_lock_bh(&ns->psp.dev_keys_lock);
> +	get_random_bytes(ns->psp.dev_keys[next_phase], NSIM_PSP_DEV_KEY_SIZE);
> +	spin_unlock_bh(&ns->psp.dev_keys_lock);
>  
>  	return 0;
>  }
> @@ -295,6 +338,10 @@ int nsim_psp_init(struct netdevsim *ns)
>  	struct dentry *ddir = ns->nsim_dev_port->ddir;
>  	struct psp_dev *psd;
>  
> +	spin_lock_init(&ns->psp.dev_keys_lock);
> +	get_random_bytes(ns->psp.dev_keys[0], NSIM_PSP_DEV_KEY_SIZE);
> +	get_random_bytes(ns->psp.dev_keys[1], NSIM_PSP_DEV_KEY_SIZE);
> +
>  	psd = psp_dev_create(ns->netdev, &nsim_psp_ops, &nsim_psp_caps, ns);
>  	if (IS_ERR(psd))
>  		return PTR_ERR(psd);
> 
> -- 
> 2.52.0
> 

Re: [PATCH net-next 3/6] netdevsim: psp: move rx processing into nsim_poll()

[Willem de Bruijn]

Daniel Zahka wrote:
> nsim_do_psp() does PSP decap and skb extension creation in the tx
> path. This has the slightly undesirable property of not allowing the
> psp rx code to run on PSP packets cooked up in userspace and
> transmitted on a packet socket from the peer dev (e.g. packetdrill).

Whether this happens in the nsim_start_xmit tx side handler directly
or is deferred to nsim_napi_rx is irrelevant, isn't it?


> This commit instead triggers the psp rx path just based on parsing the
> received skb. The current code relies on a bit of a hack to simulate
> authentication with the proper key: the peer's psd->generation was
> placed into the tx key, and during decap used to fill out the
> extension the packet before being sent up the psp rx path. This commit
> removes that hack, which creates a transient break in psp.py test
> cases that rely on this behavior (e.g. data_send_bad_key). Subsequent
> commits which introduce real aes-gcm crypto will restore the correct
> behavior.
> 
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

> +/* Returns true if skb was consumed, false otherwise. */
> +bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
> +{
> +	struct psp_dev *psd;
> +	struct psphdr *psph;
> +	struct udphdr *uh;
> +	int payload_len;
> +	u32 versions;
> +	int psp_off;
> +	bool is_udp;
> +	int l3_hlen;
> +	u8 version;
> +	u32 psd_id;
> +	int err;
> +
> +	if (skb->protocol == htons(ETH_P_IP)) {
> +		struct iphdr *iph;
> +
> +		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> +			return false;
> +
> +		iph = (struct iphdr *)skb->data;
> +		if (iph->ihl < 5)
> +			return false;
> +
> +		is_udp = iph->protocol == IPPROTO_UDP;
> +		l3_hlen = iph->ihl * 4;
> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +		struct ipv6hdr *ip6h;
> +
> +		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> +			return false;
> +		ip6h = (struct ipv6hdr *)skb->data;
> +		is_udp = ip6h->nexthdr == IPPROTO_UDP;
> +		l3_hlen = sizeof(struct ipv6hdr);
> +	} else {
> +		return false;
> +	}
> +
> +	if (!is_udp)
> +		return false;
> +
> +	if (!pskb_may_pull(skb, l3_hlen + sizeof(struct udphdr) + PSP_HDR_SIZE))
> +		return false;
> +
> +	uh = (struct udphdr *)(skb->data + l3_hlen);
> +	if (uh->dest != htons(PSP_DEFAULT_UDP_PORT))
> +		return false;
> +
> +	psph = (struct psphdr *)(uh + 1);
> +	version = FIELD_GET(PSPHDR_VERFL_VERSION, psph->verfl);

This seems to reimplement a lot of psp_dev_rcv. Is that needed?

Is it a hint that this psp driver API needs some work?

> +	rcu_read_lock();
> +	psd = rcu_dereference(ns->psp.dev);
> +	if (psd) {
> +		versions = READ_ONCE(psd->config.versions);
> +		psd_id = psd->id;
> +	}
> +	rcu_read_unlock();
> +
> +	if (!psd || !(versions & (1 << version))) {
> +		skb->ip_summed = CHECKSUM_NONE;
> +		return false;
> +	}
> +
> +	psp_off = l3_hlen + sizeof(struct udphdr);
> +	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
> +	if (payload_len < 0)
> +		goto drop;
> +
> +	skb_push(skb, ETH_HLEN);
> +	skb->mac_len = ETH_HLEN;
> +	err = psp_dev_rcv(skb, psd_id, 0, false);
> +	if (err)
> +		goto drop;
> +
> +	skb_reset_mac_header(skb);
> +	skb_pull(skb, ETH_HLEN);

Similarly this is a bit of a hack, pushing and pulling a fake Ethernet
offset.

And is this skb_reset_mac_header needed?

> +	skb->decrypted = 1;
> +
> +	u64_stats_update_begin(&ns->psp.syncp);
> +	u64_stats_inc(&ns->psp.rx_packets);
> +	u64_stats_add(&ns->psp.rx_bytes, payload_len);
> +	u64_stats_update_end(&ns->psp.syncp);
> +
> +	return false;
> +
> +drop:
> +	kfree_skb_reason(skb, SKB_DROP_REASON_PSP_INPUT);
> +	return true;
> +}
> +
>  static int
>  nsim_psp_set_config(struct psp_dev *psd, struct psp_dev_config *conf,
>  		    struct netlink_ext_ack *extack)
> 
> -- 
> 2.52.0
> 

Re: [PATCH net-next 5/6] netdevsim: psp: add real aes-gcm encryption and decryption

[Willem de Bruijn]

Daniel Zahka wrote:
> Implement real AES-GCM-128/256 encryption and decryption for PSP
> packets in the netdevsim driver, and remove gmac from supported
> versions.
> 
> We now have to add and remove the PSP ICV trailer from packets. We
> linearize skb's because the aesgcm crypto library does not work on
> non-linear buffers.
> 
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next 6/6] netdevsim: psp: count rx authentication and length errors

[Willem de Bruijn]

Daniel Zahka wrote:
> Now that netdevsim does psp parsing and aes-gcm decryption in rx, we
> can report authentication and length errors in the psp stats api.
> 
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next 4/6] netdevsim: psp: implement kdf from psp spec

[Daniel Zahka]

On 5/11/26 3:49 PM, Willem de Bruijn wrote:
> Daniel Zahka wrote:
>> Implement the PSP key derivation function (KDF) per the PSP
>> Architecture Spec.
>>
>> The kdf is used to generate spi + session key pairs, and will also be
> Text is a bit ambiguous here: the kdf does not generate the spi. It
> derives a session key from the master key and spi.
>
>> used in the rx path to re-derive the tx key used by the peer.
>>
>> Also, remove support for psd->generation, as it is not needed for
>> netdevsim after removing the fake authentication hack.
> Is psd->generation only used inside driver code, not by the core PSP
> stack? Else it should be set to !!(ns->psp.spi & PSP_SPI_KEY_PHASE) on
> key rotation. If only used by the driver, no need to reset it on each
> rotation.


Core tries to 'suggest' a generation to the driver, which is the last 
generation + 1, before calling into key_rotate(), but this won't work 
for netdevsim. I could set the generation to !!(ns->psp.spi & 
PSP_SPI_KEY_PHASE) so that it aliases the device key selection bit, but 
I think this is basically the same as just setting it to 0. This series 
makes the generation field dead code in core. The old netdevsim 
implementation relied on it to do its fake authentication hack, but that 
is removed with this series. The only real hw implementation we have, 
mlx5, does not support device key generations. Maybe we need to just 
remove that until a real driver comes along as a user.


>> Assisted-by: Claude:claude-opus-4.6
>> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
>>   enum skb_drop_reason
>>   nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
>>   {
>> @@ -155,7 +189,7 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>>   		  struct netlink_ext_ack *extack)
>>   {
>>   	struct netdevsim *ns = psd->drv_priv;
>> -	int i;
>> +	unsigned int phase;
>>   
>>   	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
>>   		NL_SET_ERR_MSG(extack, "SPI space exhausted");
>> @@ -163,9 +197,11 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
>>   	}
>>   
>>   	assoc->spi = cpu_to_be32(++ns->psp.spi);
>> -	assoc->key[0] = psd->generation;
>> -	for (i = 1; i < PSP_MAX_KEY; i++)
>> -		assoc->key[i] = ns->psp.spi + i;
>> +	phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
>> +
>> +	/* dev_keys_lock not needed because of psd->lock */
> Can you elaborate a bit?
>
> Is dev_keys_lock only used to synchronize the writers, then? Which after
> device init would only be concurrent invocations of nsim_key_rotate. But
> that operation correctly also holds the device lock using
> psp_device_get_locked.


This is an error in splitting changes in the series on my part. The 
spinlock is used to synchronize the writer with the reader running the 
kdf in the napi_poll() path in the later aes-gcm commit. I should have 
added the spinlock stuff when that reader was introduced. The comment is 
just pointing out that all of the psp_dev_ops on a psd are serialized 
with the psd->lock. I'll fix that in the respin.

Re: [PATCH net-next 3/6] netdevsim: psp: move rx processing into nsim_poll()

[Daniel Zahka]

On 5/11/26 4:03 PM, Willem de Bruijn wrote:
> Daniel Zahka wrote:
>> nsim_do_psp() does PSP decap and skb extension creation in the tx
>> path. This has the slightly undesirable property of not allowing the
>> psp rx code to run on PSP packets cooked up in userspace and
>> transmitted on a packet socket from the peer dev (e.g. packetdrill).
> Whether this happens in the nsim_start_xmit tx side handler directly
> or is deferred to nsim_napi_rx is irrelevant, isn't it?


You're right. The way netdevsim works, it is entirely immaterial. I'll 
correct the erroneous commit message, but I still think having the decap 
code in the napi_poll side makes a little bit more logical sense here.


>> +/* Returns true if skb was consumed, false otherwise. */
>> +bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
>> +{
>> +	struct psp_dev *psd;
>> +	struct psphdr *psph;
>> +	struct udphdr *uh;
>> +	int payload_len;
>> +	u32 versions;
>> +	int psp_off;
>> +	bool is_udp;
>> +	int l3_hlen;
>> +	u8 version;
>> +	u32 psd_id;
>> +	int err;
>> +
>> +	if (skb->protocol == htons(ETH_P_IP)) {
>> +		struct iphdr *iph;
>> +
>> +		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
>> +			return false;
>> +
>> +		iph = (struct iphdr *)skb->data;
>> +		if (iph->ihl < 5)
>> +			return false;
>> +
>> +		is_udp = iph->protocol == IPPROTO_UDP;
>> +		l3_hlen = iph->ihl * 4;
>> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {
>> +		struct ipv6hdr *ip6h;
>> +
>> +		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
>> +			return false;
>> +		ip6h = (struct ipv6hdr *)skb->data;
>> +		is_udp = ip6h->nexthdr == IPPROTO_UDP;
>> +		l3_hlen = sizeof(struct ipv6hdr);
>> +	} else {
>> +		return false;
>> +	}
>> +
>> +	if (!is_udp)
>> +		return false;
>> +
>> +	if (!pskb_may_pull(skb, l3_hlen + sizeof(struct udphdr) + PSP_HDR_SIZE))
>> +		return false;
>> +
>> +	uh = (struct udphdr *)(skb->data + l3_hlen);
>> +	if (uh->dest != htons(PSP_DEFAULT_UDP_PORT))
>> +		return false;
>> +
>> +	psph = (struct psphdr *)(uh + 1);
>> +	version = FIELD_GET(PSPHDR_VERFL_VERSION, psph->verfl);
> This seems to reimplement a lot of psp_dev_rcv. Is that needed?
>
> Is it a hint that this psp driver API needs some work?


It could be. I'd have to split the parsing from the decap logic in 
psp_dev_rcv(). I just wonder if another user of the two separate halves 
other than netdevsim will come along.


>> +	rcu_read_lock();
>> +	psd = rcu_dereference(ns->psp.dev);
>> +	if (psd) {
>> +		versions = READ_ONCE(psd->config.versions);
>> +		psd_id = psd->id;
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	if (!psd || !(versions & (1 << version))) {
>> +		skb->ip_summed = CHECKSUM_NONE;
>> +		return false;
>> +	}
>> +
>> +	psp_off = l3_hlen + sizeof(struct udphdr);
>> +	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
>> +	if (payload_len < 0)
>> +		goto drop;
>> +
>> +	skb_push(skb, ETH_HLEN);
>> +	skb->mac_len = ETH_HLEN;
>> +	err = psp_dev_rcv(skb, psd_id, 0, false);
>> +	if (err)
>> +		goto drop;
>> +
>> +	skb_reset_mac_header(skb);
>> +	skb_pull(skb, ETH_HLEN);
> Similarly this is a bit of a hack, pushing and pulling a fake Ethernet
> offset.
>
> And is this skb_reset_mac_header needed?


skb_reset_mac_header() is needed because psp_dev_rcv() shifts the l2 and l3 headers forward, and the mac header has already been set. psp_dev_rcv() expects the mac header to be there. I hear what you're saying though. The driver api could probably handle these for us, but I also didn't want to overfit to netdevsim without another user. I'll explore some options before I post this again.

Re: [PATCH net-next 4/6] netdevsim: psp: implement kdf from psp spec

[Willem de Bruijn]

Daniel Zahka wrote:
> 
> On 5/11/26 3:49 PM, Willem de Bruijn wrote:
> > Daniel Zahka wrote:
> >> Implement the PSP key derivation function (KDF) per the PSP
> >> Architecture Spec.
> >>
> >> The kdf is used to generate spi + session key pairs, and will also be
> > Text is a bit ambiguous here: the kdf does not generate the spi. It
> > derives a session key from the master key and spi.
> >
> >> used in the rx path to re-derive the tx key used by the peer.
> >>
> >> Also, remove support for psd->generation, as it is not needed for
> >> netdevsim after removing the fake authentication hack.
> > Is psd->generation only used inside driver code, not by the core PSP
> > stack? Else it should be set to !!(ns->psp.spi & PSP_SPI_KEY_PHASE) on
> > key rotation. If only used by the driver, no need to reset it on each
> > rotation.
> 
> 
> Core tries to 'suggest' a generation to the driver, which is the last 
> generation + 1, before calling into key_rotate(),


Oh right, I recall the feature now. It is more than just the MSB bit flip.

> but this won't work for netdevsim.

Why not, if it otherwise dead code to the driver?

That seems most in line with intent. I'd say keep it and allow this driver
to ignore it, if it has purpose to the stack. Or remove it otherwise. Does
not have to be in this series. But I don't see a need to explicitly touch
the field.

I think the intent was previousy to avoid on double rotate to deliver
data from gen N+2 to sockets associated with an SPI from gen N. But I
don't know the details and whether that is obsolete (probably).

> I could set the generation to !!(ns->psp.spi & 
> PSP_SPI_KEY_PHASE) so that it aliases the device key selection bit, but 
> I think this is basically the same as just setting it to 0. This series 
> makes the generation field dead code in core. The old netdevsim 
> implementation relied on it to do its fake authentication hack, but that 
> is removed with this series. The only real hw implementation we have, 
> mlx5, does not support device key generations. Maybe we need to just 
> remove that until a real driver comes along as a user.
> 
> 
> >> Assisted-by: Claude:claude-opus-4.6
> >> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> >>   enum skb_drop_reason
> >>   nsim_psp_handle_tx(struct sk_buff *skb, struct netdevsim *ns)
> >>   {
> >> @@ -155,7 +189,7 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
> >>   		  struct netlink_ext_ack *extack)
> >>   {
> >>   	struct netdevsim *ns = psd->drv_priv;
> >> -	int i;
> >> +	unsigned int phase;
> >>   
> >>   	if ((ns->psp.spi ^ (ns->psp.spi + 1)) & PSP_SPI_KEY_PHASE) {
> >>   		NL_SET_ERR_MSG(extack, "SPI space exhausted");
> >> @@ -163,9 +197,11 @@ nsim_rx_spi_alloc(struct psp_dev *psd, u32 version,
> >>   	}
> >>   
> >>   	assoc->spi = cpu_to_be32(++ns->psp.spi);
> >> -	assoc->key[0] = psd->generation;
> >> -	for (i = 1; i < PSP_MAX_KEY; i++)
> >> -		assoc->key[i] = ns->psp.spi + i;
> >> +	phase = !!(ns->psp.spi & PSP_SPI_KEY_PHASE);
> >> +
> >> +	/* dev_keys_lock not needed because of psd->lock */
> > Can you elaborate a bit?
> >
> > Is dev_keys_lock only used to synchronize the writers, then? Which after
> > device init would only be concurrent invocations of nsim_key_rotate. But
> > that operation correctly also holds the device lock using
> > psp_device_get_locked.
> 
> 
> This is an error in splitting changes in the series on my part. The 
> spinlock is used to synchronize the writer with the reader running the 
> kdf in the napi_poll() path in the later aes-gcm commit. I should have 
> added the spinlock stuff when that reader was introduced. The comment is 
> just pointing out that all of the psp_dev_ops on a psd are serialized 
> with the psd->lock. I'll fix that in the respin.
> 

Re: [PATCH net-next 3/6] netdevsim: psp: move rx processing into nsim_poll()

[Willem de Bruijn]

Daniel Zahka wrote:
> 
> On 5/11/26 4:03 PM, Willem de Bruijn wrote:
> > Daniel Zahka wrote:
> >> nsim_do_psp() does PSP decap and skb extension creation in the tx
> >> path. This has the slightly undesirable property of not allowing the
> >> psp rx code to run on PSP packets cooked up in userspace and
> >> transmitted on a packet socket from the peer dev (e.g. packetdrill).
> > Whether this happens in the nsim_start_xmit tx side handler directly
> > or is deferred to nsim_napi_rx is irrelevant, isn't it?
> 
> 
> You're right. The way netdevsim works, it is entirely immaterial. I'll 
> correct the erroneous commit message, but I still think having the decap 
> code in the napi_poll side makes a little bit more logical sense here.

Agreed
 
> 
> >> +/* Returns true if skb was consumed, false otherwise. */
> >> +bool nsim_psp_handle_rx(struct netdevsim *ns, struct sk_buff *skb)
> >> +{
> >> +	struct psp_dev *psd;
> >> +	struct psphdr *psph;
> >> +	struct udphdr *uh;
> >> +	int payload_len;
> >> +	u32 versions;
> >> +	int psp_off;
> >> +	bool is_udp;
> >> +	int l3_hlen;
> >> +	u8 version;
> >> +	u32 psd_id;
> >> +	int err;
> >> +
> >> +	if (skb->protocol == htons(ETH_P_IP)) {
> >> +		struct iphdr *iph;
> >> +
> >> +		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> >> +			return false;
> >> +
> >> +		iph = (struct iphdr *)skb->data;
> >> +		if (iph->ihl < 5)
> >> +			return false;
> >> +
> >> +		is_udp = iph->protocol == IPPROTO_UDP;
> >> +		l3_hlen = iph->ihl * 4;
> >> +	} else if (skb->protocol == htons(ETH_P_IPV6)) {
> >> +		struct ipv6hdr *ip6h;
> >> +
> >> +		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> >> +			return false;
> >> +		ip6h = (struct ipv6hdr *)skb->data;
> >> +		is_udp = ip6h->nexthdr == IPPROTO_UDP;
> >> +		l3_hlen = sizeof(struct ipv6hdr);
> >> +	} else {
> >> +		return false;
> >> +	}
> >> +
> >> +	if (!is_udp)
> >> +		return false;
> >> +
> >> +	if (!pskb_may_pull(skb, l3_hlen + sizeof(struct udphdr) + PSP_HDR_SIZE))
> >> +		return false;
> >> +
> >> +	uh = (struct udphdr *)(skb->data + l3_hlen);
> >> +	if (uh->dest != htons(PSP_DEFAULT_UDP_PORT))
> >> +		return false;
> >> +
> >> +	psph = (struct psphdr *)(uh + 1);
> >> +	version = FIELD_GET(PSPHDR_VERFL_VERSION, psph->verfl);
> > This seems to reimplement a lot of psp_dev_rcv. Is that needed?
> >
> > Is it a hint that this psp driver API needs some work?
> 
> 
> It could be. I'd have to split the parsing from the decap logic in 
> psp_dev_rcv(). I just wonder if another user of the two separate halves 
> other than netdevsim will come along.

Fair. Why does this driver need it. Only for that version check? If so,
can that move after the generic decap.

> 
> >> +	rcu_read_lock();
> >> +	psd = rcu_dereference(ns->psp.dev);
> >> +	if (psd) {
> >> +		versions = READ_ONCE(psd->config.versions);
> >> +		psd_id = psd->id;
> >> +	}
> >> +	rcu_read_unlock();
> >> +
> >> +	if (!psd || !(versions & (1 << version))) {
> >> +		skb->ip_summed = CHECKSUM_NONE;
> >> +		return false;
> >> +	}
> >> +
> >> +	psp_off = l3_hlen + sizeof(struct udphdr);
> >> +	payload_len = skb->len - psp_off - PSP_HDR_SIZE - PSP_TRL_SIZE;
> >> +	if (payload_len < 0)
> >> +		goto drop;
> >> +
> >> +	skb_push(skb, ETH_HLEN);
> >> +	skb->mac_len = ETH_HLEN;
> >> +	err = psp_dev_rcv(skb, psd_id, 0, false);
> >> +	if (err)
> >> +		goto drop;
> >> +
> >> +	skb_reset_mac_header(skb);
> >> +	skb_pull(skb, ETH_HLEN);
> > Similarly this is a bit of a hack, pushing and pulling a fake Ethernet
> > offset.
> >
> > And is this skb_reset_mac_header needed?
> 
> 
> skb_reset_mac_header() is needed because psp_dev_rcv() shifts the l2 and l3 headers forward, and the mac header has already been set. psp_dev_rcv() expects the mac header to be there. I hear what you're saying though. The driver api could probably handle these for us, but I also didn't want to overfit to netdevsim without another user. I'll explore some options before I post this again.

Ack, thanks.