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

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

[Tom Parkin]

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
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

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

[Guillaume Nault]

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.

> 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.

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

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;
}

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

[Tom Parkin]

[-- 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 --]

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

[Guillaume Nault]

On Thu, Dec 24, 2020 at 02:24:32PM +0000, Tom Parkin wrote:
> 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.

ppp_mutex was added by commit 15fd0cd9a2ad ("net: autoconvert trivial
BKL users to private mutex") as a replacement for the big kernel lock.
We should head towards removing it, rather than expanding its usage
(locking dependencies are already complex enough in ppp_generic).

Also, as a refcount marks a dependency to another object, it's important
to take it before the dependency becomes visible to external entities.

> 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;
> > }
> >