2024-05-06, 03:16:21 +0200, Antonio Quartulli wrote:

This specific structure is used in the ovpn kernel module
to wrap and carry around a standard kernel socket.

ovpn takes ownership of passed sockets and therefore an ovpn
specific objects is attathced to them for status tracking

typos:      object    attached

[...]

+
+/* Finalize release of socket, called after RCU grace period */

kref_put seems to call ovpn_socket_release_kref without waiting, and
then that calls ovpn_socket_detach immediately as well. Am I missing
something?

+static void ovpn_socket_detach(struct socket *sock)
+{
+	if (!sock)
+		return;
+
+	sockfd_put(sock);
+}

[...]

+
+/* Finalize release of socket, called after RCU grace period */

Did that comment get misplaced? It doesn't match the code.

+static int ovpn_socket_attach(struct socket *sock, struct ovpn_peer *peer)
+{
+	int ret = -EOPNOTSUPP;
+
+	if (!sock || !peer)
+		return -EINVAL;
+
+	if (sock->sk->sk_protocol == IPPROTO_UDP)
+		ret = ovpn_udp_socket_attach(sock, peer->ovpn);
+
+	return ret;
+}

[...]

+
+int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
+{
+	struct ovpn_socket *old_data;
+
+	/* sanity check */
+	if (sock->sk->sk_protocol != IPPROTO_UDP) {
+		netdev_err(ovpn->dev, "%s: expected UDP socket\n", __func__);

Maybe use DEBUG_NET_WARN_ON_ONCE here since it's never expected to
actually happen? That would help track down (in test/debug setups) how
we ended up here.

+		return -EINVAL;
+	}
+
+	/* make sure no pre-existing encapsulation handler exists */
+	rcu_read_lock();
+	old_data = rcu_dereference_sk_user_data(sock->sk);
+	rcu_read_unlock();
+	if (old_data) {
+		if (old_data->ovpn == ovpn) {

You should stay under rcu_read_unlock if you access old_data's fields.

+			netdev_dbg(ovpn->dev,
+				   "%s: provided socket already owned by this interface\n",
+				   __func__);
+			return -EALREADY;
+		}
+
+		netdev_err(ovpn->dev, "%s: provided socket already taken by other user\n",
+			   __func__);
+		return -EBUSY;
+	}
+
+	return 0;
+}

-- 
Sabrina