diff mbox series

[Openvpn-devel,ovpn,net] ovpn: fix race between deleting interface and adding new peer

Message ID 20260317141916.437559-1-a@unstable.cc
State New
Headers show
Series [Openvpn-devel,ovpn,net] ovpn: fix race between deleting interface and adding new peer | expand

Commit Message

Antonio Quartulli March 17, 2026, 2:19 p.m. UTC 
From: Antonio Quartulli <antonio@openvpn.net>

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.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Closes: https://lore.kernel.org/netdev/aaVgJ16edTfQkYbx@v4bel/
Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
Fixes: 80747caef33d ("ovpn: introduce the ovpn_peer object")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/main.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Sabrina Dubroca March 19, 2026, 2:35 p.m. UTC | #1 
2026-03-17, 15:19:16 +0100, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@openvpn.net>
> 
> 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.
> 
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Closes: https://lore.kernel.org/netdev/aaVgJ16edTfQkYbx@v4bel/
> Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
> Fixes: 80747caef33d ("ovpn: introduce the ovpn_peer object")
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> ---
>  drivers/net/ovpn/main.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks. This issue probably got introduced when I made you move some
stuff from the UNREGISTER notifier into deletion, during the initial
review.
Antonio Quartulli March 19, 2026, 2:42 p.m. UTC | #2 
On 19/03/2026 15:35, Sabrina Dubroca wrote:
> 2026-03-17, 15:19:16 +0100, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@openvpn.net>
>>
>> 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.
>>
>> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
>> Closes: https://lore.kernel.org/netdev/aaVgJ16edTfQkYbx@v4bel/
>> Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
>> Fixes: 80747caef33d ("ovpn: introduce the ovpn_peer object")
>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>> ---
>>   drivers/net/ovpn/main.c | 12 ++----------
>>   1 file changed, 2 insertions(+), 10 deletions(-)
> 
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> Thanks. This issue probably got introduced when I made you move some
> stuff from the UNREGISTER notifier into deletion, during the initial
> review.
> 

I also think it was around those times.
This area has been reshuffled quite a bit :)


Will send to net ASAP.

Thanks!

Regards,
diff mbox series

Patch

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index 2e0420febda0..0eab305780c7 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -92,6 +92,8 @@  static void ovpn_net_uninit(struct net_device *dev)
 {
 	struct ovpn_priv *ovpn = netdev_priv(dev);
 
+	cancel_delayed_work_sync(&ovpn->keepalive_work);
+	ovpn_peers_free(ovpn, NULL, OVPN_DEL_PEER_REASON_TEARDOWN);
 	gro_cells_destroy(&ovpn->gro_cells);
 }
 
@@ -208,15 +210,6 @@  static int ovpn_newlink(struct net_device *dev,
 	return register_netdevice(dev);
 }
 
-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);
-	ovpn_peers_free(ovpn, NULL, OVPN_DEL_PEER_REASON_TEARDOWN);
-	unregister_netdevice_queue(dev, head);
-}
-
 static int ovpn_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct ovpn_priv *ovpn = netdev_priv(dev);
@@ -235,7 +228,6 @@  static struct rtnl_link_ops ovpn_link_ops = {
 	.policy = ovpn_policy,
 	.maxtype = IFLA_OVPN_MAX,
 	.newlink = ovpn_newlink,
-	.dellink = ovpn_dellink,
 	.fill_info = ovpn_fill_info,
 };