[RFC net] ovpn: fix race between deleting interface and adding new peer

[RFC net] ovpn: fix race between deleting interface and adding new peer

[Antonio Quartulli]

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.

Also ovpn_peer_add() has now an extra check that forces the
function to bail out if the device reg_state is not REGISTERED.
This way any incoming CMD_PEER_NEW racing with the interface
deletion routine will simply stop before adding the peer.

Note that the above check happens while holding the netdev_lock
to prevent racing netdev state changes.

ovpn_dellink() is now empty and can 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")
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---

This patch is sent as RFC to give the AI a chance to review it once
again, since it was able to spot a new race condition in its
previous version.


 drivers/net/ovpn/main.c | 12 ++----------
 drivers/net/ovpn/peer.c | 21 ++++++++++++++++++---
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index 2e0420febda0..9993c1dfe471 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);
 
+	disable_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,
 };
 
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index c02dfab51a6e..7bf912e40ee2 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -1034,14 +1034,29 @@ static int ovpn_peer_add_p2p(struct ovpn_priv *ovpn, struct ovpn_peer *peer)
  */
 int ovpn_peer_add(struct ovpn_priv *ovpn, struct ovpn_peer *peer)
 {
+	int ret = -ENODEV;
+
+	/* Prevent adding new peers while destroying the ovpn interface.
+	 * Failing to do so would end up holding the device reference
+	 * endlessly hostage of the new peer object with no chance of
+	 * release..
+	 */
+	netdev_lock(ovpn->dev);
+	if (ovpn->dev->reg_state != NETREG_REGISTERED)
+		goto out;
+
 	switch (ovpn->mode) {
 	case OVPN_MODE_MP:
-		return ovpn_peer_add_mp(ovpn, peer);
+		ret = ovpn_peer_add_mp(ovpn, peer);
+		break;
 	case OVPN_MODE_P2P:
-		return ovpn_peer_add_p2p(ovpn, peer);
+		ret = ovpn_peer_add_p2p(ovpn, peer);
+		break;
 	}
+out:
+	netdev_unlock(ovpn->dev);
 
-	return -EOPNOTSUPP;
+	return ret;
 }
 
 /**
-- 
2.53.0

Re: [RFC net] ovpn: fix race between deleting interface and adding new peer

[Jakub Kicinski]

On Mon,  4 May 2026 16:20:33 +0200 Antonio Quartulli wrote:
> This patch is sent as RFC to give the AI a chance to review it once
> again, since it was able to spot a new race condition in its
> previous version.

FWIW I think you can just ask for Sashiko to track the openvpn mailing
list?

Re: [RFC net] ovpn: fix race between deleting interface and adding new peer

[Antonio Quartulli]

On 05/05/2026 01:54, Jakub Kicinski wrote:
> On Mon,  4 May 2026 16:20:33 +0200 Antonio Quartulli wrote:
>> This patch is sent as RFC to give the AI a chance to review it once
>> again, since it was able to spot a new race condition in its
>> previous version.
> 
> FWIW I think you can just ask for Sashiko to track the openvpn mailing
> list?

Yes, indeed.
I just went through this discussion for the batman-adv kernel module.

@Roman: is it possible for Sashiko to consider certain patches only? On 
the openvpn-devel mailing list there are also patches for the openvpn 
userspace program flying around.


Regards,


-- 
Antonio Quartulli
OpenVPN Inc.

Re: [RFC net] ovpn: fix race between deleting interface and adding new peer

[Roman Gushchin]

Antonio Quartulli <antonio@openvpn.net> writes:

> On 05/05/2026 01:54, Jakub Kicinski wrote:
>> On Mon,  4 May 2026 16:20:33 +0200 Antonio Quartulli wrote:
>>> This patch is sent as RFC to give the AI a chance to review it once
>>> again, since it was able to spot a new race condition in its
>>> previous version.
>> FWIW I think you can just ask for Sashiko to track the openvpn
>> mailing
>> list?
>
> Yes, indeed.
> I just went through this discussion for the batman-adv kernel module.
>
> @Roman: is it possible for Sashiko to consider certain patches only?
> On the openvpn-devel mailing list there are also patches for the
> openvpn userspace program flying around.

Sashiko ignores all patches which it can't apply successfully to the
kernel tree, so it's likely not a problem.

If it will be a problem, I'll master something.

Thanks

>
>
> Regards,

Re: [RFC net] ovpn: fix race between deleting interface and adding new peer

[Antonio Quartulli]

On 04/05/2026 16:20, Antonio Quartulli wrote:
> This patch is sent as RFC to give the AI a chance to review it once
> again, since it was able to spot a new race condition in its
> previous version.

So we got a "critical" report from sashiko-gemini.

The AI analyzed what happens along the error path, by backtracking all 
callers and checking for potential problems.

It found a potential UAF due to a worker not being disabled when 
free'ing the peer in this early stage.

The analysis seems sound and I will deepen it.

However, this bug is not introduced by this patch.
It just happened that sashiko investigated the error path due to this 
patch introducing a "return -ENODEV".
But the problem existed before because we have other spots where an 
error can be returned.

For this reason this patch now seems to be ok and to cover all issues we 
have found so far.

Once my outstanding PR for net is merged, I will send a new one with 
this fix (and maybe the UAF fix too).


Thanks a lot.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.