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

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-23, 18:45:43 -0700, Jakub Kicinski wrote:
> On Mon, 23 Mar 2026 18:43:04 -0700 Jakub Kicinski wrote:
> > On Fri, 20 Mar 2026 11:03:51 +0100 Antonio Quartulli wrote:
> > > While deleting an existing ovpn interface, there is a very
> > > narrow window where adding a new peer via netlink may cause
> > > the netdevice to hang and prevent its unregistration.
> > > 
> > > It may happen during ovpn_dellink(), when all existing peers are
> > > freed and the device is queued for deregistration, but a
> > > CMD_PEER_NEW message comes in adding a new peer that takes again
> > > a reference to the netdev.
> > > 
> > > At this point there is no way to release the device because we are
> > > under the assumption that all peers were already released.
> > > 
> > > Fix the race condition by releasing all peers in ndo_uninit(),
> > > when the netdevice has already been removed from the netdev
> > > list and thus an incoming CMD_PEER_NEW cannot have any effect
> > > anymore.
> > > 
> > > At this point ovpn_dellink() becomes empty and can just be
> > > removed.  
> > 
> > This looks like a step in the right direction but AI points out that
> > it's not enough:
> 
> On second thought I wonder if the fix will not be to move the flush
> even later. So please fix the AI-reported issue in the same submission.

But if we move it later (priv_destructor? that's the only driver CB
left), netdev_wait_allrefs_any won't be happy.

An idea, on top of this patch:

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

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

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.


@Antonio: btw, I've always been a bit unsure about the "if (schedule)
hold(peer)" in ovpn_peer_keepalive_work_single, all those races make
me worry again that we could schedule while the refcount drops to 0.

-- 
Sabrina