Commit ec6a328b authored by Paolo Abeni's avatar Paolo Abeni
Browse files

Merge tag 'ovpn-net-20250603' of https://github.com/OpenVPN/ovpn-net-next 

Antonio Quartulli says:

====================
In this batch you can find the following bug fixes:

Patch 1: when releasing a UDP socket we were wrongly invoking
setup_udp_tunnel_sock() with an empty config. This was not
properly shutting down the UDP encap state.
With this patch we simply undo what was done during setup.

Patch 2: ovpn was holding a reference to a 'struct socket'
without increasing its reference counter. This was intended
and worked as expected until we hit a race condition where
user space tries to close the socket while kernel space is
also releasing it. In this case the (struct socket *)->sk
member would disappear under our feet leading to a null-ptr-deref.
This patch fixes this issue by having struct ovpn_socket hold
a reference directly to the sk member while also increasing
its reference counter.

Patch 3: in case of errors along the TCP RX path (softirq)
we want to immediately delete the peer, but this operation may
sleep. With this patch we move the peer deletion to a scheduled
worker.

Patch 4 and 5 are instead fixing minor issues in the ovpn
kselftests.

* tag 'ovpn-net-20250603' of https://github.com/OpenVPN/ovpn-net-next:
  selftest/net/ovpn: fix missing file
  selftest/net/ovpn: fix TCP socket creation
  ovpn: avoid sleep in atomic context in TCP RX error path
  ovpn: ensure sk is still valid during cleanup
  ovpn: properly deconfigure UDP-tunnel
====================

Link: https://patch.msgid.link/20250603111110.4575-1-antonio@openvpn.net/


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parents 501fe52a 9c7e8b31

drivers/net/ovpn/io.c

+4 −4
Original line number Diff line number Diff line
@@ -134,7 +134,7 @@ void ovpn_decrypt_post(void *data, int ret) 


	rcu_read_lock();

	sock = rcu_dereference(peer->sock);

	if (sock && sock->sock->sk->sk_protocol == IPPROTO_UDP)

	if (sock && sock->sk->sk_protocol == IPPROTO_UDP)

		/* check if this peer changed local or remote endpoint */

		ovpn_peer_endpoints_update(peer, skb);

	rcu_read_unlock();
@@ -270,12 +270,12 @@ void ovpn_encrypt_post(void *data, int ret) 
	if (unlikely(!sock))

		goto err_unlock;



	switch (sock->sock->sk->sk_protocol) {

	switch (sock->sk->sk_protocol) {

	case IPPROTO_UDP:

		ovpn_udp_send_skb(peer, sock->sock, skb);

		ovpn_udp_send_skb(peer, sock->sk, skb);

		break;

	case IPPROTO_TCP:

		ovpn_tcp_send_skb(peer, sock->sock, skb);

		ovpn_tcp_send_skb(peer, sock->sk, skb);

		break;

	default:

		/* no transport configured yet */

drivers/net/ovpn/netlink.c

+8 −8
Original line number Diff line number Diff line
@@ -501,7 +501,7 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info) 
	/* when using a TCP socket the remote IP is not expected */

	rcu_read_lock();

	sock = rcu_dereference(peer->sock);

	if (sock && sock->sock->sk->sk_protocol == IPPROTO_TCP &&

	if (sock && sock->sk->sk_protocol == IPPROTO_TCP &&

	    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||

	     attrs[OVPN_A_PEER_REMOTE_IPV6])) {

		rcu_read_unlock();
@@ -559,14 +559,14 @@ static int ovpn_nl_send_peer(struct sk_buff *skb, const struct genl_info *info, 
		goto err_unlock;

	}



	if (!net_eq(genl_info_net(info), sock_net(sock->sock->sk))) {

	if (!net_eq(genl_info_net(info), sock_net(sock->sk))) {

		id = peernet2id_alloc(genl_info_net(info),

				      sock_net(sock->sock->sk),

				      sock_net(sock->sk),

				      GFP_ATOMIC);

		if (nla_put_s32(skb, OVPN_A_PEER_SOCKET_NETNSID, id))

			goto err_unlock;

	}

	local_port = inet_sk(sock->sock->sk)->inet_sport;

	local_port = inet_sk(sock->sk)->inet_sport;

	rcu_read_unlock();



	if (nla_put_u32(skb, OVPN_A_PEER_ID, peer->id))
@@ -1153,8 +1153,8 @@ int ovpn_nl_peer_del_notify(struct ovpn_peer *peer) 
		ret = -EINVAL;

		goto err_unlock;

	}

	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),

				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);

	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,

				OVPN_NLGRP_PEERS, GFP_ATOMIC);

	rcu_read_unlock();



	return 0;
