Re: [PATCH net 1/1] ovpn: fix race between deleting interface and adding new peer

[Sabrina Dubroca <sd@queasysnail.net>]

2026-04-23, 15:43:31 +0200, Antonio Quartulli wrote:
> 
> 
> On 23/04/2026 14:16, Antonio Quartulli wrote:
> > On 23/04/2026 04:20, Jakub Kicinski wrote:
> > > On Wed, 22 Apr 2026 14:32:42 +0200 Antonio Quartulli wrote:
> > > > +    /* Prevent adding new peers while destroying the ovpn interface.
> > > > +     * Failing to do so would end up holding the device reference
> > > > +     * endlessly hostage of the new peer object with no chance of
> > > > +     * release..
> > > > +     */
> > > > +    if (ovpn->dev->reg_state >= NETREG_UNREGISTERING)
> > > > +        return -ENODEV;
> > > 
> > > AI review suggests wrapping reg_state read in READ_ONCE(), I think
> > > that's legit. Also nit: I think > or != REGISTERED would be more
> > > idiomatic than comparing >= UNREGSITERING ?
> > 
> > Agreed on READ_ONCE. Will fix it.
> > 
> > As for your second point, I am fine with "!= REGISTERED" as that's the
> > only state we should be accepting new peers in any case.
> > 
> > > 
> > > If you agree make sure Sashiko doesn't complain about anything else
> > > once it's public:
> > > https://sashiko.dev/#/patchset/20260422123242.530882-2-
> > > antonio@openvpn.net
> > 
> > Is there any way to get earlier access to these reviews? (at least for
> > patches somehow related to me/ovpn)
> > 
> 
> Dang! sashiko reminded me that I should swap cancel_delayed_work_sync with
> disable_delayed_work_sync. Will fix that.
> 
> 
> As for the second remarks..It has convincing arguments, but I need some more
> time to think about it.
> 
> Sabrina, do you have an opinion?

Sadly, it seems possible.

I wanted to add rcu_read_lock around everything ovpn_peer_add() does,
so that unregister_netdevice_many_notify() gets stuck on the
synchronize_net() call after setting UNREGISTERING, but
ovpn_peer_add_p2p needs to be able to sleep [1].  If we move the
ovpn->lock from ovpn_peer_add_* to ovpn_peer_add, so that we check
reg_state and insert the new peer without being preempted, we should
be ok. But maybe submit that to netdev as RFC (not directly a pull
request) so that sashiko tells me how wrong I am?

We could also wrap all of ovpn_peer_add (including the new reg_state
check) in netdev_lock, since we can't move to NETREG_UNREGISTERING
until netdev_lock is released (then I think we wouldn't need the
READ_ONCE(reg_state)). But I don't know if that's an acceptable use of
netdev_lock.


[1] maybe we should add a might_sleep() annotation directly in
unlock_ovpn (instead of/in addition to the one already in
ovpn_socket_release), I'm already getting a bit rusty on the locking :/

-- 
Sabrina