Re: [RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

On 3/26/26 9:11 AM, Qingfang Deng wrote:
> +static int pppoe_gro_complete(struct sk_buff *skb, int nhoff)
> +{
> +	struct pppoe_hdr *phdr = (struct pppoe_hdr *)(skb->data + nhoff);
> +	__be16 type = pppoe_hdr_proto(phdr);
> +	struct packet_offload *ptype;
> +	unsigned int len;
> +
> +	ptype = gro_find_complete_by_type(type);
> +	if (!ptype)
> +		return -ENOENT;
> +
> +	len = skb->len - (nhoff + sizeof(*phdr));
> +	len = min(len, 0xFFFFU);

AFAICS, when the computed len is >= 64K, and the above min() will
truncate it, later pppoe_rcv() will drop the packet.

I think you should prevent such case at GRO time, flushing the chain
when it grows too big.

> +	phdr->length = cpu_to_be16(len);
> +
> +	return INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
> +				  ipv6_gro_complete, inet_gro_complete,
> +				  skb, nhoff + PPPOE_SES_HLEN);
> +}
> +
> +static struct sk_buff *pppoe_gso_segment(struct sk_buff *skb,
> +					 netdev_features_t features)
> +{
> +	unsigned int pppoe_hlen = sizeof(struct pppoe_hdr) + 2;
> +	struct sk_buff *segs = ERR_PTR(-EINVAL);
> +	u16 mac_offset = skb->mac_header;
> +	struct packet_offload *ptype;
> +	u16 mac_len = skb->mac_len;
> +	struct pppoe_hdr *phdr;
> +	__be16 orig_type, type;
> +	int len, nhoff;
> +
> +	skb_reset_network_header(skb);
> +	nhoff = skb_network_header(skb) - skb_mac_header(skb);
> +
> +	if (unlikely(!pskb_may_pull(skb, pppoe_hlen)))
> +		goto out;
> +
> +	phdr = (struct pppoe_hdr *)skb_network_header(skb);
> +	type = pppoe_hdr_proto(phdr);
> +	ptype = gro_find_complete_by_type(type);
> +	if (!ptype)
> +		goto out;
> +
> +	orig_type = skb->protocol;
> +	__skb_pull(skb, pppoe_hlen);
> +	segs = ptype->callbacks.gso_segment(skb, features);
> +	if (IS_ERR_OR_NULL(segs)) {
> +		skb_gso_error_unwind(skb, orig_type, pppoe_hlen, mac_offset,
> +				     mac_len);
> +		goto out;
> +	}
> +
> +	skb = segs;
> +	do {
> +		phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
> +		len = skb->len - (nhoff + sizeof(*phdr));
> +		phdr->length = cpu_to_be16(len);
> +		skb->network_header = (u8 *)phdr - skb->head;

I understand is quite late for the following question, but...
The network headers points to the pppoe hdr. Should it point to the
actual IP hdr?

Why not? A comment in the code or in the commit message would be
appreciated.

/P