netdev - Re: [PATCH net-next v18 20/25] ovpn: implement peer add/get/dump/delete via netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30c50783-096b-4114-aa55-c3edbeb38d49@openvpn.net>
Date: Mon, 20 Jan 2025 15:52:10 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v18 20/25] ovpn: implement peer
 add/get/dump/delete via netlink

On 17/01/2025 12:48, Sabrina Dubroca wrote:
[...]
> -------- 8< --------
> 
> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 72357bb5f30b..19aa4ee6d468 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c
> @@ -733,6 +733,9 @@ int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info)
>   
>   	netdev_dbg(ovpn->dev, "del peer %u\n", peer->id);
>   	ret = ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_USERSPACE);
> +	if (ret >= 0 && peer->sock)
> +		wait_for_completion(&peer->sock_detach);
> +
>   	ovpn_peer_put(peer);
>   
>   	return ret;
> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> index b032390047fe..6120521d0c32 100644
> --- a/drivers/net/ovpn/peer.c
> +++ b/drivers/net/ovpn/peer.c
> @@ -92,6 +92,7 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_priv *ovpn, u32 id)
>   	ovpn_peer_stats_init(&peer->vpn_stats);
>   	ovpn_peer_stats_init(&peer->link_stats);
>   	INIT_WORK(&peer->keepalive_work, ovpn_peer_keepalive_send);
> +	init_completion(&peer->sock_detach);
>   
>   	ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
>   	if (ret < 0) {
> diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
> index 7a062cc5a5a4..8c54bf5709ef 100644
> --- a/drivers/net/ovpn/peer.h
> +++ b/drivers/net/ovpn/peer.h
> @@ -112,6 +112,7 @@ struct ovpn_peer {
>   	struct rcu_head rcu;
>   	struct work_struct remove_work;
>   	struct work_struct keepalive_work;
> +	struct completion sock_detach;
>   };
>   
>   /**
> diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
> index a5c3bc834a35..7cefac42c3be 100644
> --- a/drivers/net/ovpn/socket.c
> +++ b/drivers/net/ovpn/socket.c
> @@ -31,6 +31,8 @@ static void ovpn_socket_release_kref(struct kref *kref)
>   
>   	sockfd_put(sock->sock);
>   	kfree_rcu(sock, rcu);
> +
> +	complete(&sock->peer->sock_detach);
>   }
>   
>   /**
> @@ -181,12 +183,12 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
>   
>   	ovpn_sock->sock = sock;
>   	kref_init(&ovpn_sock->refcount);
> +	ovpn_sock->peer = peer;

Unfortunately this won't work for UDP, because we are taking a reference 
only to the *first* peer using this specific socket (= the peer 
allocating the ovpn_socket object).
All other peers won't get to this line.

Instead, we should somewhat only take the reference to the "last" peer 
using this socket: the peer which will truly ignite the socket detachment.
That's the peer that has to wait upon peer_del for full socket detachment.

Trying to get something smart out of my brains right now ...

Cheers,

>   
>   	/* TCP sockets are per-peer, therefore they are linked to their unique
>   	 * peer
>   	 */
>   	if (sock->sk->sk_protocol == IPPROTO_TCP) {
> -		ovpn_sock->peer = peer;
>   		ovpn_peer_hold(peer);
>   	} else if (sock->sk->sk_protocol == IPPROTO_UDP) {
>   		/* in UDP we only link the ovpn instance since the socket is
> diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h
> index 15827e347f53..3f5a35fd9048 100644
> --- a/drivers/net/ovpn/socket.h
> +++ b/drivers/net/ovpn/socket.h
> @@ -28,12 +28,12 @@ struct ovpn_peer;
>    * @rcu: member used to schedule RCU destructor callback
>    */
>   struct ovpn_socket {
> +	struct ovpn_peer *peer;
>   	union {
>   		struct {
>   			struct ovpn_priv *ovpn;
>   			netdevice_tracker dev_tracker;
>   		};
> -		struct ovpn_peer *peer;
>   	};
>   
>   	struct socket *sock;
> 
> 

-- 
Antonio Quartulli
OpenVPN Inc.