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

Paolo Abeni
2026-04-30T09:34:25Z

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