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

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

[Qingfang Deng]

Bound PPPoE sockets pass received frames to the generic PPP layer. They
currently do so through __sk_receive_skb(), which takes the socket BH
lock and serializes calls to ppp_input().

The lock originally prevented ppp_input() from racing with
ppp_unregister_channel(). Commit ec4215683e47 ("ppp: defer channel free
to an RCU grace period to fix pppol2tp RX UAF") now keeps the generic
PPP channel alive until in-flight RCU readers have completed, so bound
packets can be passed directly to ppp_input() from pppoe_rcv().

The direct bound receive path also skips the socket filter check in
__sk_receive_skb(). This does not affect normal pppd PPPoE operation:
pppd uses the kernel PPP channel and does not attach a receive filter to
a socket. Instead, it uses the generic PPP pass and active filters,
installed through PPPIOCSPASS and PPPIOCSACTIVE, unaffected by this
change. Set skb->dev to NULL to match __sk_receive_skb()'s other
behavior.

Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
A local receive-path benchmark sends 1492-byte IPv4 packets through
multiqueue TAP -> PPPoE -> PPP in a 16-vCPU x86-64 QEMU KVM guest.
The guest runs on 16 E-cores. Senders are pinned to distinct guest CPUs.
CONFIG_PARAVIRT_SPINLOCKS and CONFIG_X86_NATIVE_CPU are enabled.
This synchronous TAP receive path does not use GRO.
The generic PPP receive lock (ppp->rlock) is retained in both
variants. Median throughput over 12 samples per cell across four
boots, expressed as IP-layer Gbps:

  Senders       Before       After     Change
        1        19.08       19.63     +2.89%
        2        31.52       33.77     +7.15%
        4        53.67       63.48    +18.28%
        8        39.79       51.50    +29.42%
       16        49.40       61.33    +24.15%

Repeating the test with a separate benchmark-only change that removes
the generic PPP receive lock:

  Senders       Before       After     Change
        1        19.45       19.89     +2.23%
        2        33.29       34.92     +4.90%
        4        61.72       67.01     +8.57%
        8        44.46      123.76   +178.35%
       16        54.62      220.48   +303.69%

Eight and sixteen senders perform worse than four whenever at least
one receive lock remains. With both locks removed, throughput
rises from 67.01 Gbps with four senders to 123.76 Gbps with eight
and 220.48 Gbps with sixteen.

A separate matched GRO-off/on test uses threaded TAP NAPI and UDP
packets with a valid checksum. Senders and NAPI threads share the same
1/2/4/8/16 guest-CPU sets. UDP GRO forwarding is enabled in both modes;
only rx-gro changes. Medians use the same sample and boot counts.
With the generic PPP receive lock retained, in IP-layer Gbps:

                   GRO off                GRO on
  Senders     Before      After      Before      After
        1       13.44       13.42       14.35       14.15
        2       21.11       20.81       25.80       25.26
        4       42.18       40.89       45.31       44.76
        8       48.09       59.12       74.96       75.73
       16       46.84       54.99      142.00      142.27

With the benchmark-only generic PPP receive lock removal:

                   GRO off                GRO on
  Senders     Before      After      Before      After
        1       13.47       13.41       14.20       14.32
        2       20.63       19.86       25.58       25.21
        4       41.15       40.21       41.91       44.02
        8       50.61       70.97       76.39       74.38
       16       48.69      122.52      141.55      142.89

GRO coalesces 3.33-5.07 input packets per PPP receive skb across
measured samples. Packet and byte counters account for every datagram,
with zero TAP drops or PPP receive errors. The threaded-NAPI results
are a separate comparison from the synchronous TAP results above.

GPT Codex assisted in the benchmarks.
---
v4: repost after net-next is open, and set skb->dev = NULL before input.
    attach a benchmark with TAP's NAPI path.

v3:
https://lore.kernel.org/netdev/20261030045943.2093-1-qingfang.deng@linux.dev/
---
 drivers/net/ppp/pppoe.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 67c28ac763ef..5d5187e4976b 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;
+
+	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);
 
 drop:
-- 
2.34.1

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

[Qingfang Deng]

Queue unbound packets directly with sock_queue_rcv_skb() and remove
the unused backlog callback.

Previously, __sk_receive_skb() serialized receive processing with
PPPIOCGCHAN through the socket lock. Receivers either finished before
the ioctl acquired ownership or ran from the backlog after PPPOX_BOUND
was set.

The new lockless path relies on synchronize_net() from commit
92c1bf630abf ("pppox: drain queued packets on channel handoff").
After setting PPPOX_BOUND, PPPIOCGCHAN waits for receivers that saw
the old state to finish queueing before draining sk_receive_queue
into PPP. This prevents packets from being stranded after handoff.
SOCK_RCU_FREE protects the socket lifetime during teardown.

Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
v4: no changes

v3:
https://lore.kernel.org/netdev/20261030045943.2093-2-qingfang.deng@linux.dev/
---
 drivers/net/ppp/pppoe.c | 40 ++++++----------------------------------
 1 file changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 5d5187e4976b..51445fddb4d3 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -340,35 +340,7 @@ static struct notifier_block pppoe_notifier = {
 
 /************************************************************************
  *
- * Do the real work of receiving a PPPoE Session frame.
- *
- ***********************************************************************/
-static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
-{
-	struct pppox_sock *po = pppox_sk(sk);
-
-	/* Backlog receive. Semantics of backlog rcv preclude any code from
-	 * executing in lock_sock()/release_sock() bounds; meaning sk->sk_state
-	 * can't change.
-	 */
-
-	if (sk->sk_state & PPPOX_BOUND) {
-		ppp_input(&po->chan, skb);
-	} else {
-		if (sock_queue_rcv_skb(sk, skb))
-			goto abort_kfree;
-	}
-
-	return NET_RX_SUCCESS;
-
-abort_kfree:
-	kfree_skb(skb);
-	return NET_RX_DROP;
-}
-
-/************************************************************************
- *
- * Receive wrapper called in BH context.
+ * Receive a PPPoE Session frame.
  *
  ***********************************************************************/
 static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
@@ -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;
+
+	return NET_RX_SUCCESS;
 
 drop:
 	kfree_skb(skb);
@@ -528,7 +501,6 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
 	sock->state	= SS_UNCONNECTED;
 	sock->ops	= &pppoe_ops;
 
-	sk->sk_backlog_rcv	= pppoe_rcv_core;
 	sk->sk_destruct		= pppoe_destruct;
 	sk->sk_state		= PPPOX_NONE;
 	sk->sk_type		= SOCK_STREAM;
-- 
2.34.1

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

[Simon Horman]

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.

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

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.

[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?

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.

[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.

Would taking ppp_recv_lock() around ppp_receive_error() in ppp_input(),
as ppp_input_error() already does, fix both cases?

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

[Simon Horman]

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);

[ ... ]