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

[Qingfang Deng]

On 2026/5/6 20:14, Simon Horman wrote:
> From: 'Simon Horman' <horms@xxxxxxxxxx>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> net: pppoe: implement GRO/GSO support
>
> This commit adds GRO and GSO support for PPPoE, which improves rx throughput
> significantly by handling packets where the pppoe header length field matches
> the exact packet length.
>
> > diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> > index bdd61c504a1c4..363204e0c49a1 100644
> > --- a/drivers/net/ppp/pppoe.c
> > +++ b/drivers/net/ppp/pppoe.c
> [ ... ]
> > @@ -1103,6 +1104,164 @@ static struct pernet_operations pppoe_net_ops = {
> >   	.size = sizeof(struct pppoe_net),
> >   };
> >   
> > +static u16
> > +compare_pppoe_header(const struct pppoe_hdr *phdr,
> > +		     const struct pppoe_hdr *phdr2)
> > +{
> > +	__be16 proto = *(const __be16 *)(phdr + 1);
> Because struct pppoe_hdr is packed, the compiler assigns it 1-byte alignment
> semantics, which phdr + 1 inherits.
>
> Does explicitly casting this to __be16 * bypass the compiler's unaligned access
> protections?
>
> If skb->data has an odd alignment from the NIC, this could trigger a fatal
> unaligned memory access exception on strict-alignment architectures. Would it
> be safer to use get_unaligned() here?
You can disregard all the alignment warnings. On those architectures, 
drivers are required to 4-byte align the network header, so the PPPoE 
header will also be aligned.
> > +	__be16 proto2 = *(const __be16 *)(phdr2 + 1);
> > +
> > +	return (__force u16)((phdr->sid ^ phdr2->sid) | (proto ^ proto2));
> > +}
> > +
> > +static __be16 pppoe_hdr_proto(const struct pppoe_hdr *phdr)
> > +{
> > +	__be16 proto = *(const __be16 *)(phdr + 1);
> Could this also result in an unaligned access regression for the same reason?
>
> [ ... ]
> > +static struct sk_buff *pppoe_gro_receive(struct list_head *head,
> > +					 struct sk_buff *skb)
> > +{
> [ ... ]
> > +	off_pppoe = skb_gro_offset(skb);
> > +	hlen = off_pppoe + PPPOE_SES_HLEN;
> > +	phdr = skb_gro_header(skb, hlen, off_pppoe);
> > +	if (unlikely(!phdr))
> > +		goto out;
> > +
> > +	/* filter for session packets (type:1, ver:1, code:0) */
> > +	if (*(const __be16 *)phdr != cpu_to_be16(0x1100))
> Is it possible this cast could similarly trigger an unaligned access exception
> if the header offset is oddly aligned?
>
> [ ... ]
> > +static struct sk_buff *pppoe_gso_segment(struct sk_buff *skb,
> > +					 netdev_features_t features)
> > +{
> [ ... ]
> > +	segs = ptype->callbacks.gso_segment(skb, features);
> > +	if (IS_ERR_OR_NULL(segs)) {
> > +		skb_gso_error_unwind(skb, orig_type, PPPOE_SES_HLEN, mac_offset,
> > +				     mac_len);
> If the inner protocol's gso_segment callback returns NULL (indicating the
> hardware can natively offload the segmentation), skb_gso_error_unwind() will
> be called.
>
> skb_gso_error_unwind() unconditionally sets skb->encapsulation = 1. However,
> PPPoE does not configure the inner header offsets (such as
> skb->inner_network_header).
>
> When the driver sees skb->encapsulation == 1, could it attempt to read the
> uninitialized inner header offsets, parsing the outer MAC header as an inner
> IP header and misprogramming the hardware TSO context?
>
> Does PPPoE need to explicitly clear TSO features before calling the inner
> segmentation to force software segmentation, avoiding the need for
> skb_gso_error_unwind() here entirely?

I'm not sure how to test this. Is this what happens when the underlying 
interface supports TSO?

+Cc: Pablo