Message-ID: <f35c2ec2-ef00-442d-94cd-fa695268c4f2@gmail.com>
Date: Wed, 6 Nov 2024 02:31:33 +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, steffen.klassert@...unet.com,
 antony.antony@...unet.com
Subject: Re: [PATCH net-next v11 02/23] net: introduce OpenVPN Data Channel
 Offload (ovpn)

On 29.10.2024 12:47, Antonio Quartulli wrote:
> OpenVPN is a userspace software existing since around 2005 that allows
> users to create secure tunnels.
> 
> So far OpenVPN has implemented all operations in userspace, which
> implies several back and forth between kernel and user land in order to
> process packets (encapsulate/decapsulate, encrypt/decrypt, rerouting..).
> 
> With `ovpn` we intend to move the fast path (data channel) entirely
> in kernel space and thus improve user measured throughput over the
> tunnel.
> 
> `ovpn` is implemented as a simple virtual network device driver, that
> can be manipulated by means of the standard RTNL APIs. A device of kind
> `ovpn` allows only IPv4/6 traffic and can be of type:
> * P2P (peer-to-peer): any packet sent over the interface will be
>    encapsulated and transmitted to the other side (typical OpenVPN
>    client or peer-to-peer behaviour);
> * P2MP (point-to-multipoint): packets sent over the interface are
>    transmitted to peers based on existing routes (typical OpenVPN
>    server behaviour).
> 
> After the interface has been created, OpenVPN in userspace can
> configure it using a new Netlink API. Specifically it is possible
> to manage peers and their keys.
> 
> The OpenVPN control channel is multiplexed over the same transport
> socket by means of OP codes. Anything that is not DATA_V2 (OpenVPN
> OP code for data traffic) is sent to userspace and handled there.
> This way the `ovpn` codebase is kept as compact as possible while
> focusing on handling data traffic only (fast path).
> 
> Any OpenVPN control feature (like cipher negotiation, TLS handshake,
> rekeying, etc.) is still fully handled by the userspace process.
> 
> When userspace establishes a new connection with a peer, it first
> performs the handshake and then passes the socket to the `ovpn` kernel
> module, which takes ownership. From this moment on `ovpn` will handle
> data traffic for the new peer.
> When control packets are received on the link, they are forwarded to
> userspace through the same transport socket they were received on, as
> userspace is still listening to them.
> 
> Some events (like peer deletion) are sent to a Netlink multicast group.
> 
> Although it wasn't easy to convince the community, `ovpn` implements
> only a limited number of the data-channel features supported by the
> userspace program.
> 
> Each feature that made it to `ovpn` was attentively vetted to
> avoid carrying too much legacy along with us (and to give a clear cut to
> old and probalby-not-so-useful features).
> 
> Notably, only encryption using AEAD ciphers (specifically
> ChaCha20Poly1305 and AES-GCM) was implemented. Supporting any other
> cipher out there was not deemed useful.
> 
> Both UDP and TCP sockets ae supported.

s/ae/are/

> As explained above, in case of P2MP mode, OpenVPN will use the main system
> routing table to decide which packet goes to which peer. This implies
> that no routing table was re-implemented in the `ovpn` kernel module.
> 
> This kernel module can be enabled by selecting the CONFIG_OVPN entry
> in the networking drivers section.

Most of the above text has no relation to the patch itself. Should it be 
moved to the cover letter?

