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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <517236bb-fd03-43cb-a264-c7d191058eef@openvpn.net>
Date: Mon, 4 Mar 2024 22:30:53 +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)

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?

> 
>> +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@...nvpn.net>
>> + *		James Yonan <james@...nvpn.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

-- 
Antonio Quartulli
OpenVPN Inc.