[PATCH net-next] ppp: tear down bridge before clearing pch->chan

[PATCH net-next] ppp: tear down bridge before clearing pch->chan

[Qingfang Deng]

As we previously did to ppp_disconnect_channel(), also move
ppp_unbridge_channels() before pch->chan is set to NULL in
ppp_unregister_channel().

ppp_unbridge_channels() calls synchronize_rcu(), so no concurrent RCU
readers in ppp_channel_bridge_input() can observe the channel after its
chan pointer is cleared.

This makes the !pchb->chan check in ppp_channel_bridge_input()
redundant and can be safely removed.

Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
 drivers/net/ppp/ppp_generic.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index b097d1b38ac9..3a609d48a424 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2285,17 +2285,11 @@ static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
 		goto out_rcu;
 
 	spin_lock_bh(&pchb->downl);
-	if (!pchb->chan) {
-		/* channel got unregistered */
-		kfree_skb(skb);
-		goto outl;
-	}
 
 	skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
 	if (!pchb->chan->ops->start_xmit(pchb->chan, skb))
 		kfree_skb(skb);
 
-outl:
 	spin_unlock_bh(&pchb->downl);
 out_rcu:
 	rcu_read_unlock();
@@ -2997,6 +2991,8 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	 * the channel's start_xmit or ioctl routine before we proceed.
 	 */
 	ppp_disconnect_channel(pch);
+	ppp_unbridge_channels(pch);
+
 	down_write(&pch->chan_sem);
 	spin_lock_bh(&pch->downl);
 	pch->chan = NULL;
@@ -3008,8 +3004,6 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	list_del(&pch->list);
 	spin_unlock_bh(&pn->all_channels_lock);
 
-	ppp_unbridge_channels(pch);
-
 	pch->file.dead = 1;
 	wake_up_interruptible(&pch->file.rwait);
 
-- 
2.43.0

Re: [PATCH net-next] ppp: tear down bridge before clearing pch->chan

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ppp: tear down bridge before clearing pch->chan

This commit moves ppp_unbridge_channels() before pch->chan is cleared
and removes the NULL check for pchb->chan in ppp_channel_bridge_input(),
relying on synchronize_rcu() inside ppp_unbridge_channels() for safety.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index b097d1b38ac9c..3a609d48a4248 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -2285,17 +2285,11 @@ static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
>  		goto out_rcu;
>  
>  	spin_lock_bh(&pchb->downl);
> -	if (!pchb->chan) {
> -		/* channel got unregistered */
> -		kfree_skb(skb);
> -		goto outl;
> -	}
>  
>  	skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
>  	if (!pchb->chan->ops->start_xmit(pchb->chan, skb))

Could removing this check lead to a NULL pointer dereference if both channels
in a bridge (A and B) are unregistered concurrently?

If Thread 1 unregisters A and calls ppp_unbridge_channels(A), it clears
both A->bridge and B->bridge to NULL, then blocks on synchronize_rcu().

If Thread 2 unregisters B and calls ppp_unbridge_channels(B), it finds
B->bridge is already NULL and returns -EINVAL early, bypassing its own
synchronize_rcu() call.

Thread 2 would then proceed to set B->chan = NULL while an RCU reader
(Thread 3) might still be active here, leading to a crash when it tries
to access pchb->chan->ops->start_xmit.

[ ... ]

> @@ -2997,6 +2991,8 @@ ppp_unregister_channel(struct ppp_channel *chan)
>  	 * the channel's start_xmit or ioctl routine before we proceed.
>  	 */
>  	ppp_disconnect_channel(pch);
> +	ppp_unbridge_channels(pch);
> +

Does moving ppp_unbridge_channels() here introduce a re-bridging race window?

At this point, the channel is still in the pn->all_channels list. After
ppp_unbridge_channels(pch) completes, pch->bridge is NULL.

Could a concurrent PPPIOCBRIDGECHAN ioctl from another channel find this
channel via ppp_find_channel() and successfully establish a new bridge
with it, since ppp_bridge_channels() only verifies that pch->bridge is NULL?

If so, ppp_unregister_channel() would then unconditionally set pch->chan = NULL
and remove it from the list without tearing down the newly formed bridge.
This regression could crash the peer channel when it processes a packet.

>  	down_write(&pch->chan_sem);
>  	spin_lock_bh(&pch->downl);
>  	pch->chan = NULL;
-- 
pw-bot: cr