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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Mon, 4 Mar 2024 21:47:08 +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)

> 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.

> 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.

> +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?

> +#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