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

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-24, 14:30:06 -0700, Jakub Kicinski wrote:
> On Tue, 24 Mar 2026 11:09:11 +0100 Sabrina Dubroca wrote:
> > -------- 8< --------
> > diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> > index c7f382437630..ebec8c2ff427 100644
> > --- a/drivers/net/ovpn/netlink.c
> > +++ b/drivers/net/ovpn/netlink.c
> > @@ -90,8 +90,11 @@ void ovpn_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
> >  	netdevice_tracker *tracker = (netdevice_tracker *)&info->user_ptr[1];
> >  	struct ovpn_priv *ovpn = info->user_ptr[0];
> >  
> > -	if (ovpn)
> > +	if (ovpn) {
> > +		if (READ_ONCE(dev->reg_state) >= NETREG_UNREGISTERING)
> > +			ovpn_peers_free(ovpn, NULL, OVPN_DEL_PEER_REASON_TEARDOWN);
> >  		netdev_put(ovpn->dev, tracker);
> > +	}
> >  }
> >  
> >  static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
> > -------- 8< --------
> > 
> > This would clean up a peer that may have been added while we were
> > starting device unregistration. We hold a reference on the device so
> > no UAF possible, netdev_wait_allrefs_any will wait for this. If we
> > don't have a racing peer creation, ndo_uninit takes care of the peers.
> 
> 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.

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

> > And s/cancel_delayed_work_sync/disable_delayed_work_sync/ for the
> > keepalive_work.
> > 
> > 
> > LLM claims it's because of parallel_ops, I don't think this is
> > related? It also claims this issue is only for UDP sockets (and TCP
> > would see a UAF on the keepalive), but ovpn_peer_new always holds the
> > ovpn netdev, so I don't think there's a difference there.
> 
> Yup, I think the LLMs are trying to be helpful and are looking for some
> write lock earlier in the path. As much as they are annoying I can't
> blame them here, I feel like we try to make things lockless too often.

Well, if we want to protect against netdev removal, we have to use the
one lock one we're trying hard to not depend on so much (rtnl)?
At the ovpn level operations are not lockless.

The LLM found a legit race that I missed, but I'm always puzzled by
inconsistencies like that.

-- 
Sabrina