Message-ID: <a7b262c8-5795-44b0-9544-26f12f78e703@openvpn.net>
Date: Tue, 5 Mar 2024 13:29:33 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next v2 02/22] net: introduce OpenVPN Data Channel
 Offload (ovpn)

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@...nvpn.net>
>>>> + *		Antonio Quartulli <antonio@...nvpn.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@...nvpn.net>
>>>> + *		Antonio Quartulli <antonio@...nvpn.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
OpenVPN Inc.

Powered by blists - more mailing lists