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

On 2026/5/6 20:14, Simon Horman wrote:
> From: 'Simon Horman' <[email protected]>
>
> 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