Re: [PATCH net-next v4 1/2] pppoe: pass bound packets directly to generic PPP

[Qingfang Deng <qingfang.deng@linux.dev>]

On 11/7/2026 2:12 AM, Simon Horman wrote:
> 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=1
> ---
> pppoe: pass bound packets directly to generic PPP
> 
> This changes pppoe_rcv() to hand frames for PPPOX_BOUND sockets straight
> to ppp_input(), instead of going through __sk_receive_skb(). It relies on
> commit ec4215683e47 deferring the channel free to an RCU grace period. It
> also clears skb->dev, matching what __sk_receive_skb() used to do.
> 
>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
>> index 67c28ac763efd..5d5187e4976b3 100644
>> --- a/drivers/net/ppp/pppoe.c
>> +++ b/drivers/net/ppp/pppoe.c
>> @@ -415,6 +415,12 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>>   	if (!po)
>>   		goto drop;
>>   
>> +	skb->dev = NULL;
> 
> [Severity: Low]
> Should the comment in ppp_bridge_gso_segment() be updated as well? It
> still says __sk_receive_skb() is what clears skb->dev:
> 
> 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.
> 	 */
> 	...
> }
> 
> Bound PPPoE frames are the ones that reach the bridge path, and they no
> longer go through __sk_receive_skb(). The invariant now holds only
> because of this assignment in pppoe_rcv(). Nothing in pppoe.c says that
> the GSO path depends on it.
> 
> The follow-up patch "pppoe: queue unbound packets directly" does not
> touch ppp_generic.c, so the comment stays stale after that patch too.

Ack.

>> +
>> +	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);
> 
> [Severity: Medium]
> The commit message says:
> 
>    "The direct bound receive path also skips the socket filter check in
>    __sk_receive_skb()."
> 
> Does this path skip more than the socket filter? __sk_receive_skb()
> calls sk_filter_trim_cap(), and that function runs the LSM hook before
> the filter:
> 
> net/core/filter.c:sk_filter_trim_cap() {
> 	...
> 	err = security_sock_rcv_skb(sk, skb);
> 	if (err)
> 		return SKB_DROP_REASON_SECURITY_HOOK;
> 	...
> }
> 
> apparmor_socket_sock_rcv_skb() mediates sockets of any family when
> skb->secmark is set. BPF-LSM programs on socket_sock_rcv_skb also apply
> to AF_PPPOX. After this change, neither runs for bound session frames
> from the remote peer. A configured policy would therefore fail open.

> Some other checks are skipped as well:
> 
>    - A filter attached with SO_ATTACH_FILTER or SO_ATTACH_BPF, even one
>      locked with SO_LOCK_FILTER, is still accepted by setsockopt(), but it
>      never runs on bound frames and never trims them.
> 
>    - The sk_rcvqueues_full() check and the sk_drops / drop reason
>      accounting in __sk_receive_skb() no longer happen.
> 
> pppol2tp_recv() already calls ppp_input() directly for bound sockets, so
> there is precedent. Could the commit message still describe the LSM
> bypass and the other skipped checks?

I can mention in the commit message that it make pppoe_rcv behave like 
pppol2tp. This commit is intended to bypass the backlog queue, so no 
need to mention sk_rcvqueues_full().

> This stays true after the follow-up patch "pppoe: queue unbound packets
> directly". There, only the unbound path still reaches sk_filter, via
> sock_queue_rcv_skb().
> 
> [Severity: Low]
> Does include/linux/ppp_channel.h need an update along with this? Its SMP
> locking notes say:
> 
>   * The channel code must ensure that when it calls ppp_unregister_channel,
>   * nothing is executing in any of the procedures above, for that
>   * channel.
> 
> PPPoE followed that rule before this patch. ppp_input() ran under
> bh_lock_sock(), and every ppp_unregister_channel() caller (through
> pppox_unbind_sock()) holds lock_sock().
> 
> Now pppoe_rcv() calls ppp_input() from softirq without the socket lock.
> So ppp_input() and ppp_unregister_channel() can run at the same time on
> the same channel.
> 
> That looks safe in practice:
> 
>    - struct channel is freed through call_rcu().
>    - pch->ppp is cleared and then followed by synchronize_net().
>    - pch->bridge is cleared and then followed by synchronize_rcu().
> 
> Even so, the documented contract no longer matches the rule PPPoE and
> pppol2tp now rely on.

