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.