@@ -1218,8 +1218,8 @@ int ovpn_nl_key_swap_notify(struct ovpn_peer *peer, u8 key_id) 
		ret = -EINVAL;

		goto err_unlock;

	}

	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sock->sk),

				msg, 0, OVPN_NLGRP_PEERS, GFP_ATOMIC);

	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,

				OVPN_NLGRP_PEERS, GFP_ATOMIC);

	rcu_read_unlock();



	return 0;

drivers/net/ovpn/peer.c

+2 −2
Original line number Diff line number Diff line
@@ -1145,7 +1145,7 @@ static void ovpn_peer_release_p2p(struct ovpn_priv *ovpn, struct sock *sk, 


	if (sk) {

		ovpn_sock = rcu_access_pointer(peer->sock);

		if (!ovpn_sock || ovpn_sock->sock->sk != sk) {

		if (!ovpn_sock || ovpn_sock->sk != sk) {

			spin_unlock_bh(&ovpn->lock);

			ovpn_peer_put(peer);

			return;
@@ -1175,7 +1175,7 @@ static void ovpn_peers_release_mp(struct ovpn_priv *ovpn, struct sock *sk, 
		if (sk) {

			rcu_read_lock();

			ovpn_sock = rcu_dereference(peer->sock);

			remove = ovpn_sock && ovpn_sock->sock->sk == sk;

			remove = ovpn_sock && ovpn_sock->sk == sk;

			rcu_read_unlock();

		}

drivers/net/ovpn/socket.c

+38 −30
Original line number Diff line number Diff line
@@ -24,9 +24,9 @@ static void ovpn_socket_release_kref(struct kref *kref) 
	struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,

						refcount);



	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)

	if (sock->sk->sk_protocol == IPPROTO_UDP)

		ovpn_udp_socket_detach(sock);

	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)

	else if (sock->sk->sk_protocol == IPPROTO_TCP)

		ovpn_tcp_socket_detach(sock);

}
@@ -75,14 +75,6 @@ void ovpn_socket_release(struct ovpn_peer *peer) 
	if (!sock)

		return;



	/* sanity check: we should not end up here if the socket

	 * was already closed

	 */

	if (!sock->sock->sk) {

		DEBUG_NET_WARN_ON_ONCE(1);

		return;

	}



	/* Drop the reference while holding the sock lock to avoid

	 * concurrent ovpn_socket_new call to mess up with a partially

	 * detached socket.
@@ -90,22 +82,24 @@ void ovpn_socket_release(struct ovpn_peer *peer) 
	 * Holding the lock ensures that a socket with refcnt 0 is fully

	 * detached before it can be picked by a concurrent reader.

	 */

	lock_sock(sock->sock->sk);

	lock_sock(sock->sk);

	released = ovpn_socket_put(peer, sock);

	release_sock(sock->sock->sk);

	release_sock(sock->sk);



	/* align all readers with sk_user_data being NULL */

	synchronize_rcu();



	/* following cleanup should happen with lock released */

	if (released) {

		if (sock->sock->sk->sk_protocol == IPPROTO_UDP) {

		if (sock->sk->sk_protocol == IPPROTO_UDP) {

			netdev_put(sock->ovpn->dev, &sock->dev_tracker);

		} else if (sock->sock->sk->sk_protocol == IPPROTO_TCP) {

		} else if (sock->sk->sk_protocol == IPPROTO_TCP) {

			/* wait for TCP jobs to terminate */

			ovpn_tcp_socket_wait_finish(sock);

			ovpn_peer_put(sock->peer);

		}

		/* drop reference acquired in ovpn_socket_new() */

		sock_put(sock->sk);

		/* we can call plain kfree() because we already waited one RCU

		 * period due to synchronize_rcu()

		 */
@@ -118,12 +112,14 @@ static bool ovpn_socket_hold(struct ovpn_socket *sock) 
	return kref_get_unless_zero(&sock->refcount);

}



static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer)

static int ovpn_socket_attach(struct ovpn_socket *ovpn_sock,

			      struct socket *sock,

			      struct ovpn_peer *peer)

{

	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)

		return ovpn_udp_socket_attach(sock, peer->ovpn);

	else if (sock->sock->sk->sk_protocol == IPPROTO_TCP)

		return ovpn_tcp_socket_attach(sock, peer);

	if (sock->sk->sk_protocol == IPPROTO_UDP)

		return ovpn_udp_socket_attach(ovpn_sock, sock, peer->ovpn);

	else if (sock->sk->sk_protocol == IPPROTO_TCP)

		return ovpn_tcp_socket_attach(ovpn_sock, peer);



	return -EOPNOTSUPP;

}
@@ -138,14 +134,15 @@ static int ovpn_socket_attach(struct ovpn_socket *sock, struct ovpn_peer *peer) 
struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)

