Skip to contentSkip to conversation

ovpn: Fix race condition in ovpn_dellink()

PATCH In review Hyunwoo Kim 3 participants 3 messages Mar 2 – Mar 6

Subsystems (3)
Networking Network drivers Openvpn data channel offload
lore
Share & tools
lore

Hyunwoo Kim

author
#0 lore raw

PATCH 1/1 ovpn: Fix race condition in ovpn_dellink()

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

Fixes 3ecfd9349f40 ("ovpn: implement keepalive mechanism")
Signed-off-by Hyunwoo Kim <[email protected]>
---
 drivers/net/ovpn/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ovpn/main.c +1 -1
index 2e0420febda0..ada73dd35ba4 100644
@@ -212,7 +212,7 @@ static void ovpn_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct ovpn_priv *ovpn = netdev_priv(dev);
 
-	cancel_delayed_work_sync(&ovpn->keepalive_work);
+	disable_delayed_work_sync(&ovpn->keepalive_work);
 	ovpn_peers_free(ovpn, NULL, OVPN_DEL_PEER_REASON_TEARDOWN);
 	unregister_netdevice_queue(dev, head);
 }
Signature · 1 line
2.43.0
maintainer
#1 lore raw

Hi,

Quoted 13 lines · Hyunwoo Kim

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)

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

Regards,

Signature · 2 lines
Antonio Quartulli
OpenVPN Inc.
reviewer
#2 lore raw

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

Quoted 31 lines · Antonio Quartulli

Hi,

Quoted 13 lines · Hyunwoo Kim

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)

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

Quoted 7 lines

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:

drivers/net/ovpn/main.c +4 -0
index 2e0420febda0..3bf6ea5f7208 100644
@@ -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).

Signature · 1 line
Sabrina