[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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