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