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:

quoted

On Tue, Dec 01, 2020 at 11:52:49AM +0000, Tom Parkin wrote:

quoted

+	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 :).

quoted

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

quoted

+	}
+	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!