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

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-25, 14:37:36 +0100, Antonio Quartulli wrote:
> Hi!
> 
> On 24/03/2026 23:40, Sabrina Dubroca wrote:
> > 2026-03-24, 14:30:06 -0700, Jakub Kicinski wrote:
> > > LGTM. This or change all the write paths to check if the device is still
> > > alive after taking the lock.
> > 
> > I think peer_new is the only relevant path here (other paths use an
> > existing peer and modify/delete the peer itself or its keys while
> > holding a reference), and we don't take a lock except to insert the
> > peer in the ovpn instance (ie netdev)'s hashtable (or peer slot, for
> > single-peer instances). I guess we could add the reg_state >=
> > NETREG_UNREGISTERING check to ovpn_peer_add_{p2p,mp} and reject adding
> > the peer. It seems cleaner than my ovpn_nl_post_doit() diff above.
> 
> Yeah, I like the check in ovpn_peer_add_* too.
> 
> > 
> > > > Or we can call ovpn_peers_free on every NETDEV_UNREGISTER notification
> > > > that netdev_wait_allrefs_any sends us (but then we don't need it in
> > > > ndo_uninit).
> > > 
> > > Hm, wouldn't we need a notification _after_ netdev_wait_allrefs_any() ?
> > 
> > netdev_wait_allrefs_any() will never complete if we don't clean up all
> > the peers, since they hold a ref on the netdev.
> > 
> > But calling ovpn_peers_free on netdev_wait_allrefs_any()'s
> > 
> >      /* Rebroadcast unregister notification */
> > 
> > should clean up peers that got added while we were unregistering.
> 
> But with the check in ovpn_peer_add_*, we don't need this extra call to
> ovpn_peers_free(), right?

Yes. This was an alternative idea for the same thing.

-- 
Sabrina