On Tue, Jan 05, 2021 at 09:17:43PM +0000, Tom Parkin wrote:
> err_unset:
> write_lock_bh(&pch->upl);
> - RCU_INIT_POINTER(pch->bridge, NULL);
> + /* Re-check pch->bridge with upl held since a racing unbridge might
> already
> + * have cleared it and dropped the reference on pch->bridge.
> + */
> + if (rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl)))
> {
> + RCU_INIT_POINTER(pch->bridge, NULL);
> + drop_ref = true;
> + }
> write_unlock_bh(&pch->upl);
> synchronize_rcu();
> +
> + if (drop_ref)
> + if (refcount_dec_and_test(&pchb->file.refcnt))
> + ppp_destroy_channel(pchb);
> +
I think this works because ppp_mutex prevents pch->bridge from being
reassigned to another channel. However, this isn't obvious when reading
the code, and well, I prefer to not introduce new dependencies on
ppp_mutex (otherwise we'd better go with your original patch).
I think we could just save pch->bridge while we're under ->upl
protection, and then drop the reference of that channel (if non-NULL):
err_unset:
write_lock_bh(&pch->upl);
+ /* Re-read pch->bridge in case it was modified concurrently */
+ pchb = rcu_dereference_protected(pch->bridge,
+ lockdep_is_held(&pch->upl));
+ RCU_INIT_POINTER(pch->bridge, NULL);
write_unlock_bh(&pch->upl);
synchronize_rcu();
+
+ if (pchb)
+ if (refcount_dec_and_test(&pchb->file.refcnt))
+ ppp_destroy_channel(pchb);
+
return -EALREADY;
}
This way we know that pchb is the channel we were pointing to when we
cleared pch->bridge. And this is also a bit simpler than using an extra
boolean.
