During initialization, we override socket callbacks and set sk_user_data
to an ovpn_socket instance. Currently, these two operations are
decoupled: callbacks are overridden before sk_user_data is set. While
existing callbacks perform safety checks for NULL or non-ovpn
sk_user_data, this condition causes a "half-formed" state where valid
packets arriving during attachment trigger error logs (e.g., "invoked on
non ovpn socket").
Set sk_user_data before overriding the callbacks so that it can be
accessed safely from them. Since we already check that the socket has no
sk_user_data before setting it, this remains safe even if an interrupt
accesses the socket after sk_user_data is set but before the callbacks
are overridden.
This also requires initializing all protocol-specific fields (such as
tcp_tx_work and peer links) before calling ovpn_socket_attach, ensuring
the ovpn_socket is fully formed before it becomes visible to any
callback.
Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
Signed-off-by: Ralf Lici <[email protected]>
---
Changes since v1:
- reset sk_user_data to NULL in case of error
- removed a redundant goto sock_release in ovpn_socket_new
- expanded commit message with additional information on the purpose of
this change
- added explanation in the commit message of why the per-protocol
ovpn_sock initialization code in ovpn_socket_new was moved
- added Fixes tag
drivers/net/ovpn/socket.c | 39 +++++++++++++++++++++------------------
drivers/net/ovpn/tcp.c | 9 +++++++--
drivers/net/ovpn/udp.c | 1 +
3 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index 9750871ab65c..448cee3b3f9f 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -200,6 +200,22 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock,
struct ovpn_peer *peer)
ovpn_sock->sk = sk;
kref_init(&ovpn_sock->refcount);
+ /* TCP sockets are per-peer, therefore they are linked to their unique
+ * peer
+ */
+ 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 (sk->sk_protocol == IPPROTO_UDP) {
+ /* in UDP we only link the ovpn instance since the socket is
+ * shared among multiple peers
+ */
+ ovpn_sock->ovpn = peer->ovpn;
+ netdev_hold(peer->ovpn->dev, &ovpn_sock->dev_tracker,
+ GFP_KERNEL);
+ }
+
/* the newly created ovpn_socket is holding reference to sk,
* therefore we increase its refcounter.
*
@@ -212,29 +228,16 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock,
struct ovpn_peer *peer)
ret = ovpn_socket_attach(ovpn_sock, sock, peer);
if (ret < 0) {
+ if (sk->sk_protocol == IPPROTO_TCP)
+ ovpn_peer_put(peer);
+ else if (sk->sk_protocol == IPPROTO_UDP)
+ netdev_put(peer->ovpn->dev, &ovpn_sock->dev_tracker);
+
sock_put(sk);
kfree(ovpn_sock);
ovpn_sock = ERR_PTR(ret);
- goto sock_release;
- }
-
- /* TCP sockets are per-peer, therefore they are linked to their unique
- * peer
- */
- 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 (sk->sk_protocol == IPPROTO_UDP) {
- /* in UDP we only link the ovpn instance since the socket is
- * shared among multiple peers
- */
- ovpn_sock->ovpn = peer->ovpn;
- netdev_hold(peer->ovpn->dev, &ovpn_sock->dev_tracker,
- GFP_KERNEL);
}
- rcu_assign_sk_user_data(sk, ovpn_sock);
sock_release:
release_sock(sk);
return ovpn_sock;
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 0d7f30360d87..f0b4e07ba924 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -487,6 +487,7 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
/* make sure no pre-existing encapsulation handler exists */
if (ovpn_sock->sk->sk_user_data)
return -EBUSY;
+ rcu_assign_sk_user_data(ovpn_sock->sk, ovpn_sock);
/* only a fully connected socket is expected. Connection should be
* handled in userspace
@@ -495,13 +496,14 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
net_err_ratelimited("%s: provided TCP socket is not in
ESTABLISHED state: %d\n",
netdev_name(peer->ovpn->dev),
ovpn_sock->sk->sk_state);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
ret = strp_init(&peer->tcp.strp, ovpn_sock->sk, &cb);
if (ret < 0) {
DEBUG_NET_WARN_ON_ONCE(1);
- return ret;
+ goto err;
}
INIT_WORK(&peer->tcp.defer_del_work, ovpn_tcp_peer_del_work);
@@ -536,6 +538,9 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
strp_check_rcv(&peer->tcp.strp);
return 0;
+err:
+ rcu_assign_sk_user_data(ovpn_sock->sk, NULL);
+ return ret;
}
static void ovpn_tcp_close(struct sock *sk, long timeout)
diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index d6a0f7a0b75d..272b535ecaad 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -386,6 +386,7 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
struct socket *sock,
struct ovpn_priv *ovpn)
{
struct udp_tunnel_sock_cfg cfg = {
+ .sk_user_data = ovpn_sock,
.encap_type = UDP_ENCAP_OVPNINUDP,
.encap_rcv = ovpn_udp_encap_recv,
.encap_destroy = ovpn_udp_encap_destroy,
--
2.52.0
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
