Re: [PATCH v2 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

[Guillaume Nault <gnault@redhat.com>]

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!