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

Sabrina Dubroca
2024-05-08T17:11:06Z

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