diff mbox series

[net-next,v2,02/22] net: introduce OpenVPN Data Channel Offload (ovpn)

Message ID 20240304150914.11444-3-antonio@openvpn.net (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introducing OpenVPN Data Channel Offload | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 2805 this patch: 2806
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: openvpn-devel@lists.sourceforge.net
netdev/build_clang fail Errors and warnings before: 999 this patch: 1001
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 3019 this patch: 3020
netdev/checkpatch warning CHECK: Please don't use multiple blank lines WARNING: line length of 82 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Antonio Quartulli March 4, 2024, 3:08 p.m. UTC
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.

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.

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.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 MAINTAINERS               |   8 +++
 drivers/net/Kconfig       |  13 +++++
 drivers/net/Makefile      |   1 +
 drivers/net/ovpn/Makefile |  11 ++++
 drivers/net/ovpn/io.c     |  23 ++++++++
 drivers/net/ovpn/io.h     |  19 ++++++
 drivers/net/ovpn/main.c   | 118 ++++++++++++++++++++++++++++++++++++++
 drivers/net/ovpn/main.h   |  38 ++++++++++++
 include/uapi/linux/udp.h  |   1 +
 9 files changed, 232 insertions(+)
 create mode 100644 drivers/net/ovpn/Makefile
 create mode 100644 drivers/net/ovpn/io.c
 create mode 100644 drivers/net/ovpn/io.h
 create mode 100644 drivers/net/ovpn/main.c
 create mode 100644 drivers/net/ovpn/main.h

Comments

Andrew Lunn March 4, 2024, 8:47 p.m. UTC | #1
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> new file mode 100644
> index 000000000000..a1e19402e36d
> --- /dev/null
> +++ b/drivers/net/ovpn/io.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@openvpn.net>
> + *		Antonio Quartulli <antonio@openvpn.net>
> + */
> +
> +#include "io.h"
> +
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>

It is normal to put local headers last.

> diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h
> new file mode 100644
> index 000000000000..0a076d14f721
> --- /dev/null
> +++ b/drivers/net/ovpn/io.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* OpenVPN data channel offload
> + *
> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> + *
> + *  Author:	James Yonan <james@openvpn.net>
> + *		Antonio Quartulli <antonio@openvpn.net>
> + */
> +
> +#ifndef _NET_OVPN_OVPN_H_
> +#define _NET_OVPN_OVPN_H_
> +
> +#include <linux/netdevice.h>
> +
> +struct sk_buff;
> +

Once you have the headers in the normal order, you probably won't need
this.

> +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 000000000000..25964eb89aac
> --- /dev/null
> +++ b/drivers/net/ovpn/main.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  OpenVPN data channel offload
> + *
> + *  Copyright (C) 2020-2024 OpenVPN, Inc.
> + *
> + *  Author:	Antonio Quartulli <antonio@openvpn.net>
> + *		James Yonan <james@openvpn.net>
> + */
> +
> +#include "main.h"
> +#include "io.h"
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/types.h>
> +#include <linux/net.h>
> +#include <linux/inetdevice.h>
> +#include <linux/netdevice.h>
> +#include <linux/version.h>
> +
> +
> +/* Driver info */

Double blank lines are generally not liked. I'm surprised checkpatch
did not warn?

> +#define DRV_NAME	"ovpn"
> +#define DRV_VERSION	OVPN_VERSION
> +#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
> +#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."
> +
> +/* Net device open */
> +static int ovpn_net_open(struct net_device *dev)
> +{
> +	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
> +
> +	if (dev_v4) {
> +		/* disable redirects as Linux gets confused by ovpn handling same-LAN routing */

Although Linux in general allows longer lines, netdev has kept with
80. Please wrap.

> +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
> +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;

Wireguard has the same. How is Linux getting confused? Maybe we should
consider fixing this properly?

> +#ifndef OVPN_VERSION
> +#define OVPN_VERSION "3.0.0"
> +#endif

What could sensible define it to some other value?

These version numbers are generally useless. A driver is not
standalone. It fits within a kernel. If you get a bug report, what you
actually want to know is the kernel version, ideally the git hash.

    Andrew
Antonio Quartulli March 4, 2024, 9:30 p.m. UTC | #2
Hi Andrew,

On 04/03/2024 21:47, Andrew Lunn wrote:
>> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
>> new file mode 100644
>> index 000000000000..a1e19402e36d
>> --- /dev/null
>> +++ b/drivers/net/ovpn/io.c
>> @@ -0,0 +1,23 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*  OpenVPN data channel offload
>> + *
>> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
>> + *
>> + *  Author:	James Yonan <james@openvpn.net>
>> + *		Antonio Quartulli <antonio@openvpn.net>
>> + */
>> +
>> +#include "io.h"
>> +
>> +#include <linux/netdevice.h>
>> +#include <linux/skbuff.h>
> 
> It is normal to put local headers last.

Ok, will make this change on all files.

> 
>> diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h
>> new file mode 100644
>> index 000000000000..0a076d14f721
>> --- /dev/null
>> +++ b/drivers/net/ovpn/io.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* OpenVPN data channel offload
>> + *
>> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
>> + *
>> + *  Author:	James Yonan <james@openvpn.net>
>> + *		Antonio Quartulli <antonio@openvpn.net>
>> + */
>> +
>> +#ifndef _NET_OVPN_OVPN_H_
>> +#define _NET_OVPN_OVPN_H_
>> +
>> +#include <linux/netdevice.h>
>> +
>> +struct sk_buff;
>> +
> 
> Once you have the headers in the normal order, you probably won't need
> this.

True, but I personally I always try to include headers in any file where 
they are needed, to avoid implicitly forcing some kind of include 
ordering or dependency. Isn't it recommended?

> 
>> +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 000000000000..25964eb89aac
>> --- /dev/null
>> +++ b/drivers/net/ovpn/main.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*  OpenVPN data channel offload
>> + *
>> + *  Copyright (C) 2020-2024 OpenVPN, Inc.
>> + *
>> + *  Author:	Antonio Quartulli <antonio@openvpn.net>
>> + *		James Yonan <james@openvpn.net>
>> + */
>> +
>> +#include "main.h"
>> +#include "io.h"
>> +
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/types.h>
>> +#include <linux/net.h>
>> +#include <linux/inetdevice.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/version.h>
>> +
>> +
>> +/* Driver info */
> 
> Double blank lines are generally not liked. I'm surprised checkpatch
> did not warn?

No, it did not complain. I added an extra white line between headers and 
code, but I can remove it and avoid double blank lines at all.

> 
>> +#define DRV_NAME	"ovpn"
>> +#define DRV_VERSION	OVPN_VERSION
>> +#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
>> +#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."
>> +
>> +/* Net device open */
>> +static int ovpn_net_open(struct net_device *dev)
>> +{
>> +	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
>> +
>> +	if (dev_v4) {
>> +		/* disable redirects as Linux gets confused by ovpn handling same-LAN routing */
> 
> Although Linux in general allows longer lines, netdev has kept with
> 80. Please wrap.

Oh ok. I thought the line length was relaxed kernel-wide.
Will wrap all lines as needed then.

> 
>> +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
>> +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
> 
> Wireguard has the same. How is Linux getting confused? Maybe we should
> consider fixing this properly?
> 
>> +#ifndef OVPN_VERSION
>> +#define OVPN_VERSION "3.0.0"
>> +#endif
> 
> What could sensible define it to some other value?
> 
> These version numbers are generally useless. A driver is not
> standalone. It fits within a kernel. If you get a bug report, what you
> actually want to know is the kernel version, ideally the git hash.

True, unless the kernel module was compiled as out-of-tree or manually 
(back-)ported to a different kernel. In that case I'd need the exact 
version to know what the reporter was running. Right?

Although, while porting to another kernel ovpn could always reference 
its original kernel as its own version.

I.e.: ovpn-6.9.0 built for linux-4.4.1

Does it make sense?
How do other drivers deal with this?
For example batman-adv has its own:

#define BATADV_SOURCE_VERSION "2024.1"

It helps when compiling the code out of tree.

Regards,


> 
>      Andrew
Andrew Lunn March 4, 2024, 10:46 p.m. UTC | #3
On Mon, Mar 04, 2024 at 10:30:53PM +0100, Antonio Quartulli wrote:
> Hi Andrew,
> 
> On 04/03/2024 21:47, Andrew Lunn wrote:
> > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> > > new file mode 100644
> > > index 000000000000..a1e19402e36d
> > > --- /dev/null
> > > +++ b/drivers/net/ovpn/io.c
> > > @@ -0,0 +1,23 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*  OpenVPN data channel offload
> > > + *
> > > + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> > > + *
> > > + *  Author:	James Yonan <james@openvpn.net>
> > > + *		Antonio Quartulli <antonio@openvpn.net>
> > > + */
> > > +
> > > +#include "io.h"
> > > +
> > > +#include <linux/netdevice.h>
> > > +#include <linux/skbuff.h>
> > 
> > It is normal to put local headers last.
> 
> Ok, will make this change on all files.
> 
> > 
> > > diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h
> > > new file mode 100644
> > > index 000000000000..0a076d14f721
> > > --- /dev/null
> > > +++ b/drivers/net/ovpn/io.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/* OpenVPN data channel offload
> > > + *
> > > + *  Copyright (C) 2019-2024 OpenVPN, Inc.
> > > + *
> > > + *  Author:	James Yonan <james@openvpn.net>
> > > + *		Antonio Quartulli <antonio@openvpn.net>
> > > + */
> > > +
> > > +#ifndef _NET_OVPN_OVPN_H_
> > > +#define _NET_OVPN_OVPN_H_
> > > +
> > > +#include <linux/netdevice.h>
> > > +
> > > +struct sk_buff;
> > > +
> > 
> > Once you have the headers in the normal order, you probably won't need
> > this.
> 
> True, but I personally I always try to include headers in any file where
> they are needed, to avoid implicitly forcing some kind of include ordering
> or dependency. Isn't it recommended?

It is a bit of a balancing act. There is a massive patch series
crossing the entire kernel which significantly reduces the kernel
build time by optimising includes. It only includes what is needed,
and it breaks up some of the big header files. The compiler spends a
significant time processing include files. So don't include what you
don't need, and try at avoid including the same header multiple times.

> > > +/* Driver info */
> > 
> > Double blank lines are generally not liked. I'm surprised checkpatch
> > did not warn?
> 
> No, it did not complain. I added an extra white line between headers and
> code, but I can remove it and avoid double blank lines at all.
> 
> > 
> > > +#define DRV_NAME	"ovpn"
> > > +#define DRV_VERSION	OVPN_VERSION
> > > +#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
> > > +#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."
> > > +
> > > +/* Net device open */
> > > +static int ovpn_net_open(struct net_device *dev)
> > > +{
> > > +	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
> > > +
> > > +	if (dev_v4) {
> > > +		/* disable redirects as Linux gets confused by ovpn handling same-LAN routing */
> > 
> > Although Linux in general allows longer lines, netdev has kept with
> > 80. Please wrap.
> 
> Oh ok. I thought the line length was relaxed kernel-wide.
> Will wrap all lines as needed then.

https://patchwork.kernel.org/project/netdevbpf/patch/20240304150914.11444-3-antonio@openvpn.net/

Notice the netdev/checkpatch test:

CHECK: Please don't use multiple blank lines WARNING: line length of
82 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
WARNING: line length of 96 exceeds 80 columns

There are some other test failures you should look at.

> > 
> > > +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
> > > +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
> > 
> > Wireguard has the same. How is Linux getting confused? Maybe we should
> > consider fixing this properly?
> > 
> > > +#ifndef OVPN_VERSION
> > > +#define OVPN_VERSION "3.0.0"
> > > +#endif
> > 
> > What could sensible define it to some other value?
> > 
> > These version numbers are generally useless. A driver is not
> > standalone. It fits within a kernel. If you get a bug report, what you
> > actually want to know is the kernel version, ideally the git hash.
> 
> True, unless the kernel module was compiled as out-of-tree or manually
> (back-)ported to a different kernel. In that case I'd need the exact version
> to know what the reporter was running. Right?

With my mainline hat on: You don't compile an in tree module out of
tree.

> Although, while porting to another kernel ovpn could always reference its
> original kernel as its own version.
> 
> I.e.: ovpn-6.9.0 built for linux-4.4.1
> 
> Does it make sense?
> How do other drivers deal with this?

$ ethtool -i enp2s0
[sudo] password for andrew: 
driver: r8169
version: 6.6.9-amd64

It reports uname -r. This is what my Debian kernel calls itself. And a
hand built kernel should have a git hash. A Redhat kernel probably has
something which identifies it as Redhat. So if somebody backports it
to a distribution Frankenkernel, you should be able to identify what
the kernel is.

We tell driver writes to implement ethtool .get_drvinfo, and leave
ethtool_drvinfo.version empty. The ethtool core will then fill it with
uname -r. That should identify the kernel the driver is running in.

There is no reason a virtual device should not implement ethtool.

BATMAN is a bit schizophrenic, both in tree and out of tree. I can
understand that for something like BATMAN which is quite niche. But my
guess would be, OpenVPN is big enough that vendors will do the
backport, to their Frankenkernel, you don't need to keep an out of
tree version as well as the in tree version.

      Andrew
Antonio Quartulli March 5, 2024, 12:29 p.m. UTC | #4
On 04/03/2024 23:46, Andrew Lunn wrote:
> On Mon, Mar 04, 2024 at 10:30:53PM +0100, Antonio Quartulli wrote:
>> Hi Andrew,
>>
>> On 04/03/2024 21:47, Andrew Lunn wrote:
>>>> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
>>>> new file mode 100644
>>>> index 000000000000..a1e19402e36d
>>>> --- /dev/null
>>>> +++ b/drivers/net/ovpn/io.c
>>>> @@ -0,0 +1,23 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*  OpenVPN data channel offload
>>>> + *
>>>> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
>>>> + *
>>>> + *  Author:	James Yonan <james@openvpn.net>
>>>> + *		Antonio Quartulli <antonio@openvpn.net>
>>>> + */
>>>> +
>>>> +#include "io.h"
>>>> +
>>>> +#include <linux/netdevice.h>
>>>> +#include <linux/skbuff.h>
>>>
>>> It is normal to put local headers last.
>>
>> Ok, will make this change on all files.
>>
>>>
>>>> diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h
>>>> new file mode 100644
>>>> index 000000000000..0a076d14f721
>>>> --- /dev/null
>>>> +++ b/drivers/net/ovpn/io.h
>>>> @@ -0,0 +1,19 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/* OpenVPN data channel offload
>>>> + *
>>>> + *  Copyright (C) 2019-2024 OpenVPN, Inc.
>>>> + *
>>>> + *  Author:	James Yonan <james@openvpn.net>
>>>> + *		Antonio Quartulli <antonio@openvpn.net>
>>>> + */
>>>> +
>>>> +#ifndef _NET_OVPN_OVPN_H_
>>>> +#define _NET_OVPN_OVPN_H_
>>>> +
>>>> +#include <linux/netdevice.h>
>>>> +
>>>> +struct sk_buff;
>>>> +
>>>
>>> Once you have the headers in the normal order, you probably won't need
>>> this.
>>
>> True, but I personally I always try to include headers in any file where
>> they are needed, to avoid implicitly forcing some kind of include ordering
>> or dependency. Isn't it recommended?
> 
> It is a bit of a balancing act. There is a massive patch series
> crossing the entire kernel which significantly reduces the kernel
> build time by optimising includes. It only includes what is needed,
> and it breaks up some of the big header files. The compiler spends a
> significant time processing include files. So don't include what you
> don't need, and try at avoid including the same header multiple times.

ACK

>>>> +#define DRV_NAME	"ovpn"
>>>> +#define DRV_VERSION	OVPN_VERSION
>>>> +#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
>>>> +#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."
>>>> +
>>>> +/* Net device open */
>>>> +static int ovpn_net_open(struct net_device *dev)
>>>> +{
>>>> +	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
>>>> +
>>>> +	if (dev_v4) {
>>>> +		/* disable redirects as Linux gets confused by ovpn handling same-LAN routing */
>>>
>>> Although Linux in general allows longer lines, netdev has kept with
>>> 80. Please wrap.
>>
>> Oh ok. I thought the line length was relaxed kernel-wide.
>> Will wrap all lines as needed then.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20240304150914.11444-3-antonio@openvpn.net/
> 
> Notice the netdev/checkpatch test:
> 
> CHECK: Please don't use multiple blank lines WARNING: line length of
> 82 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
> WARNING: line length of 96 exceeds 80 columns
> 
> There are some other test failures you should look at.

Now that I think about it, I did not run checkpatch with --strict, so I 
must have missed some warnings/messages.

Will double check. thanks.

> 
>>>
>>>> +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
>>>> +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
>>>
>>> Wireguard has the same. How is Linux getting confused? Maybe we should
>>> consider fixing this properly?
>>>
>>>> +#ifndef OVPN_VERSION
>>>> +#define OVPN_VERSION "3.0.0"
>>>> +#endif
>>>
>>> What could sensible define it to some other value?
>>>
>>> These version numbers are generally useless. A driver is not
>>> standalone. It fits within a kernel. If you get a bug report, what you
>>> actually want to know is the kernel version, ideally the git hash.
>>
>> True, unless the kernel module was compiled as out-of-tree or manually
>> (back-)ported to a different kernel. In that case I'd need the exact version
>> to know what the reporter was running. Right?
> 
> With my mainline hat on: You don't compile an in tree module out of
> tree.
> 
>> Although, while porting to another kernel ovpn could always reference its
>> original kernel as its own version.
>>
>> I.e.: ovpn-6.9.0 built for linux-4.4.1
>>
>> Does it make sense?
>> How do other drivers deal with this?
> 
> $ ethtool -i enp2s0
> [sudo] password for andrew:
> driver: r8169
> version: 6.6.9-amd64
> 
> It reports uname -r. This is what my Debian kernel calls itself. And a
> hand built kernel should have a git hash. A Redhat kernel probably has
> something which identifies it as Redhat. So if somebody backports it
> to a distribution Frankenkernel, you should be able to identify what
> the kernel is.
> 
> We tell driver writes to implement ethtool .get_drvinfo, and leave
> ethtool_drvinfo.version empty. The ethtool core will then fill it with
> uname -r. That should identify the kernel the driver is running in.
> 
> There is no reason a virtual device should not implement ethtool.
> 
> BATMAN is a bit schizophrenic, both in tree and out of tree. I can
> understand that for something like BATMAN which is quite niche. But my
> guess would be, OpenVPN is big enough that vendors will do the
> backport, to their Frankenkernel, you don't need to keep an out of
> tree version as well as the in tree version.

I think the common usecase with batman-adv is OpenWrt: like batman-adv, 
also OpenVPN is widely used on small routers/gateways. It is convenient 
for distros like OpenWRT to be able to compile out-of-tree modules that 
are more recent than the kernel being shipped with the stable release.

Wifi drivers are also part of this roller-coaster, but they go through 
the "backports" project[1].

Maybe I should look into hooking in "backports" as well - it may give us 
what we need without requiring an out-of-tree package.

I guess I'll drop the internal version for now.

Regards,


[1] https://backports.wiki.kernel.org/index.php/Main_Page

> 
>        Andrew
Antonio Quartulli March 6, 2024, 3:51 p.m. UTC | #5
On 04/03/2024 21:47, Andrew Lunn wrote:
>> +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
>> +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
> 
> Wireguard has the same. How is Linux getting confused? Maybe we should
> consider fixing this properly?
> 

I wanted to reply to this point individually.

The reason for requiring this setting lies in the OpenVPN server acting 
as relay point for hosts in the same subnet.

Example: given the a.b.c.0/24 IP network, you will have .2 that in order 
to talk to .3 must have its traffic relayed by .1 (the server).

When the kernel sees this traffic it will send the ICMP redirects, 
because it believes that .2 should directly talk to .3 without passing 
through .1.

It of course makes sense in a normal network with a classic broadcast 
domain, but this is not the case in a VPN implemented as a start topology.

Does it make sense?

The only way I see to fix this globally is to have an extra flag in the 
netdevice signaling this peculiarity and thus disabling ICMP redirects 
automatically.

Regards,
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 04e5f7c20e30..5ded5e931622 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16529,6 +16529,14 @@  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
 F:	Documentation/filesystems/overlayfs.rst
 F:	fs/overlayfs/
 
+OPENVPN DATA CHANNEL OFFLOAD
+M:	Antonio Quartulli <antonio@openvpn.net>
+L:	openvpn-devel@lists.sourceforge.net (moderated for non-subscribers)
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/ovpn/
+F:	include/uapi/linux/ovpn.h
+
 P54 WIRELESS DRIVER
 M:	Christian Lamparter <chunkeey@googlemail.com>
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 8ca0bc223b30..66ddfd758a40 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
+	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 7cab36f94782..a0b33e7c29ad 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 000000000000..7b8b1d0ff9b4
--- /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@openvpn.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 000000000000..a1e19402e36d
--- /dev/null
+++ b/drivers/net/ovpn/io.c
@@ -0,0 +1,23 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*  OpenVPN data channel offload
+ *
+ *  Copyright (C) 2019-2024 OpenVPN, Inc.
+ *
+ *  Author:	James Yonan <james@openvpn.net>
+ *		Antonio Quartulli <antonio@openvpn.net>
+ */
+
+#include "io.h"
+
+#include <linux/netdevice.h>
+#include <linux/skbuff.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 000000000000..0a076d14f721
--- /dev/null
+++ b/drivers/net/ovpn/io.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* OpenVPN data channel offload
+ *
+ *  Copyright (C) 2019-2024 OpenVPN, Inc.
+ *
+ *  Author:	James Yonan <james@openvpn.net>
+ *		Antonio Quartulli <antonio@openvpn.net>
+ */
+
+#ifndef _NET_OVPN_OVPN_H_
+#define _NET_OVPN_OVPN_H_
+
+#include <linux/netdevice.h>
+
+struct sk_buff;
+
+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 000000000000..25964eb89aac
--- /dev/null
+++ b/drivers/net/ovpn/main.c
@@ -0,0 +1,118 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*  OpenVPN data channel offload
+ *
+ *  Copyright (C) 2020-2024 OpenVPN, Inc.
+ *
+ *  Author:	Antonio Quartulli <antonio@openvpn.net>
+ *		James Yonan <james@openvpn.net>
+ */
+
+#include "main.h"
+#include "io.h"
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/net.h>
+#include <linux/inetdevice.h>
+#include <linux/netdevice.h>
+#include <linux/version.h>
+
+
+/* Driver info */
+#define DRV_NAME	"ovpn"
+#define DRV_VERSION	OVPN_VERSION
+#define DRV_DESCRIPTION	"OpenVPN data channel offload (ovpn)"
+#define DRV_COPYRIGHT	"(C) 2020-2024 OpenVPN, Inc."
+
+/* Net device open */
+static int ovpn_net_open(struct net_device *dev)
+{
+	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
+
+	if (dev_v4) {
+		/* disable redirects as Linux gets confused by ovpn handling same-LAN routing */
+		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
+		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;
+	}
+
+	netif_tx_start_all_queues(dev);
+	return 0;
+}
+
+/* Net device stop -- called prior to device unload */
+static int ovpn_net_stop(struct net_device *dev)
+{
+	netif_tx_stop_all_queues(dev);
+	return 0;
+}
+
+bool ovpn_dev_is_valid(const struct net_device *dev)
+{
+	return dev->netdev_ops->ndo_start_xmit == ovpn_net_xmit;
+}
+
+static const struct net_device_ops ovpn_netdev_ops = {
+	.ndo_open		= ovpn_net_open,
+	.ndo_stop		= ovpn_net_stop,
+	.ndo_start_xmit		= ovpn_net_xmit,
+	.ndo_get_stats64        = dev_get_tstats64,
+};
+
+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;
+	}
+
+	return 0;
+}
+
+static __exit void ovpn_cleanup(void)
+{
+	unregister_netdevice_notifier(&ovpn_netdev_notifier);
+}
+
+module_init(ovpn_init);
+module_exit(ovpn_cleanup);
+
+MODULE_DESCRIPTION(DRV_DESCRIPTION);
+MODULE_AUTHOR(DRV_COPYRIGHT);
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h
new file mode 100644
index 000000000000..cc624116e2f9
--- /dev/null
+++ b/drivers/net/ovpn/main.h
@@ -0,0 +1,38 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*  OpenVPN data channel offload
+ *
+ *  Copyright (C) 2019-2024 OpenVPN, Inc.
+ *
+ *  Author:	James Yonan <james@openvpn.net>
+ *		Antonio Quartulli <antonio@openvpn.net>
+ */
+
+#ifndef _NET_OVPN_MAIN_H_
+#define _NET_OVPN_MAIN_H_
+
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/printk.h>
+#include <linux/udp.h>
+
+#ifndef OVPN_VERSION
+#define OVPN_VERSION "3.0.0"
+#endif
+
+struct net_device;
+struct ovpn_struct;
+enum ovpn_mode;
+
+bool ovpn_dev_is_valid(const struct net_device *dev);
+int ovpn_iface_create(const char *name, enum ovpn_mode mode, struct net *net);
+void ovpn_iface_destruct(struct ovpn_struct *ovpn, bool unregister_device);
+
+#define SKB_HEADER_LEN                                       \
+	(max(sizeof(struct iphdr), sizeof(struct ipv6hdr)) + \
+	 sizeof(struct udphdr) + NET_SKB_PAD)
+
+#define OVPN_HEAD_ROOM ALIGN(16 + SKB_HEADER_LEN, 4)
+#define OVPN_MAX_PADDING 16
+#define OVPN_QUEUE_LEN 1024
+
+#endif /* _NET_OVPN_MAIN_H_ */
diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
index 4828794efcf8..0dd94757127f 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 */
 
 #endif /* _UAPI_LINUX_UDP_H */