Re: [PATCH net] ppp: hold mutex when unbridging channels in unregister path

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]

On  Thu, Dec 24, 2020 at 11:28:18 +0100, Guillaume Nault wrote:
> On Wed, Dec 23, 2020 at 06:47:30PM +0000, Tom Parkin wrote:
> > Channels are bridged using the PPPIOCBRIDGECHAN ioctl, which executes
> > with the ppp_mutex held.
> > 
> > Unbridging may occur in two code paths: firstly an explicit
> > PPPIOCUNBRIDGECHAN ioctl, and secondly on channel unregister.  The
> > latter may occur when closing the /dev/ppp instance or on teardown of
> > the channel itself.
> > 
> > This opens up a refcount underflow bug if ppp_bridge_channels called via.
> > ioctl races with ppp_unbridge_channels called via. file release.
> > 
> > The race is triggered by ppp_unbridge_channels taking the error path
> 
> This is actually ppp_bridge_channels.
> 

Will fix, thanks.

> > through the 'err_unset' label.  In this scenario, pch->bridge has been
> > set, but no reference will be taken on pch->file because the function
> > errors out.  Therefore, if ppp_unbridge_channels is called in the window
> > between pch->bridge being set and pch->bridge being unset, it will
> > erroneously drop the reference on pch->file and cause a refcount
> > underflow.
> > 
> > To avoid this, hold the ppp_mutex while calling ppp_unbridge_channels in
> > the shutdown path.  This serialises the unbridge operation with any
> > concurrently executing ioctl.
> > 
> > Signed-off-by: Tom Parkin <tparkin@katalix.com>
> > ---
> >  drivers/net/ppp/ppp_generic.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 09c27f7773f9..e87a05fee2db 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -2938,7 +2938,9 @@ ppp_unregister_channel(struct ppp_channel *chan)
> >  	list_del(&pch->list);
> >  	spin_unlock_bh(&pn->all_channels_lock);
> >  
> > +	mutex_lock(&ppp_mutex);
> >  	ppp_unbridge_channels(pch);
> > +	mutex_unlock(&ppp_mutex);
> >  
> >  	pch->file.dead = 1;
> >  	wake_up_interruptible(&pch->file.rwait);
> > -- 
> > 2.17.1
> > 
> 
> The problem is that assigning ->bridge and taking a reference on that
> channel isn't atomic. Holding ppp_mutex looks like a workaround for
> this problem.

You're quite right -- that is the underlying issue.

> I think the refcount should be taken before unlocking ->upl in
> ppp_bridge_channels().

Aye, that's the other option :-)

I wasn't sure whether it was better to use the same locking structure
as the ioctl call, or to rework the code to make the two things
effectively atomic as you suggest.

I'll try this approach.

Thanks for your review!

> 
> Something like this (completely untested):
> 
> ---- 8< ----
>  static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
>  {
>  	write_lock_bh(&pch->upl);
>  	if (pch->ppp ||
>  	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
>  		write_unlock_bh(&pch->upl);
>  		return -EALREADY;
>  	}
> +
> +	refcount_inc(&pchb->file.refcnt);
>  	rcu_assign_pointer(pch->bridge, pchb);
>  	write_unlock_bh(&pch->upl);
> 
> 	write_lock_bh(&pchb->upl);
> 	if (pchb->ppp ||
> 	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
> 		write_unlock_bh(&pchb->upl);
> 		goto err_unset;
> 	}
> +
> +	refcount_inc(&pch->file.refcnt);
> 	rcu_assign_pointer(pchb->bridge, pch);
> 	write_unlock_bh(&pchb->upl);
> 
> -	refcount_inc(&pch->file.refcnt);
> -	refcount_inc(&pchb->file.refcnt);
> -
> 	return 0;
> 
> err_unset:
> 	write_lock_bh(&pch->upl);
> 	RCU_INIT_POINTER(pch->bridge, NULL);
> 	write_unlock_bh(&pch->upl);
> 	synchronize_rcu();
> +
> +	if (refcount_dec_and_test(&pchb->file.refcnt))
> +		ppp_destroy_channel(pchb);
> +
> 	return -EALREADY;
> }
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]