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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 8 May 2024 19:10:56 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Sergey Ryazanov <ryazanov.s.a@...il.com>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 08/24] ovpn: introduce the ovpn_socket object

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