On Thu, Dec 03, 2020 at 11:57:18AM +0000, Tom Parkin wrote:
> On Thu, Dec 03, 2020 at 01:23:18 +0100, Guillaume Nault wrote:
> > On Tue, Dec 01, 2020 at 11:52:49AM +0000, Tom Parkin wrote:
> > > + if (!pchb) {
> > > + write_unlock_bh(&pch->upl);
> > > + return -EINVAL;
> >
> > I'm not sure I'd consider this case as an error.
>
> To be honest I'd probably tend agree with you, but I was seeking to
> maintain consistency with how PPPIOCCONNECT/PPPIOCDISCONN behave. The
> latter returns EINVAL if the channel isn't connected to an interface.
Indeed, that makes sense. I didn't think about that. I was mostly
thinking about the case where ->bridge was concurently reset by another
ppp_unbridge_channels(), which doesn't look like an error to me. But
we can let userspace responsible for properly using the API (or
ignoring EINVAL when they don't).
> If you feel strongly I'm happy to change it but IMO it's better to be
> consistent with existing ioctl calls.
I don't feel strongly about it :).
> > If there's no bridged channel, there's just nothing to do.
> > Furthermore, there might be situations where this is not really an
> > error (see the possible race below).
> >
> > > + }
> > > + RCU_INIT_POINTER(pch->bridge, NULL);
> > > + write_unlock_bh(&pch->upl);
> > > +
> > > + write_lock_bh(&pchb->upl);
> > > + RCU_INIT_POINTER(pchb->bridge, NULL);
> > > + write_unlock_bh(&pchb->upl);
> > > +
> > > + synchronize_rcu();
> > > +
> > > + if (refcount_dec_and_test(&pch->file.refcnt))
> > > + ppp_destroy_channel(pch);
> >
> > I think that we could have a situation where pchb->bridge could be
> > different from pch.
> > If ppp_unbridge_channels() was also called concurrently on pchb, then
> > pchb->bridge might have been already reset. And it might have dropped
> > the reference it had on pch. In this case, we'd erroneously decrement
> > the refcnt again.
> >
> > In theory, pchb->bridge might even have been reassigned to a different
> > channel. So we'd reset pchb->bridge, but without decrementing the
> > refcnt of the channel it pointed to (and again, we'd erroneously
> > decrement pch's refcount instead).
> >
> > So I think we need to save pchb->bridge to a local variable while we
> > hold pchb->upl, and then drop the refcount of that channel, instead of
> > assuming that it's equal to pch.
>
> Ack, yes.
>
> The v1 series protected against this, although by nesting locks :-|
Well, I think the v1 could deadlock in this situation. The RFC was
immune to this problem, as it didn't modify ->bridge on pchb.
> I think in the case that pchb->bridge != pch, we probably want to
> leave pchb alone, so:
>
> 1. Don't unset the pchb->bridge pointer
> 2. Don't drop the pch reference (pchb doesn't hold a reference on pch
> because pchb->bridge != pch)
>
> This is on the assumption that pchb has been reassigned -- in that
> scenario we don't want to mess with pchb at all since it'll break the
> other bridge instance.
Yes you're right.
Thanks!
