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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <53dc5388-630f-47e1-a6c1-6c3bb91ee2ac@openvpn.net>
Date: Wed, 8 May 2024 22:38:58 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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

On 08/05/2024 19:10, Sabrina Dubroca wrote:
> 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

thanks

> 
> 
>> 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?

hmm what do we need to wait for exactly? (Maybe I am missing something)
The ovpn_socket will survive a bit longer thanks to kfree_rcu.

> 
>> +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.

yeah it did. wiping it.

> 
>> +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.

will do, thanks for the suggestion

> 
>> +		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.

My assumption was: if we have an ovpn object in the user data, it means 
that its reference counter was increased to account for this usage.

But I presume we have no guarantee that it won't be decreased while 
outside of the rcu read lock area.

Will move the check inside.

> 
>> +			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;
>> +}
> 

Thanks!

-- 
Antonio Quartulli
OpenVPN Inc.