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.