Message-ID: <1dffb833-1688-4572-bbf8-c6524cd84402@openvpn.net>
Date: Fri, 15 Nov 2024 15:28:35 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 07/23] ovpn: introduce the ovpn_socket object

On 10/11/2024 19:26, Sergey Ryazanov wrote:
> On 29.10.2024 12:47, 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 attached to them for status tracking
>> purposes.
>>
>> Initially only UDP support is introduced. TCP will come in a later
>> patch.
>>
>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
> 
> [...]
> 
>> diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..090a3232ab0ec19702110f1a90f45c7f10889f6f
>> --- /dev/null
>> +++ b/drivers/net/ovpn/socket.c
>> @@ -0,0 +1,120 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*  OpenVPN data channel offload
>> + *
>> + *  Copyright (C) 2020-2024 OpenVPN, Inc.
>> + *
>> + *  Author:    James Yonan <james@...nvpn.net>
>> + *        Antonio Quartulli <antonio@...nvpn.net>
>> + */
>> +
>> +#include <linux/net.h>
>> +#include <linux/netdevice.h>
>> +
>> +#include "ovpnstruct.h"
>> +#include "main.h"
>> +#include "io.h"
>> +#include "peer.h"
>> +#include "socket.h"
>> +#include "udp.h"
>> +
>> +static void ovpn_socket_detach(struct socket *sock)
>> +{
>> +    if (!sock)
>> +        return;
>> +
>> +    sockfd_put(sock);
>> +}
>> +
>> +/**
>> + * ovpn_socket_release_kref - kref_put callback
>> + * @kref: the kref object
>> + */
>> +void ovpn_socket_release_kref(struct kref *kref)
>> +{
>> +    struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,
>> +                        refcount);
>> +
>> +    ovpn_socket_detach(sock->sock);
>> +    kfree_rcu(sock, rcu);
>> +}
>> +
>> +static bool ovpn_socket_hold(struct ovpn_socket *sock)
>> +{
>> +    return kref_get_unless_zero(&sock->refcount);
> 
> Why do we need to wrap this kref acquiring call into the function. Why 
> we cannot simply call kref_get_unless_zero() from ovpn_socket_get()?

Generally I prefer to keep the API among objects consistent.
In this specific case, it means having hold() and put() helpers in order 
to avoid calling kref_* functions directly in the code.

This is a pretty simple case because hold() is called only once, but I 
still like to be consistent.

> 
>> +}
>> +
>> +static struct ovpn_socket *ovpn_socket_get(struct socket *sock)
>> +{
>> +    struct ovpn_socket *ovpn_sock;
>> +
>> +    rcu_read_lock();
>> +    ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
>> +    if (!ovpn_socket_hold(ovpn_sock)) {
>> +        pr_warn("%s: found ovpn_socket with ref = 0\n", __func__);
> 
> Should we be more specific here and print warning with 
> netdev_warn(ovpn_sock->ovpn->dev, ...)?

ACK must be an unnoticed leftover

> 
> And, BTW, how we can pick-up a half-destroyed socket?

I don't think this can happen under basic conditions.
But I am pretty sure in case of bugs this *could* happen quite easily.

[...]

>> +/**
>> + * ovpn_udp_socket_attach - set udp-tunnel CBs on socket and link it 
>> to ovpn
>> + * @sock: socket to configure
>> + * @ovpn: the openvp instance to link
>> + *
>> + * After invoking this function, the sock will be controlled by ovpn 
>> so that
>> + * any incoming packet may be processed by ovpn first.
>> + *
>> + * Return: 0 on success or a negative error code otherwise
>> + */
>> +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct 
>> *ovpn)
>> +{
>> +    struct ovpn_socket *old_data;
>> +    int ret = 0;
>> +
>> +    /* sanity check */
>> +    if (sock->sk->sk_protocol != IPPROTO_UDP) {
> 
> The function will be called only for a UDP socket. The caller makes sure 
> this is truth. So, why do we need this check?

To avoid this function being copied/called somewhere else in the future 
and we forget about this critical assumption.

Indeed it's a just sanity check.

> 
>> +        DEBUG_NET_WARN_ON_ONCE(1);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* make sure no pre-existing encapsulation handler exists */
>> +    rcu_read_lock();
>> +    old_data = rcu_dereference_sk_user_data(sock->sk);
>> +    if (!old_data) {
>> +        /* socket is currently unused - we can take it */
>> +        rcu_read_unlock();
>> +        return 0;
>> +    }
>> +
>> +    /* socket is in use. We need to understand if it's owned by this 
>> ovpn
>> +     * instance or by something else.
>> +     * In the former case, we can increase the refcounter and happily
>> +     * use it, because the same UDP socket is expected to be shared 
>> among
>> +     * different peers.
>> +     *
>> +     * Unlikely TCP, a single UDP socket can be used to talk to many 
>> remote
>> +     * hosts and therefore openvpn instantiates one only for all its 
>> peers
>> +     */
>> +    if ((READ_ONCE(udp_sk(sock->sk)->encap_type) == 
>> UDP_ENCAP_OVPNINUDP) &&
>> +        old_data->ovpn == ovpn) {
>> +        netdev_dbg(ovpn->dev,
>> +               "%s: provided socket already owned by this interface\n",
>> +               __func__);
> 
> Why do we need the function name being printed here?

leftover, will fix, thanks!

> 
>> +        ret = -EALREADY;
>> +    } else {
>> +        netdev_err(ovpn->dev,
>> +               "%s: provided socket already taken by other user\n",
>> +               __func__);
> 
> The same comment regarding the function name printing.

ACK

> 
> And why 'error' level? There is a few ways to fall into this case and 
> each of them implies a user-space screw up. But why we consider these 
> user-space screw ups our (kernel) problem? I suggesting to reduce level 
> at least to 'warning' or maybe even 'notice'. See level definitions in 
> include/linux/kern_levels.h

Yeah, this can be reduced. The error will be reported to the user via 
netlink in any case.

Thanks!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists