Re: [PATCH net-next v3 08/24] ovpn: introduce the ovpn_socket object

[Sabrina Dubroca <sd@queasysnail.net>]

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


> diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
> new file mode 100644
> index 000000000000..a4a4d69162f0
> --- /dev/null
> +++ b/drivers/net/ovpn/socket.c
[...]
> +
> +/* 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;
> +}

> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
> new file mode 100644
> index 000000000000..4b7d96a13df0
> --- /dev/null
> +++ b/drivers/net/ovpn/udp.c
[...]
> +
> +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