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

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

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?

> +	__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?

> +		goto out;
> +	}
[ ... ]
-- 
pw-bot: changes-requested