Re: [PATCH net] ovpn: Fix race condition in ovpn_dellink()

[Sabrina Dubroca <sd@queasysnail.net>]

Sorry Antonio, things are a bit busy over here, I'm finally getting to
this.

2026-03-03, 21:15:59 +0100, Antonio Quartulli wrote:
> Hi,
> 
> On 02/03/2026 11:02, Hyunwoo Kim wrote:
> > When ovpn_dellink() is called, it invokes
> > cancel_delayed_work_sync() to stop keepalive_work before freeing
> > the device.
> > However, ovpn_nl_peer_new_doit() runs without any lock shared
> > with the RTNL path, so keepalive_work can be scheduled after
> > cancel_delayed_work_sync() returns.
> > 
> > The following is a simple race scenario:
> > 
> >            cpu0                            cpu1
> > 
> > ovpn_dellink(dev)
> >    cancel_delayed_work_sync(keepalive_work)
> >                                     ovpn_nl_peer_new_doit()
> >                                       ovpn_nl_peer_modify()
> >                                         ovpn_peer_keepalive_set()
> >                                           mod_delayed_work(keepalive_work)
> > 
> > To prevent this race condition, cancel_delayed_work_sync() is
> > replaced with disable_delayed_work_sync().
> 
> I was about to agree on your fix, however, it seems there is a larger issue
> here and this patch is just addressing one symptom.
> 
> If you are truly able to execute the whole ovpn_nl_peer_new_doit() after
> cancel_delayed_work_sync() and before the netdev refcounter reaches 0, I
> think we may get stuck in the "wait for device to be freed..." loop.
> 
> That's because the ovpn_peer_new() will acquire a reference to the netdev,
> preventing it to be fully released.
> 
> 
> Sabrina, do you have any thought on this?
> It seems as if there is no safe guard against adding peers while destroying
> the interface.

Feel free to add:

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index 2e0420febda0..3bf6ea5f7208 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -214,6 +214,10 @@ static void ovpn_dellink(struct net_device *dev, struct list_head *head)
 
 	cancel_delayed_work_sync(&ovpn->keepalive_work);
 	ovpn_peers_free(ovpn, NULL, OVPN_DEL_PEER_REASON_TEARDOWN);
+
+	pr_warn("sleeping\n");
+	msleep(10000);
+
 	unregister_netdevice_queue(dev, head);
 }
 

to help reproduce the race, I usually do something like that if it's
possible to sleep in that context.


I think you're right, the newly-added peer will miss
ovpn_peers_free. Can we simply move cancel_delayed_work_sync +
ovpn_peers_free to ndo_uninit and not provide a dellink?

At ndo_uninit, the netdevice is no longer listed so PEER_NEW won't
find it, but delaying the release of the reference on the netdev via
peer cleanup should still be ok (unregister_netdevice_many_notify
calls unlist_netdevice and then ndo_uninit, then we add it to the
todo_list and netdev_run_todo processes that list and does
netdev_wait_allrefs_any).

-- 
Sabrina