| Message ID | 20240506011637.27272-9-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-05-06, 03:16:21 +0200, 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 attathced to them for status tracking typos: object attached > diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c > new file mode 100644 > index 000000000000..a4a4d69162f0 > --- /dev/null > +++ b/drivers/net/ovpn/socket.c [...] > + > +/* Finalize release of socket, called after RCU grace period */ kref_put seems to call ovpn_socket_release_kref without waiting, and then that calls ovpn_socket_detach immediately as well. Am I missing something? > +static void ovpn_socket_detach(struct socket *sock) > +{ > + if (!sock) > + return; > + > + sockfd_put(sock); > +} [...] > + > +/* Finalize release of socket, called after RCU grace period */ Did that comment get misplaced? It doesn't match the code. > +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; > +} > diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c > new file mode 100644 > index 000000000000..4b7d96a13df0 > --- /dev/null > +++ b/drivers/net/ovpn/udp.c [...] > + > +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn) > +{ > + struct ovpn_socket *old_data; > + > + /* sanity check */ > + if (sock->sk->sk_protocol != IPPROTO_UDP) { > + netdev_err(ovpn->dev, "%s: expected UDP socket\n", __func__); Maybe use DEBUG_NET_WARN_ON_ONCE here since it's never expected to actually happen? That would help track down (in test/debug setups) how we ended up here. > + return -EINVAL; > + } > + > + /* make sure no pre-existing encapsulation handler exists */ > + rcu_read_lock(); > + old_data = rcu_dereference_sk_user_data(sock->sk); > + rcu_read_unlock(); > + if (old_data) { > + if (old_data->ovpn == ovpn) { You should stay under rcu_read_unlock if you access old_data's fields. > + netdev_dbg(ovpn->dev, > + "%s: provided socket already owned by this interface\n", > + __func__); > + return -EALREADY; > + } > + > + netdev_err(ovpn->dev, "%s: provided socket already taken by other user\n", > + __func__); > + return -EBUSY; > + } > + > + return 0; > +}
On 08/05/2024 19:10, Sabrina Dubroca wrote: > 2024-05-06, 03:16:21 +0200, 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 attathced to them for status tracking > > typos: object attached thanks > > >> diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c >> new file mode 100644 >> index 000000000000..a4a4d69162f0 >> --- /dev/null >> +++ b/drivers/net/ovpn/socket.c > [...] >> + >> +/* Finalize release of socket, called after RCU grace period */ > > kref_put seems to call ovpn_socket_release_kref without waiting, and > then that calls ovpn_socket_detach immediately as well. Am I missing > something? hmm what do we need to wait for exactly? (Maybe I am missing something) The ovpn_socket will survive a bit longer thanks to kfree_rcu. > >> +static void ovpn_socket_detach(struct socket *sock) >> +{ >> + if (!sock) >> + return; >> + >> + sockfd_put(sock); >> +} > > [...] >> + >> +/* Finalize release of socket, called after RCU grace period */ > > Did that comment get misplaced? It doesn't match the code. yeah it did. wiping it. > >> +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; >> +} > >> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c >> new file mode 100644 >> index 000000000000..4b7d96a13df0 >> --- /dev/null >> +++ b/drivers/net/ovpn/udp.c > [...] >> + >> +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn) >> +{ >> + struct ovpn_socket *old_data; >> + >> + /* sanity check */ >> + if (sock->sk->sk_protocol != IPPROTO_UDP) { >> + netdev_err(ovpn->dev, "%s: expected UDP socket\n", __func__); > > Maybe use DEBUG_NET_WARN_ON_ONCE here since it's never expected to > actually happen? That would help track down (in test/debug setups) how > we ended up here. will do, thanks for the suggestion > >> + return -EINVAL; >> + } >> + >> + /* make sure no pre-existing encapsulation handler exists */ >> + rcu_read_lock(); >> + old_data = rcu_dereference_sk_user_data(sock->sk); >> + rcu_read_unlock(); >> + if (old_data) { >> + if (old_data->ovpn == ovpn) { > > You should stay under rcu_read_unlock if you access old_data's fields. My assumption was: if we have an ovpn object in the user data, it means that its reference counter was increased to account for this usage. But I presume we have no guarantee that it won't be decreased while outside of the rcu read lock area. Will move the check inside. > >> + netdev_dbg(ovpn->dev, >> + "%s: provided socket already owned by this interface\n", >> + __func__); >> + return -EALREADY; >> + } >> + >> + netdev_err(ovpn->dev, "%s: provided socket already taken by other user\n", >> + __func__); >> + return -EBUSY; >> + } >> + >> + return 0; >> +} > Thanks!
2024-05-08, 22:38:58 +0200, Antonio Quartulli wrote: > On 08/05/2024 19:10, Sabrina Dubroca wrote: > > 2024-05-06, 03:16:21 +0200, Antonio Quartulli wrote: > > > diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c > > > new file mode 100644 > > > index 000000000000..a4a4d69162f0 > > > --- /dev/null > > > +++ b/drivers/net/ovpn/socket.c > > [...] > > > + > > > +/* Finalize release of socket, called after RCU grace period */ > > > > kref_put seems to call ovpn_socket_release_kref without waiting, and > > then that calls ovpn_socket_detach immediately as well. Am I missing > > something? > > hmm what do we need to wait for exactly? (Maybe I am missing something) > The ovpn_socket will survive a bit longer thanks to kfree_rcu. The way I read this comment, it says that ovpn_socket_detach will be called after one RCU grace period, but I don't see where that grace period would come from. ovpn_socket_put -> kref_put(release=ovpn_socket_release_kref) -> ovpn_socket_release_kref -> ovpn_socket_detach No grace period here. Or am I misinterpreting the comment? There will be a grace period caused by kfree_rcu before the ovpn_socket is actually freed, is that what the comment means? > > > +static void ovpn_socket_detach(struct socket *sock) > > > +{ > > > + if (!sock) > > > + return; > > > + > > > + sockfd_put(sock); > > > +} > >
On 09/05/2024 15:32, Sabrina Dubroca wrote: > 2024-05-08, 22:38:58 +0200, Antonio Quartulli wrote: >> On 08/05/2024 19:10, Sabrina Dubroca wrote: >>> 2024-05-06, 03:16:21 +0200, Antonio Quartulli wrote: >>>> diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c >>>> new file mode 100644 >>>> index 000000000000..a4a4d69162f0 >>>> --- /dev/null >>>> +++ b/drivers/net/ovpn/socket.c >>> [...] >>>> + >>>> +/* Finalize release of socket, called after RCU grace period */ >>> >>> kref_put seems to call ovpn_socket_release_kref without waiting, and >>> then that calls ovpn_socket_detach immediately as well. Am I missing >>> something? >> >> hmm what do we need to wait for exactly? (Maybe I am missing something) >> The ovpn_socket will survive a bit longer thanks to kfree_rcu. > > The way I read this comment, it says that ovpn_socket_detach will be > called after one RCU grace period, but I don't see where that grace > period would come from. > > ovpn_socket_put -> kref_put(release=ovpn_socket_release_kref) -> > ovpn_socket_release_kref -> ovpn_socket_detach > > No grace period here. > > Or am I misinterpreting the comment? There will be a grace period > caused by kfree_rcu before the ovpn_socket is actually freed, is that > what the comment means? Forgive me - only now I realized that you were referring to what the comment says. That comment is just totally busted. I think it was there since the code was doing something totally different and was carried over and over by mistake. Sorry
diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile index ce13499b3e17..56bddc9bef83 100644 --- a/drivers/net/ovpn/Makefile +++ b/drivers/net/ovpn/Makefile @@ -13,3 +13,5 @@ ovpn-y += io.o ovpn-y += netlink.o ovpn-y += netlink-gen.o ovpn-y += peer.o +ovpn-y += socket.o +ovpn-y += udp.o diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c new file mode 100644 index 000000000000..a4a4d69162f0 --- /dev/null +++ b/drivers/net/ovpn/socket.c @@ -0,0 +1,105 @@ +// 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" + +/* Finalize release of socket, called after RCU grace period */ +static void ovpn_socket_detach(struct socket *sock) +{ + if (!sock) + return; + + sockfd_put(sock); +} + +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); +} + +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__); + ovpn_sock = NULL; + } + rcu_read_unlock(); + + return ovpn_sock; +} + +/* Finalize release of socket, called after RCU grace period */ +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; +} + +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 + */ + 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 000000000000..0d23de5a9344 --- /dev/null +++ b/drivers/net/ovpn/socket.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_SOCK_H_ +#define _NET_OVPN_SOCK_H_ + +#include <linux/net.h> +#include <linux/kref.h> +#include <linux/ptr_ring.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; +}; + +/** + * ovpn_from_udp_sock - retrieve ovpn instance object from UDP sock + * @sk: the sock to retrieve the instance from + * + * Return: the ovpn instance that this sock is bound to + */ +struct ovpn_struct *ovpn_from_udp_sock(struct sock *sk); + +/** + * ovpn_socket_release_kref - kref_put callback + * @kref: the kref object + */ +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); +} + +/** + * 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); + +#endif /* _NET_OVPN_SOCK_H_ */ diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c new file mode 100644 index 000000000000..4b7d96a13df0 --- /dev/null +++ b/drivers/net/ovpn/udp.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload + * + * Copyright (C) 2019-2024 OpenVPN, Inc. + * + * Author: Antonio Quartulli <antonio@openvpn.net> + */ + +#include <linux/netdevice.h> +#include <linux/socket.h> + +#include "ovpnstruct.h" +#include "main.h" +#include "socket.h" +#include "udp.h" + +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn) +{ + struct ovpn_socket *old_data; + + /* sanity check */ + if (sock->sk->sk_protocol != IPPROTO_UDP) { + netdev_err(ovpn->dev, "%s: expected UDP socket\n", __func__); + return -EINVAL; + } + + /* make sure no pre-existing encapsulation handler exists */ + rcu_read_lock(); + old_data = rcu_dereference_sk_user_data(sock->sk); + rcu_read_unlock(); + if (old_data) { + if (old_data->ovpn == ovpn) { + netdev_dbg(ovpn->dev, + "%s: provided socket already owned by this interface\n", + __func__); + return -EALREADY; + } + + netdev_err(ovpn->dev, "%s: provided socket already taken by other user\n", + __func__); + return -EBUSY; + } + + return 0; +} diff --git a/drivers/net/ovpn/udp.h b/drivers/net/ovpn/udp.h new file mode 100644 index 000000000000..16422a649cb9 --- /dev/null +++ b/drivers/net/ovpn/udp.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2019-2024 OpenVPN, Inc. + * + * Author: Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_UDP_H_ +#define _NET_OVPN_UDP_H_ + +struct ovpn_struct; +struct socket; + +/** + * 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); + +#endif /* _NET_OVPN_UDP_H_ */
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 attathced 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> --- drivers/net/ovpn/Makefile | 2 + drivers/net/ovpn/socket.c | 105 ++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/socket.h | 68 ++++++++++++++++++++++++ drivers/net/ovpn/udp.c | 45 ++++++++++++++++ drivers/net/ovpn/udp.h | 27 ++++++++++ 5 files changed, 247 insertions(+) create mode 100644 drivers/net/ovpn/socket.c create mode 100644 drivers/net/ovpn/socket.h create mode 100644 drivers/net/ovpn/udp.c create mode 100644 drivers/net/ovpn/udp.h