> NOTE: this first patch introduces the very basic framework only.
> Features are then added patch by patch, however, although each patch
> will compile and possibly not break at runtime, only after having
> applied the full set it is expected to see the ovpn module fully working.
> 
> Cc: steffen.klassert@...unet.com
> Cc: antony.antony@...unet.com
> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
> ---
>   MAINTAINERS               |   8 ++++
>   drivers/net/Kconfig       |  13 ++++++
>   drivers/net/Makefile      |   1 +
>   drivers/net/ovpn/Makefile |  11 +++++
>   drivers/net/ovpn/io.c     |  22 +++++++++
>   drivers/net/ovpn/io.h     |  15 ++++++
>   drivers/net/ovpn/main.c   | 116 ++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/net/ovpn/main.h   |  15 ++++++
>   include/uapi/linux/udp.h  |   1 +
>   9 files changed, 202 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f39ab140710f16b1245924bfe381cd64d499ff8a..09e193bbc218d74846cbae26f80ada3e04c3692a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17286,6 +17286,14 @@ F:	arch/openrisc/
>   F:	drivers/irqchip/irq-ompic.c
>   F:	drivers/irqchip/irq-or1k-*
>   
> +OPENVPN DATA CHANNEL OFFLOAD
> +M:	Antonio Quartulli <antonio@...nvpn.net>
> +L:	openvpn-devel@...ts.sourceforge.net (moderated for non-subscribers)
> +L:	netdev@...r.kernel.org
> +S:	Supported
> +T:	git https://github.com/OpenVPN/linux-kernel-ovpn.git
> +F:	drivers/net/ovpn/
> +
>   OPENVSWITCH
>   M:	Pravin B Shelar <pshelar@....org>
>   L:	netdev@...r.kernel.org
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 1fd5acdc73c6af0e1a861867039c3624fc618e25..269b73fcfd348a48174fb96b8f8d4f8788636fa8 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -115,6 +115,19 @@ config WIREGUARD_DEBUG
>   
>   	  Say N here unless you know what you're doing.
>   
> +config OVPN
> +	tristate "OpenVPN data channel offload"
> +	depends on NET && INET
> +	select NET_UDP_TUNNEL
> +	select DST_CACHE
> +	select CRYPTO
> +	select CRYPTO_AES
> +	select CRYPTO_GCM
> +	select CRYPTO_CHACHA20POLY1305

nit: Options from NET_UDP_TUNNEL to CRYPTO_CHACHA20POLY1305 are not 
required for changes introduced in this patch. Should they be moved to 
corresponding patches?

> +	help
> +	  This module enhances the performance of the OpenVPN userspace software
> +	  by offloading the data channel processing to kernelspace.
> +
>   config EQUALIZER
>   	tristate "EQL (serial line load balancing) support"
>   	help
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 13743d0e83b5fde479e9b30ad736be402d880dee..5152b3330e28da7eaec821018a26c973bb33ce0c 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_IPVLAN) += ipvlan/
>   obj-$(CONFIG_IPVTAP) += ipvlan/
>   obj-$(CONFIG_DUMMY) += dummy.o
>   obj-$(CONFIG_WIREGUARD) += wireguard/
> +obj-$(CONFIG_OVPN) += ovpn/
>   obj-$(CONFIG_EQUALIZER) += eql.o
>   obj-$(CONFIG_IFB) += ifb.o
>   obj-$(CONFIG_MACSEC) += macsec.o
> diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..53fb197027d787d6683e9056d3d341abf6ed38e4
> --- /dev/null
> +++ b/drivers/net/ovpn/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# ovpn -- OpenVPN data channel offload in kernel space
> +#
> +# Copyright (C) 2020-2024 OpenVPN, Inc.
> +#
> +# Author:	Antonio Quartulli <antonio@...nvpn.net>
> +
> +obj-$(CONFIG_OVPN) := ovpn.o
> +ovpn-y += main.o
> +ovpn-y += io.o
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ad3813419c33cbdfe7e8ad6f5c8b444a3540a69f
> --- /dev/null
> +++ b/drivers/net/ovpn/io.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@...nvpn.net>
> + *		Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +
> +#include "io.h"
> +
> +/* Send user data to the network
> + */
> +netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	skb_tx_error(skb);
> +	kfree_skb(skb);
> +	return NET_XMIT_DROP;
> +}
> diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..aa259be66441f7b0262f39da12d6c3dce0a9b24c
> --- /dev/null
> +++ b/drivers/net/ovpn/io.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@...nvpn.net>
> + *		Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#ifndef _NET_OVPN_OVPN_H_
> +#define _NET_OVPN_OVPN_H_
> +
> +netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev);
> +
> +#endif /* _NET_OVPN_OVPN_H_ */
> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..369a5a2b2fc1a497c8444e59f9b058eb40e49524
> --- /dev/null
> +++ b/drivers/net/ovpn/main.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2020-2024 OpenVPN, Inc.
> + *
> + *  Author:	Antonio Quartulli <antonio@...nvpn.net>
> + *		James Yonan <james@...nvpn.net>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <net/rtnetlink.h>
> +
> +#include "main.h"
> +#include "io.h"
> +
> +/* Driver info */
> +#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
> +#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."

nit: these strings are used only once for MODULE_{DESCRIPTION,AUTHOR} 
below. Can we directly use strings to avoid levels of indirection?

