[PATCH net-next] pppoe: pass bound packets directly to generic PPP

[PATCH net-next] 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().

That lifetime guarantee does not cover reuse of the ppp_channel embedded
in struct pppox_sock. An RX handler can find the old session before it
is unhashed, then resume after disconnect and reconnect have cleared and
re-registered po->chan. It could then race initialization of the new
channel or pass an old-session packet through it.

After unhashing an old session, call synchronize_net() before clearing
and reusing po->chan. This drains every receive path that could have
found the old binding while retaining concurrent delivery for the active
session.

Assisted-by: Codex:GPT-5.6
Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
 drivers/net/ppp/pppoe.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 6874a1a8edaf..062411624182 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -345,10 +345,10 @@ static struct notifier_block pppoe_notifier = {
 
 /************************************************************************
  *
- * Do the real work of receiving a PPPoE Session frame.
+ * Backlog receive a PPPoE Session frame and deliver to userspace.
  *
  ***********************************************************************/
-static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
+static int pppoe_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 {
 	struct pppox_sock *po = pppox_sk(sk);
 
@@ -373,7 +373,7 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
 
 /************************************************************************
  *
- * Receive wrapper called in BH context.
+ * Receive a PPPoE Session frame.
  *
  ***********************************************************************/
 static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
@@ -420,6 +420,10 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!po)
 		goto drop;
 
+	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:
@@ -524,7 +528,7 @@ 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_backlog_rcv	= pppoe_backlog_rcv;
 	sk->sk_destruct		= pppoe_destruct;
 	sk->sk_state		= PPPOX_NONE;
 	sk->sk_type		= SOCK_STREAM;
@@ -625,6 +629,13 @@ static int pppoe_connect(struct socket *sock, struct sockaddr_unsized *uservaddr
 		pn = pppoe_pernet(sock_net(sk));
 		delete_item(pn, po->pppoe_pa.sid,
 			    po->pppoe_pa.remote, po->pppoe_ifindex);
+
+		/* pppoe_rcv() can call ppp_input() without taking the socket
+		 * lock. Once the socket is unhashed, wait for any receive path
+		 * that found it earlier before clearing and reusing po->chan.
+		 */
+		synchronize_net();
+
 		if (po->pppoe_dev) {
 			dev_put(po->pppoe_dev);
 			po->pppoe_dev = NULL;
-- 
2.43.0

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

[Jakub Kicinski]

This is (mostly) an AI-generated review of your patch.

One non-AI question - presumably you're doing this to improve performance?
Could you add some numbers to the cover letter?
---
pppoe: pass bound packets directly to generic PPP

This drops __sk_receive_skb() for PPPOX_BOUND PPPoE sockets and hands
session frames straight to ppp_input() from pppoe_rcv(), relying on the
RCU-deferred channel free added by ec4215683e47.  It also adds a
synchronize_net() in pppoe_connect() after the old session is unhashed,
before po->chan is cleared and reused.

No Link: tags are present in the commit header.

A few questions below.

> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 6874a1a8edaf1..0624116241823 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -345,10 +345,10 @@ static struct notifier_block pppoe_notifier = {
>  
>  /************************************************************************
>   *
> - * Do the real work of receiving a PPPoE Session frame.
> + * Backlog receive a PPPoE Session frame and deliver to userspace.
>   *
>   ***********************************************************************/
> -static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
> +static int pppoe_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  {
>  	struct pppox_sock *po = pppox_sk(sk);

[Severity: Low]
Is the new banner comment accurate?  The function body still delivers to
the generic PPP layer, not to userspace:

drivers/net/ppp/pppoe.c:pppoe_backlog_rcv() {
	if (sk->sk_state & PPPOX_BOUND) {
		ppp_input(&po->chan, skb);
	} else {
		if (sock_queue_rcv_skb(sk, skb))
			goto abort_kfree;
	}
}

That branch still looks reachable: pppoe_rcv() only takes the new direct
path when it observes PPPOX_BOUND, and a frame observed while !BOUND is put
on the socket backlog by __sk_receive_skb() and only evaluated at
release_sock() time.  By then pppox_ioctl(PPPIOCGCHAN) may already have run
sk->sk_state |= PPPOX_BOUND under lock_sock().

Could the comment mention that the bound case still goes to ppp_input(), so
that a later reader does not remove the branch on the strength of the banner
text?

[ ... ]

> @@ -420,6 +420,10 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!po)
>  		goto drop;
>  
> +	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]
Does this skip socket filters and the skb->dev clearing that
__sk_receive_skb() used to perform for bound sessions?

net/core/sock.c:__sk_receive_skb() {
	reason = sk_filter_trim_cap(sk, skb, trim_cap);
	if (reason)
		goto discard_and_relse;

	skb->dev = NULL;
	...
}

Two things in sk_filter_trim_cap() look relevant here.  A filter attached
with SO_ATTACH_FILTER/SO_ATTACH_BPF is now silently ignored for all bound
session traffic; do_sock_setsockopt() routes SOL_SOCKET options to
sock_setsockopt() for PF_PPPOX since pppoe does not set SOCK_CUSTOM_SOCKOPT,
so sk->sk_filter can be non-NULL.  The pfmemalloc guard is also bypassed:

net/core/filter.c:sk_filter_trim_cap() {
	if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)) {
		NET_INC_STATS(sock_net(sk), LINUX_MIB_PFMEMALLOCDROP);
		return SKB_DROP_REASON_PFMEMALLOC;
	}
	...
}

The dropped skb->dev = NULL also matters for the frames ppp_input() parks on
pch->file.rq (proto >= 0xc000, PPP_CCPFRAG, or pch->ppp == NULL) -- those can
now sit queued indefinitely holding a non-refcounted net_device pointer.  Is
that intentional?  The cgroup-BPF and LSM parts of sk_filter_trim_cap() do
appear to be no-ops for PF_PPPOX, so those are not a concern, and the
sk_rcvqueues_full() check is not meaningful for the bound path.

Note the PPPOX_BOUND branch in pppoe_backlog_rcv() still runs under the old
contract, so the same class of frame is handled two different ways depending
on timing.  Could the changelog say something about the filtering and
skb->dev behaviour change?

[Severity: Low]
Should this read be annotated?  po->sk.sk_state is loaded here from softirq
with no lock held, while every writer stores it with a plain access under
lock_sock():

drivers/net/ppp/pppox.c:pppox_unbind_sock()	sk->sk_state = PPPOX_DEAD;
drivers/net/ppp/pppox.c:pppox_ioctl()		sk->sk_state |= PPPOX_BOUND;
drivers/net/ppp/pppoe.c:pppoe_connect()		sk->sk_state = PPPOX_CONNECTED;

Before the patch the test happened inside bh_lock_sock() in
__sk_receive_skb(), so it was serialized against those writers.  Would
KCSAN report this now, and would READ_ONCE()/WRITE_ONCE() be appropriate?

There is also a bare control dependency: pppoe_connect() fills in
po->chan.{hdrlen,mtu,private,ops}, calls ppp_register_net_channel() (which
sets chan->ppp), and only then publishes sk->sk_state, with no release
store to pair with.  The reader then loads po->chan.ppp inside ppp_input()
with no acquire.  As far as I can tell the worst outcome is ppp_input()
seeing !pch and dropping the frame, so this looks like an annotation
question rather than a correctness one.  pppol2tp_recv() has the same
unannotated pattern.

[Severity: Low]
Can frames reach ppp_input() out of arrival order now?  Two paths can feed
the same channel:

pppoe_rcv() -> ppp_input()			(softirq, no socket lock)
release_sock() -> pppoe_backlog_rcv() -> ppp_input()	(process context)

pppox_ioctl(PPPIOCGCHAN) sets sk->sk_state |= PPPOX_BOUND under lock_sock().
A frame that arrived just before that, while the socket was owned by the
ioctl caller, went to the socket backlog.  A frame arriving after the BOUND
store but before release_sock() takes the new fast path and is handed to
ppp_input() immediately, ahead of the older backlogged one.

ppp_input() serializes internally, and the window only exists once per
session while LCP/auth frames are in flight, so this may be acceptable --
but is it worth a note?

[Severity: Low]
Does this leave the documented channel contract stale?

include/linux/ppp_channel.h {
 * SMP locking notes:
 * The channel code must ensure that when it calls ppp_unregister_channel,
 * nothing is executing in any of the procedures above, for that
 * channel.
}

ppp_input() is one of the listed procedures.  After this change pppoe_rcv()
calls it from softirq with no serialization, while every pppoe caller that
reaches ppp_unregister_channel() via pppox_unbind_sock() -- pppoe_release(),
pppoe_flush_dev(), pppoe_unbind_sock_work(), pppoe_connect() -- does so
without draining in-flight ppp_input() callers.  In pppoe_connect() the new
synchronize_net() sits after pppox_unbind_sock(), so it fences the later
memset(&po->chan) and re-registration rather than the unregister itself.

The RCU-deferred struct channel free from ec4215683e47 does appear to close
the use-after-free, and pppol2tp_recv() already calls ppp_input() the same
lockless way, so this is about the text rather than a runtime failure.
Should include/linux/ppp_channel.h (and possibly
Documentation/networking/ppp_generic.rst) be updated to describe the
RCU-based rule that now makes this safe?

[ ... ]

> @@ -625,6 +629,13 @@ static int pppoe_connect(struct socket *sock, struct sockaddr_unsized *uservaddr
>  		pn = pppoe_pernet(sock_net(sk));
>  		delete_item(pn, po->pppoe_pa.sid,
>  			    po->pppoe_pa.remote, po->pppoe_ifindex);
> +
> +		/* pppoe_rcv() can call ppp_input() without taking the socket
> +		 * lock. Once the socket is unhashed, wait for any receive path
> +		 * that found it earlier before clearing and reusing po->chan.
> +		 */
> +		synchronize_net();
> +
>  		if (po->pppoe_dev) {
>  			dev_put(po->pppoe_dev);
>  			po->pppoe_dev = NULL;

[Severity: Low]
Is holding the socket lock across the grace period necessary here?
pppoe_connect() takes lock_sock(sk) at the top, so every disconnect/rebind
of a session now blocks other users of that socket for a full,
non-expedited RCU grace period.  The drain is only needed to protect the
following memset(&po->chan, 0, sizeof(po->chan)), so could it be arranged
without the socket lock held?

For the record, this does not look like it can stall RTNL:
delete_item() unhashes the socket before synchronize_net(), and
pppoe_flush_dev() only locks sockets it finds by walking pn->hash_table, so
the RTNL-holding notifier cannot find the socket sleeping in the grace
period.
-- 
pw-bot: cr

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

[Qingfang Deng]

Hi,

On 2026/8/6 9:28, Jakub Kicinski wrote:
> This is (mostly) an AI-generated review of your patch.
>
> One non-AI question - presumably you're doing this to improve performance?
> Could you add some numbers to the cover letter?

Yes. I may add those numbers to v2. Note that the generic PPP RX also 
serializes internally (which I plan to remove in another patch), so this 
patch alone won't significantly improve performance.

> ---
> pppoe: pass bound packets directly to generic PPP
>
> This drops __sk_receive_skb() for PPPOX_BOUND PPPoE sockets and hands
> session frames straight to ppp_input() from pppoe_rcv(), relying on the
> RCU-deferred channel free added by ec4215683e47.  It also adds a
> synchronize_net() in pppoe_connect() after the old session is unhashed,
> before po->chan is cleared and reused.
>
> No Link: tags are present in the commit header.
>
> A few questions below.
>
>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
>> index 6874a1a8edaf1..0624116241823 100644
>> --- a/drivers/net/ppp/pppoe.c
>> +++ b/drivers/net/ppp/pppoe.c
>> @@ -345,10 +345,10 @@ static struct notifier_block pppoe_notifier = {
>>   
>>   /************************************************************************
>>    *
>> - * Do the real work of receiving a PPPoE Session frame.
>> + * Backlog receive a PPPoE Session frame and deliver to userspace.
>>    *
>>    ***********************************************************************/
>> -static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
>> +static int pppoe_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>>   {
>>   	struct pppox_sock *po = pppox_sk(sk);
> [Severity: Low]
> Is the new banner comment accurate?  The function body still delivers to
> the generic PPP layer, not to userspace:
>
> drivers/net/ppp/pppoe.c:pppoe_backlog_rcv() {
> 	if (sk->sk_state & PPPOX_BOUND) {
> 		ppp_input(&po->chan, skb);
> 	} else {
> 		if (sock_queue_rcv_skb(sk, skb))
> 			goto abort_kfree;
> 	}
> }
>
> That branch still looks reachable: pppoe_rcv() only takes the new direct
> path when it observes PPPOX_BOUND, and a frame observed while !BOUND is put
> on the socket backlog by __sk_receive_skb() and only evaluated at
> release_sock() time.  By then pppox_ioctl(PPPIOCGCHAN) may already have run
> sk->sk_state |= PPPOX_BOUND under lock_sock().
>
> Could the comment mention that the bound case still goes to ppp_input(), so
> that a later reader does not remove the branch on the strength of the banner
> text?


I'll update the banner comment.


>
> [ ... ]
>
>> @@ -420,6 +420,10 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>>   	if (!po)
>>   		goto drop;
>>   
>> +	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]
> Does this skip socket filters and the skb->dev clearing that
> __sk_receive_skb() used to perform for bound sessions?
>
> net/core/sock.c:__sk_receive_skb() {
> 	reason = sk_filter_trim_cap(sk, skb, trim_cap);
> 	if (reason)
> 		goto discard_and_relse;
>
> 	skb->dev = NULL;
> 	...
> }
>
> Two things in sk_filter_trim_cap() look relevant here.  A filter attached
> with SO_ATTACH_FILTER/SO_ATTACH_BPF is now silently ignored for all bound
> session traffic; do_sock_setsockopt() routes SOL_SOCKET options to
> sock_setsockopt() for PF_PPPOX since pppoe does not set SOCK_CUSTOM_SOCKOPT,
> so sk->sk_filter can be non-NULL.  The pfmemalloc guard is also bypassed:
>
> net/core/filter.c:sk_filter_trim_cap() {
> 	if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)) {
> 		NET_INC_STATS(sock_net(sk), LINUX_MIB_PFMEMALLOCDROP);
> 		return SKB_DROP_REASON_PFMEMALLOC;
> 	}
> 	...
> }
>
> The dropped skb->dev = NULL also matters for the frames ppp_input() parks on
> pch->file.rq (proto >= 0xc000, PPP_CCPFRAG, or pch->ppp == NULL) -- those can
> now sit queued indefinitely holding a non-refcounted net_device pointer.  Is
> that intentional?  The cgroup-BPF and LSM parts of sk_filter_trim_cap() do
> appear to be no-ops for PF_PPPOX, so those are not a concern, and the
> sk_rcvqueues_full() check is not meaningful for the bound path.
>
> Note the PPPOX_BOUND branch in pppoe_backlog_rcv() still runs under the old
> contract, so the same class of frame is handled two different ways depending
> on timing.  Could the changelog say something about the filtering and
> skb->dev behaviour change?

The sk filter is an unintended side effect when sk_receive_skb() was 
introduced (commit 25995ff57767). pppd has never used that to filter 
PPPoE packets, instead it uses the generic PPP filter.

The dropped skb->dev = NULL should not matter, as the generic PPP layer 
does not expect it to be initialized by an underlying channel.

>
> [Severity: Low]
> Should this read be annotated?  po->sk.sk_state is loaded here from softirq
> with no lock held, while every writer stores it with a plain access under
> lock_sock():
>
> drivers/net/ppp/pppox.c:pppox_unbind_sock()	sk->sk_state = PPPOX_DEAD;
> drivers/net/ppp/pppox.c:pppox_ioctl()		sk->sk_state |= PPPOX_BOUND;
> drivers/net/ppp/pppoe.c:pppoe_connect()		sk->sk_state = PPPOX_CONNECTED;
>
> Before the patch the test happened inside bh_lock_sock() in
> __sk_receive_skb(), so it was serialized against those writers.  Would
> KCSAN report this now, and would READ_ONCE()/WRITE_ONCE() be appropriate?
>
> There is also a bare control dependency: pppoe_connect() fills in
> po->chan.{hdrlen,mtu,private,ops}, calls ppp_register_net_channel() (which
> sets chan->ppp), and only then publishes sk->sk_state, with no release
> store to pair with.  The reader then loads po->chan.ppp inside ppp_input()
> with no acquire.  As far as I can tell the worst outcome is ppp_input()
> seeing !pch and dropping the frame, so this looks like an annotation
> question rather than a correctness one.  pppol2tp_recv() has the same
> unannotated pattern.
sk->sk_state is already `volatile` so I don't think it needs an extra 
READ/WRITE_ONCE() which does the same thing.
> [Severity: Low]
> Can frames reach ppp_input() out of arrival order now?  Two paths can feed
> the same channel:
>
> pppoe_rcv() -> ppp_input()			(softirq, no socket lock)
> release_sock() -> pppoe_backlog_rcv() -> ppp_input()	(process context)
>
> pppox_ioctl(PPPIOCGCHAN) sets sk->sk_state |= PPPOX_BOUND under lock_sock().
> A frame that arrived just before that, while the socket was owned by the
> ioctl caller, went to the socket backlog.  A frame arriving after the BOUND
> store but before release_sock() takes the new fast path and is handed to
> ppp_input() immediately, ahead of the older backlogged one.
>
> ppp_input() serializes internally, and the window only exists once per
> session while LCP/auth frames are in flight, so this may be acceptable --
> but is it worth a note?


I may add a comment into pppoe_backlog_rcv().


>
> [Severity: Low]
> Does this leave the documented channel contract stale?
>
> include/linux/ppp_channel.h {
>   * SMP locking notes:
>   * The channel code must ensure that when it calls ppp_unregister_channel,
>   * nothing is executing in any of the procedures above, for that
>   * channel.
> }
>
> ppp_input() is one of the listed procedures.  After this change pppoe_rcv()
> calls it from softirq with no serialization, while every pppoe caller that
> reaches ppp_unregister_channel() via pppox_unbind_sock() -- pppoe_release(),
> pppoe_flush_dev(), pppoe_unbind_sock_work(), pppoe_connect() -- does so
> without draining in-flight ppp_input() callers.  In pppoe_connect() the new
> synchronize_net() sits after pppox_unbind_sock(), so it fences the later
> memset(&po->chan) and re-registration rather than the unregister itself.
>
> The RCU-deferred struct channel free from ec4215683e47 does appear to close
> the use-after-free, and pppol2tp_recv() already calls ppp_input() the same
> lockless way, so this is about the text rather than a runtime failure.
> Should include/linux/ppp_channel.h (and possibly
> Documentation/networking/ppp_generic.rst) be updated to describe the
> RCU-based rule that now makes this safe?
>
> [ ... ]


True, but it should be done in a separate patch.


>
>> @@ -625,6 +629,13 @@ static int pppoe_connect(struct socket *sock, struct sockaddr_unsized *uservaddr
>>   		pn = pppoe_pernet(sock_net(sk));
>>   		delete_item(pn, po->pppoe_pa.sid,
>>   			    po->pppoe_pa.remote, po->pppoe_ifindex);
>> +
>> +		/* pppoe_rcv() can call ppp_input() without taking the socket
>> +		 * lock. Once the socket is unhashed, wait for any receive path
>> +		 * that found it earlier before clearing and reusing po->chan.
>> +		 */
>> +		synchronize_net();
>> +
>>   		if (po->pppoe_dev) {
>>   			dev_put(po->pppoe_dev);
>>   			po->pppoe_dev = NULL;
> [Severity: Low]
> Is holding the socket lock across the grace period necessary here?
> pppoe_connect() takes lock_sock(sk) at the top, so every disconnect/rebind
> of a session now blocks other users of that socket for a full,
> non-expedited RCU grace period.  The drain is only needed to protect the
> following memset(&po->chan, 0, sizeof(po->chan)), so could it be arranged
> without the socket lock held?
>
> For the record, this does not look like it can stall RTNL:
> delete_item() unhashes the socket before synchronize_net(), and
> pppoe_flush_dev() only locks sockets it finds by walking pn->hash_table, so
> the RTNL-holding notifier cannot find the socket sleeping in the grace
> period.

pppd is single-threaded, so in practice, "other users of that socket" do 
not exist.

If there are multiple users of one socket, the current implementation 
needs the lock to keep the entire teardown/rebind transaction serialized.

A plain unlock creates this race:

1. Caller A unbinds and unhashes the old session, then unlocks.
2. Caller B connects a new session on the same socket. PPPOX_DEAD 
permits this.
3. B clears and registers po->chan for its session.
4. A reacquires the lock and resumes inside its old teardown branch, 
overwriting B's socket.

Best regards,

Qingfang