{

	struct ovpn_socket *ovpn_sock;

	struct sock *sk = sock->sk;

	int ret;



	lock_sock(sock->sk);

	lock_sock(sk);



	/* a TCP socket can only be owned by a single peer, therefore there

	 * can't be any other user

	 */

	if (sock->sk->sk_protocol == IPPROTO_TCP && sock->sk->sk_user_data) {

	if (sk->sk_protocol == IPPROTO_TCP && sk->sk_user_data) {

		ovpn_sock = ERR_PTR(-EBUSY);

		goto sock_release;

	}
@@ -153,8 +150,8 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) 
	/* a UDP socket can be shared across multiple peers, but we must make

	 * sure it is not owned by something else

	 */

	if (sock->sk->sk_protocol == IPPROTO_UDP) {

		u8 type = READ_ONCE(udp_sk(sock->sk)->encap_type);

	if (sk->sk_protocol == IPPROTO_UDP) {

		u8 type = READ_ONCE(udp_sk(sk)->encap_type);



		/* socket owned by other encapsulation module */

		if (type && type != UDP_ENCAP_OVPNINUDP) {
@@ -163,7 +160,7 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) 
		}



		rcu_read_lock();

		ovpn_sock = rcu_dereference_sk_user_data(sock->sk);

		ovpn_sock = rcu_dereference_sk_user_data(sk);

		if (ovpn_sock) {

			/* socket owned by another ovpn instance, we can't use it */

			if (ovpn_sock->ovpn != peer->ovpn) {
@@ -200,11 +197,22 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) 
		goto sock_release;

	}



	ovpn_sock->sock = sock;

	ovpn_sock->sk = sk;

	kref_init(&ovpn_sock->refcount);



	ret = ovpn_socket_attach(ovpn_sock, peer);

	/* the newly created ovpn_socket is holding reference to sk,

	 * therefore we increase its refcounter.

	 *

	 * This ovpn_socket instance is referenced by all peers

	 * using the same socket.

	 *

	 * ovpn_socket_release() will take care of dropping the reference.

	 */

	sock_hold(sk);



	ret = ovpn_socket_attach(ovpn_sock, sock, peer);

	if (ret < 0) {

		sock_put(sk);

		kfree(ovpn_sock);

		ovpn_sock = ERR_PTR(ret);

		goto sock_release;
@@ -213,11 +221,11 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) 
	/* TCP sockets are per-peer, therefore they are linked to their unique

	 * peer

	 */

	if (sock->sk->sk_protocol == IPPROTO_TCP) {

	if (sk->sk_protocol == IPPROTO_TCP) {

		INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);

		ovpn_sock->peer = peer;

		ovpn_peer_hold(peer);

	} else if (sock->sk->sk_protocol == IPPROTO_UDP) {

	} else if (sk->sk_protocol == IPPROTO_UDP) {

		/* in UDP we only link the ovpn instance since the socket is

		 * shared among multiple peers

		 */
@@ -226,8 +234,8 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) 
			    GFP_KERNEL);

	}



	rcu_assign_sk_user_data(sock->sk, ovpn_sock);

	rcu_assign_sk_user_data(sk, ovpn_sock);

sock_release:

	release_sock(sock->sk);

	release_sock(sk);

	return ovpn_sock;

}

drivers/net/ovpn/socket.h

+2 −2
Original line number Diff line number Diff line
@@ -22,7 +22,7 @@ struct ovpn_peer; 
 * @ovpn: ovpn instance owning this socket (UDP only)

 * @dev_tracker: reference tracker for associated dev (UDP only)

 * @peer: unique peer transmitting over this socket (TCP only)

 * @sock: the low level sock object

 * @sk: the low level sock object

 * @refcount: amount of contexts currently referencing this object

 * @work: member used to schedule release routine (it may block)

 * @tcp_tx_work: work for deferring outgoing packet processing (TCP only)
@@ -36,7 +36,7 @@ struct ovpn_socket { 
		struct ovpn_peer *peer;

	};



	struct socket *sock;

	struct sock *sk;

	struct kref refcount;

	struct work_struct work;

	struct work_struct tcp_tx_work;