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@xxxxxxxxxxx>
[...]
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@xxxxxxxxxxx>
+ * Antonio Quartulli <antonio@xxxxxxxxxxx>
+ */
+
+#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.
