[PATCH net-next 1/2] ppp: disconnect channel before nullifying pch->chan

[PATCH net-next 1/2] ppp: disconnect channel before nullifying pch->chan

[Qingfang Deng]

In ppp_unregister_channel(), pch->chan is set to NULL before calling
ppp_disconnect_channel(), which removes the channel from ppp->channels
list using list_del_rcu() + synchronize_net(). This creates an
intermediate state where the channel is still connected (on the list)
but already unregistered (pch->chan == NULL).

Call ppp_disconnect_channel() before setting pch->chan to NULL. After
the synchronize_net(), no new reader on the transmit path will hold a
reference to the channel from the list.

This eliminates the problematic state, and prepares for removing the
pch->chan NULL checks from the transmit path in a subsequent patch.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
 drivers/net/ppp/ppp_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 6344c5eb0f98..ad480b584e25 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -3032,12 +3032,12 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	 * This ensures that we have returned from any calls into
 	 * the channel's start_xmit or ioctl routine before we proceed.
 	 */
+	ppp_disconnect_channel(pch);
 	down_write(&pch->chan_sem);
 	spin_lock_bh(&pch->downl);
 	WRITE_ONCE(pch->chan, NULL);
 	spin_unlock_bh(&pch->downl);
 	up_write(&pch->chan_sem);
-	ppp_disconnect_channel(pch);
 
 	pn = ppp_pernet(pch->chan_net);
 	spin_lock_bh(&pn->all_channels_lock);
-- 
2.43.0

[PATCH net-next 2/2] ppp: remove pch->chan NULL checks from tx path

[Qingfang Deng]

Now that ppp_disconnect_channel() is called before pch->chan is set to
NULL, a channel from ppp->channels list on the transmit path is
guaranteed to have non-NULL pch->chan.

Remove the pch->chan NULL checks from ppp_push(), ppp_mp_explode(), and
ppp_fill_forward_path(), where a channel is obtained from the list.
Remove the corresponding WRITE/READ_ONCE annotations as they no longer
race.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
 drivers/net/ppp/ppp_generic.c | 68 +++++++++++------------------------
 1 file changed, 21 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index ad480b584e25..a036ddfe327b 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1602,10 +1602,7 @@ static int ppp_fill_forward_path(struct net_device_path_ctx *ctx,
 	if (!pch)
 		return -ENODEV;
 
-	chan = READ_ONCE(pch->chan);
-	if (!chan)
-		return -ENODEV;
-
+	chan = pch->chan;
 	if (!chan->ops->fill_forward_path)
 		return -EOPNOTSUPP;
 
@@ -1928,9 +1925,9 @@ ppp_push(struct ppp *ppp, struct sk_buff *skb)
 
 		spin_lock(&pch->downl);
 		chan = pch->chan;
-		if (unlikely(!chan || (!chan->direct_xmit && skb_linearize(skb)))) {
-			/* channel got unregistered, or it requires a linear
-			 * skb but linearization failed
+		if (unlikely(!chan->direct_xmit && skb_linearize(skb))) {
+			/* channel requires a linear skb but linearization
+			 * failed
 			 */
 			kfree_skb(skb);
 			ret = 1;
@@ -1991,28 +1988,23 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb)
 	hdrlen = (ppp->flags & SC_MP_XSHORTSEQ)? MPHDRLEN_SSN: MPHDRLEN;
 	i = 0;
 	list_for_each_entry(pch, &ppp->channels, clist) {
-		if (pch->chan) {
-			pch->avail = 1;
-			navail++;
-			pch->speed = pch->chan->speed;
-		} else {
-			pch->avail = 0;
-		}
-		if (pch->avail) {
-			if (skb_queue_empty(&pch->file.xq) ||
-				!pch->had_frag) {
-					if (pch->speed == 0)
-						nzero++;
-					else
-						totspeed += pch->speed;
-
-					pch->avail = 2;
-					++nfree;
-					++totfree;
-				}
-			if (!pch->had_frag && i < ppp->nxchan)
-				ppp->nxchan = i;
+		pch->avail = 1;
+		navail++;
+		pch->speed = pch->chan->speed;
+
+		if (skb_queue_empty(&pch->file.xq) || !pch->had_frag) {
+			if (pch->speed == 0)
+				nzero++;
+			else
+				totspeed += pch->speed;
+
+			pch->avail = 2;
+			++nfree;
+			++totfree;
 		}
+		if (!pch->had_frag && i < ppp->nxchan)
+			ppp->nxchan = i;
+
 		++i;
 	}
 	/*
@@ -2071,25 +2063,7 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb)
 			pch->avail = 1;
 		}
 
-		/* check the channel's mtu and whether it is still attached. */
 		spin_lock(&pch->downl);
-		if (pch->chan == NULL) {
-			/* can't use this channel, it's being deregistered */
-			if (pch->speed == 0)
-				nzero--;
-			else
-				totspeed -= pch->speed;
-
-			spin_unlock(&pch->downl);
-			pch->avail = 0;
-			totlen = len;
-			totfree--;
-			nfree--;
-			if (--navail == 0)
-				break;
-			continue;
-		}
-
 		/*
 		*if the channel speed is not set divide
 		*the packet evenly among the free channels;
@@ -3035,7 +3009,7 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	ppp_disconnect_channel(pch);
 	down_write(&pch->chan_sem);
 	spin_lock_bh(&pch->downl);
-	WRITE_ONCE(pch->chan, NULL);
+	pch->chan = NULL;
 	spin_unlock_bh(&pch->downl);
 	up_write(&pch->chan_sem);
 
-- 
2.43.0

Re: [PATCH net-next 2/2] ppp: remove pch->chan NULL checks from tx path

[Paolo Abeni]

On 3/12/26 10:37 AM, Qingfang Deng wrote:
> Now that ppp_disconnect_channel() is called before pch->chan is set to
> NULL, a channel from ppp->channels list on the transmit path is
> guaranteed to have non-NULL pch->chan.
> 
> Remove the pch->chan NULL checks from ppp_push(), ppp_mp_explode(), and
> ppp_fill_forward_path(), where a channel is obtained from the list.
> Remove the corresponding WRITE/READ_ONCE annotations as they no longer
> race.
> 
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>

Looks sane to me.

It looks like you are investing a significant amount of effort to
improve the ppp driver. I strongly suggest you to create some functional
self-tests before any other changes in this area.

The code is pretty intricate and obscure, a reasonable code coverage
could add significant confidence in non trivial changes, and could
possibly help revisiting/cleaning-up the locking schema (which is AFAICS
the most significant pain-point).

/P

Re: [PATCH net-next 1/2] ppp: disconnect channel before nullifying pch->chan

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 12 Mar 2026 17:37:30 +0800 you wrote:
> In ppp_unregister_channel(), pch->chan is set to NULL before calling
> ppp_disconnect_channel(), which removes the channel from ppp->channels
> list using list_del_rcu() + synchronize_net(). This creates an
> intermediate state where the channel is still connected (on the list)
> but already unregistered (pch->chan == NULL).
> 
> Call ppp_disconnect_channel() before setting pch->chan to NULL. After
> the synchronize_net(), no new reader on the transmit path will hold a
> reference to the channel from the list.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] ppp: disconnect channel before nullifying pch->chan
    https://git.kernel.org/netdev/net-next/c/6a196e83a1a7
  - [net-next,2/2] ppp: remove pch->chan NULL checks from tx path
    https://git.kernel.org/netdev/net-next/c/febe8012458f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html