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@openvpn.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@openvpn.net
- * Antonio Quartulli antonio@openvpn.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,