Ack.
> [Severity: Low]
> Is chan->ppp now accessed locklessly without any annotation? ppp_input()
> does a plain load:
> 
> drivers/net/ppp/ppp_generic.c:ppp_input() {
> 	struct channel *pch = chan->ppp;
> 	...
> }
> 
> Meanwhile, ppp_unregister_channel() can do a plain store at the same
> time, protected only by lock_sock():
> 
> pppox_unbind_sock()
>    ppp_unregister_channel()
>      chan->ppp = NULL;
> 
> For PPPoE, the socket BH lock used to serialize these two. Without it,
> this looks like a data race that KCSAN would report.
> 
> There is a second ordering gap. ppp_register_net_channel() publishes
> chan->ppp before it initializes pch:
> 
> 	chan->ppp = pch;
> 	init_ppp_file(&pch->file, CHANNEL);
> 
> This store has no release ordering, and the sk_state test in pppoe_rcv()
> gives no acquire ordering. In practice the gap between connect() and
> PPPIOCGCHAN hides this.
> 
> Would READ_ONCE()/WRITE_ONCE() on chan->ppp be appropriate here? Should
> chan->ppp also be published only after initialization, with release
> semantics?

Ack.

> The sk_state access is volatile, so that side is not a KCSAN concern.
> The chan->ppp race is still there after "pppoe: queue unbound packets
> directly".
> 
> [Severity: Low]
> Can frames reach generic PPP out of order during the PPPIOCGCHAN
> handoff? pppox_ioctl() does:
> 
> 		sk->sk_state |= PPPOX_BOUND;
> 		...
> 		synchronize_net();
> 		...
> 		while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> 			skb_orphan(skb);
> 			ppp_input(&po->chan, skb);
> 
> A frame that arrives after PPPOX_BOUND is set now takes the direct path.
> It reaches ppp_input() during synchronize_net(), ahead of older frames
> that are still in sk_receive_queue.
> 
> At this commit, frames that go to the backlog while the ioctl holds
> lock_sock() arrive even later, from release_sock() -> pppoe_rcv_core().
> 
> Before the patch, the socket lock kept these frames in wire order. The
> follow-up "pppoe: queue unbound packets directly" removes the backlog,
> but the reordering against sk_receive_queue remains. pppol2tp already
> behaves this way. The frames in this window are mostly LCP and other
> control frames for pppd.

The same concern was raised when reviewing my patch that introduced 
synchronize_net() in pppox_ioctl(), commit 92c1bf630ab ("pppox: drain 
queued packets on channel handoff"). Before that commit, unbound packets 
were stranded in sk_receive_queue forever.

In pppd, the window between connect() and ioctl(PPPIOCGCHAN) is minimal. 
If you prefer deliver order over best-effort deliver, should those 
packets be dropped instead?

> [Severity: High]
> This isn't a bug introduced by this patch, but ppp_input() appears to
> call ppp_receive_error() without holding ppp_recv_lock():
> 
> drivers/net/ppp/ppp_generic.c:ppp_input() {
> 	...
> 	rcu_read_lock_bh();
> 	ppp = rcu_dereference_bh(pch->ppp);
> 	if (!ppp_decompress_proto(skb)) {
> 		kfree_skb(skb);
> 		if (ppp) {
> 			DEV_STATS_INC(netdev_from_priv(ppp), rx_length_errors);
> 			ppp_receive_error(ppp);
> 		}
> 		goto done;
> 	}
> 	...
> }
> 
> ppp_input_error() makes the same call inside ppp_recv_lock() /
> ppp_recv_unlock(). ppp_receive_error() then calls slhc_toss(ppp->vj),
> which does a non-atomic comp->flags |= SLF_TOSS.
> 
> A peer can reach this branch with a PPPoE session frame of length 0 or
> 1. pskb_trim_rcsum() trims the frame, and pskb_may_pull(skb, 2) then
> fails in ppp_decompress_proto().
> 
> Can this race with PPPIOCSMAXCID in ppp_ioctl()?
> 
> 		ppp_lock(ppp);
> 		if (ppp->vj)
> 			slhc_free(ppp->vj);
> 		ppp->vj = vj;
> 		ppp_unlock(ppp);
> 
> Without rlock, ppp_receive_error() could load the old ppp->vj and write
> to the slcompress after it has been freed.
> 
> This patch adds one effect in the common single-channel PPPoE case. The
> socket BH lock no longer serializes ppp_input() calls for one channel.
> The lockless slhc_toss() can therefore run while another CPU is in
> slhc_uncompress() / slhc_remember() under rlock, and a comp->flags
> update can be lost.

pppd never uses any compression on PPPoE. Fix should be in a separate 
commit.
> Would taking ppp_recv_lock() around ppp_receive_error() in ppp_input(),
> as ppp_input_error() already does, fix both cases?