Re: [PATCH net-next v4 2/2] pppoe: queue unbound packets directly

[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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103111711.28202-1-qingfang.deng@linux.dev?part=2
---
pppoe: queue unbound packets directly

This patch makes pppoe_rcv() queue unbound packets directly with
sock_queue_rcv_skb() instead of going through __sk_receive_skb(). It also
removes the now unused pppoe_rcv_core() backlog callback. Ordering with
PPPIOCGCHAN now depends on the synchronize_net() added in commit
92c1bf630abf ("pppox: drain queued packets on channel handoff").

> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 5d5187e4976b3..51445fddb4d31 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c

[ ... ]

> @@ -417,11 +389,12 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>  
>  	skb->dev = NULL;
>  
> -	if (likely(po->sk.sk_state & PPPOX_BOUND)) {
> +	if (likely(po->sk.sk_state & PPPOX_BOUND))
>  		ppp_input(&po->chan, skb);
> -		return NET_RX_SUCCESS;
> -	}
> -	return __sk_receive_skb(&po->sk, skb, 0, 1, false);
> +	else if (sock_queue_rcv_skb(&po->sk, skb))
> +		goto drop;

[Severity: Low]
This isn't a bug, but with this change PPPoE no longer calls
__sk_receive_skb() anywhere. A comment in ppp_bridge_gso_segment() in
drivers/net/ppp/ppp_generic.c still says it does:

drivers/net/ppp/ppp_generic.c:ppp_bridge_gso_segment() {
    ...
	/* skb->dev is NULL here (cleared by __sk_receive_skb() for PPPoE),
	 * and some GSO handlers (tunnels, IPv6 UFO) dereference it.
	 */
    ...
}

The NULL skb->dev precondition still holds. The explicit skb->dev = NULL
in pppoe_rcv() above provides it, and __sock_queue_rcv_skb() also clears
it on the queued path.

Should that comment be updated to say pppoe_rcv() clears skb->dev? As
written, it points at a function PPPoE no longer uses. Someone relying on
it could drop the assignment in pppoe_rcv() and bring back the NULL-dev
problem the comment warns about.

> +
> +	return NET_RX_SUCCESS;
>  
>  drop:
>  	kfree_skb(skb);

[ ... ]