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

Antonio Quartulli
2024-05-08T20:37:44Z

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.