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@xxxxxxxxx>

> +/* 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
>