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

[Daniel Zahka <daniel.zahka@gmail.com>]

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.