> +
> +/**
> + * ovpn_dev_is_valid - check if the netdevice is of type 'ovpn'
> + * @dev: the interface to check
> + *
> + * Return: whether the netdevice is of type 'ovpn'
> + */
> +bool ovpn_dev_is_valid(const struct net_device *dev)
> +{
> +	return dev->netdev_ops->ndo_start_xmit == ovpn_net_xmit;

You can directly check for the ops matching saving one dereferencing 
operation:

return dev->netdev_ops == &ovpn_netdev_ops;

You can define an empty ovpn_netdev_ops struct for this purpose in this 
patch and fill ops later with next patches. This way you can even move 
the ovpn_net_xmit() definition to the interface creation/destruction patch.

> +}
> +
> +static int ovpn_newlink(struct net *src_net, struct net_device *dev,
> +			struct nlattr *tb[], struct nlattr *data[],
> +			struct netlink_ext_ack *extack)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct rtnl_link_ops ovpn_link_ops = {
> +	.kind = "ovpn",
> +	.netns_refund = false,
> +	.newlink = ovpn_newlink,
> +	.dellink = unregister_netdevice_queue,
> +};
> +
> +static int ovpn_netdev_notifier_call(struct notifier_block *nb,
> +				     unsigned long state, void *ptr)
> +{
> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +
> +	if (!ovpn_dev_is_valid(dev))
> +		return NOTIFY_DONE;
> +
> +	switch (state) {
> +	case NETDEV_REGISTER:
> +		/* add device to internal list for later destruction upon
> +		 * unregistration
> +		 */
> +		break;
> +	case NETDEV_UNREGISTER:
> +		/* can be delivered multiple times, so check registered flag,
> +		 * then destroy the interface
> +		 */
> +		break;
> +	case NETDEV_POST_INIT:
> +	case NETDEV_GOING_DOWN:
> +	case NETDEV_DOWN:
> +	case NETDEV_UP:
> +	case NETDEV_PRE_UP:
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block ovpn_netdev_notifier = {
> +	.notifier_call = ovpn_netdev_notifier_call,
> +};
> +
> +static int __init ovpn_init(void)
> +{
> +	int err = register_netdevice_notifier(&ovpn_netdev_notifier);
> +
> +	if (err) {
> +		pr_err("ovpn: can't register netdevice notifier: %d\n", err);
> +		return err;
> +	}
> +
> +	err = rtnl_link_register(&ovpn_link_ops);
> +	if (err) {
> +		pr_err("ovpn: can't register rtnl link ops: %d\n", err);
> +		goto unreg_netdev;
> +	}
> +
> +	return 0;
> +
> +unreg_netdev:
> +	unregister_netdevice_notifier(&ovpn_netdev_notifier);
> +	return err;
> +}
> +
> +static __exit void ovpn_cleanup(void)
> +{
> +	rtnl_link_unregister(&ovpn_link_ops);
> +	unregister_netdevice_notifier(&ovpn_netdev_notifier);
> +
> +	rcu_barrier();
> +}
> +
> +module_init(ovpn_init);
> +module_exit(ovpn_cleanup);
> +
> +MODULE_DESCRIPTION(DRV_DESCRIPTION);
> +MODULE_AUTHOR(DRV_COPYRIGHT);
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..a3215316c49bfcdf2496590bac878f145b8b27fd
> --- /dev/null
> +++ b/drivers/net/ovpn/main.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@...nvpn.net>
> + *		Antonio Quartulli <antonio@...nvpn.net>
> + */
> +
> +#ifndef _NET_OVPN_MAIN_H_
> +#define _NET_OVPN_MAIN_H_
> +
> +bool ovpn_dev_is_valid(const struct net_device *dev);
> +
> +#endif /* _NET_OVPN_MAIN_H_ */
> diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
> index d85d671deed3c78f6969189281b9083dcac000c6..edca3e430305a6bffc34e617421f1f3071582e69 100644
> --- a/include/uapi/linux/udp.h
> +++ b/include/uapi/linux/udp.h
> @@ -43,5 +43,6 @@ struct udphdr {
>   #define UDP_ENCAP_GTP1U		5 /* 3GPP TS 29.060 */
>   #define UDP_ENCAP_RXRPC		6
>   #define TCP_ENCAP_ESPINTCP	7 /* Yikes, this is really xfrm encap types. */
> +#define UDP_ENCAP_OVPNINUDP	8 /* OpenVPN traffic */

nit: this specific change does not belong to this specific patch.

>   
>   #endif /* _UAPI_LINUX_UDP_H */
> 

Powered by blists - more mailing lists