Message-ID: <62d382f8-ea45-4157-b54b-8fed7bdafcca@gmail.com>
Date: Sun, 10 Nov 2024 20:26:52 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
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 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()?

> +}
> +
> +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, ...)?

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

> +		ovpn_sock = NULL;
> +	}
> +	rcu_read_unlock();
> +
> +	return ovpn_sock;
> +}
> +
> +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;
> +}
> +
> +/**
> + * ovpn_socket_new - create a new socket and initialize it
> + * @sock: the kernel socket to embed
> + * @peer: the peer reachable via this socket
> + *
> + * Return: an openvpn socket on success or a negative error code otherwise
> + */
> +struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
> +{
> +	struct ovpn_socket *ovpn_sock;
> +	int ret;
> +
> +	ret = ovpn_socket_attach(sock, peer);
> +	if (ret < 0 && ret != -EALREADY)
> +		return ERR_PTR(ret);
> +
> +	/* if this socket is already owned by this interface, just increase the
> +	 * refcounter and use it as expected.
> +	 *
> +	 * Since UDP sockets can be used to talk to multiple remote endpoints,
> +	 * openvpn normally instantiates only one socket and shares it among all
> +	 * its peers. For this reason, when we find out that a socket is already
> +	 * used for some other peer in *this* instance, we can happily increase
> +	 * its refcounter and use it normally.
> +	 */
> +	if (ret == -EALREADY) {
> +		/* caller is expected to increase the sock refcounter before
> +		 * passing it to this function. For this reason we drop it if
> +		 * not needed, like when this socket is already owned.
> +		 */
> +		ovpn_sock = ovpn_socket_get(sock);
> +		sockfd_put(sock);
> +		return ovpn_sock;
> +	}
> +
> +	ovpn_sock = kzalloc(sizeof(*ovpn_sock), GFP_KERNEL);
> +	if (!ovpn_sock)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ovpn_sock->ovpn = peer->ovpn;
> +	ovpn_sock->sock = sock;
> +	kref_init(&ovpn_sock->refcount);
> +
> +	rcu_assign_sk_user_data(sock->sk, ovpn_sock);
> +
> +	return ovpn_sock;
> +}
> diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ad9c5073b085482da95ee8ebf40acf20bf2e4b3
> --- /dev/null
> +++ b/drivers/net/ovpn/socket.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2020-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@...nvpn.net>
> + *		Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#ifndef _NET_OVPN_SOCK_H_
> +#define _NET_OVPN_SOCK_H_
> +
> +#include <linux/net.h>
> +#include <linux/kref.h>
> +#include <net/sock.h>
> +
> +struct ovpn_struct;
> +struct ovpn_peer;
> +
> +/**
> + * struct ovpn_socket - a kernel socket referenced in the ovpn code
> + * @ovpn: ovpn instance owning this socket (UDP only)
> + * @sock: the low level sock object
> + * @refcount: amount of contexts currently referencing this object
> + * @rcu: member used to schedule RCU destructor callback
> + */
> +struct ovpn_socket {
> +	struct ovpn_struct *ovpn;
> +	struct socket *sock;
> +	struct kref refcount;
> +	struct rcu_head rcu;
> +};
> +
> +void ovpn_socket_release_kref(struct kref *kref);
> +
> +/**
> + * ovpn_socket_put - decrease reference counter
> + * @sock: the socket whose reference counter should be decreased
> + */
> +static inline void ovpn_socket_put(struct ovpn_socket *sock)
> +{
> +	kref_put(&sock->refcount, ovpn_socket_release_kref);
> +}
> +
> +struct ovpn_socket *ovpn_socket_new(struct socket *sock,
> +				    struct ovpn_peer *peer);
> +
> +#endif /* _NET_OVPN_SOCK_H_ */
> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c10474d252e19a0626d17a6f5dd328a5e5811551
> --- /dev/null
> +++ b/drivers/net/ovpn/udp.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/socket.h>
> +#include <net/udp.h>
> +
> +#include "ovpnstruct.h"
> +#include "main.h"
> +#include "socket.h"
> +#include "udp.h"
> +
> +/**
> + * 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?

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

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

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

> +		ret = -EBUSY;
> +	}
> +	rcu_read_unlock();
> +
> +	return ret;
> +}

--
Sergey

Powered by blists - more mailing lists