Message-ID: <07050ffc-aa8e-417a-b35b-0cf627fc226f@lunn.ch>
Date: Mon, 4 Mar 2024 23:46:17 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Antonio Quartulli <antonio@...nvpn.net>
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 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.

> > > +/* 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

Powered by blists - more mailing lists