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 <tpar...@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;
> > }
> >
