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

[Antonio Quartulli <antonio@openvpn.net>]

Hi!

On 24/03/2026 23:40, Sabrina Dubroca wrote:
> 2026-03-24, 14:30:06 -0700, Jakub Kicinski wrote:
>> On Tue, 24 Mar 2026 11:09:11 +0100 Sabrina Dubroca wrote:
>>> -------- 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.
>>
>> LGTM. This or change all the write paths to check if the device is still
>> alive after taking the lock.
> 
> I think peer_new is the only relevant path here (other paths use an
> existing peer and modify/delete the peer itself or its keys while
> holding a reference), and we don't take a lock except to insert the
> peer in the ovpn instance (ie netdev)'s hashtable (or peer slot, for
> single-peer instances). I guess we could add the reg_state >=
> NETREG_UNREGISTERING check to ovpn_peer_add_{p2p,mp} and reject adding
> the peer. It seems cleaner than my ovpn_nl_post_doit() diff above.

Yeah, I like the check in ovpn_peer_add_* too.

> 
>>> 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).
>>
>> Hm, wouldn't we need a notification _after_ netdev_wait_allrefs_any() ?
> 
> netdev_wait_allrefs_any() will never complete if we don't clean up all
> the peers, since they hold a ref on the netdev.
> 
> But calling ovpn_peers_free on netdev_wait_allrefs_any()'s
> 
>      /* Rebroadcast unregister notification */
> 
> should clean up peers that got added while we were unregistering.

But with the check in ovpn_peer_add_*, we don't need this extra call to 
ovpn_peers_free(), right?

> 
>>> And s/cancel_delayed_work_sync/disable_delayed_work_sync/ for the
>>> keepalive_work.

ACK

Regards,

>>>
>>>
>>> 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.
>>
>> Yup, I think the LLMs are trying to be helpful and are looking for some
>> write lock earlier in the path. As much as they are annoying I can't
>> blame them here, I feel like we try to make things lockless too often.
> 
> Well, if we want to protect against netdev removal, we have to use the
> one lock one we're trying hard to not depend on so much (rtnl)?
> At the ovpn level operations are not lockless.
> 
> The LLM found a legit race that I missed, but I'm always puzzled by
> inconsistencies like that.

-- 
Antonio Quartulli
OpenVPN Inc.