Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
From: Antonio Quartulli
Date: Fri Nov 15 2024 - 08:00:20 EST
On 09/11/2024 02:01, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
Add basic infrastructure for handling ovpn interfaces.
Signed-off-by: Antonio Quartulli <antonio@xxxxxxxxxxx>
---
drivers/net/ovpn/main.c | 115 ++++++++++++++++++++++++++++++++
++++++++--
drivers/net/ovpn/main.h | 7 +++
drivers/net/ovpn/ovpnstruct.h | 8 +++
drivers/net/ovpn/packet.h | 40 +++++++++++++++
include/uapi/linux/if_link.h | 15 ++++++
5 files changed, 180 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index
d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101..eead7677b8239eb3c48bb26ca95492d88512b8d4 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -10,18 +10,52 @@
#include <linux/genetlink.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/inetdevice.h>
+#include <net/ip.h>
#include <net/rtnetlink.h>
-#include <uapi/linux/ovpn.h>
+#include <uapi/linux/if_arp.h>
#include "ovpnstruct.h"
#include "main.h"
#include "netlink.h"
#include "io.h"
+#include "packet.h"
/* Driver info */
#define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)"
#define DRV_COPYRIGHT "(C) 2020-2024 OpenVPN, Inc."
+static void ovpn_struct_free(struct net_device *net)
+{
+}
nit: since this handler is not mandatory, its introduction can be moved
to the later patch, which actually fills it with meaningful operations.
ehmm sure I will move it
+static int ovpn_net_open(struct net_device *dev)
+{
+ netif_tx_start_all_queues(dev);
+ return 0;
+}
+
+static int ovpn_net_stop(struct net_device *dev)
+{
+ netif_tx_stop_all_queues(dev);
+ return 0;
+}
+
+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,
+};
+
+static const struct device_type ovpn_type = {
+ .name = OVPN_FAMILY_NAME,
nit: same question here regarding name deriviation. Are you sure that
the device type name is the same as the GENL family name?
Like I said in the previous patch, I want all representative strings to
be "ovpn", that is already the netlink family name.
But I can create another constant to document this explicitly.
+};
+
+static const struct nla_policy ovpn_policy[IFLA_OVPN_MAX + 1] = {
+ [IFLA_OVPN_MODE] = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_P2P,
+ OVPN_MODE_MP),
+};
+
/**
* ovpn_dev_is_valid - check if the netdevice is of type 'ovpn'
* @dev: the interface to check
@@ -33,16 +67,76 @@ bool ovpn_dev_is_valid(const struct net_device *dev)
return dev->netdev_ops->ndo_start_xmit == ovpn_net_xmit;
}
+static void ovpn_setup(struct net_device *dev)
+{
+ /* compute the overhead considering AEAD encryption */
+ const int overhead = sizeof(u32) + NONCE_WIRE_SIZE + 16 +
Where are these magic sizeof(u32) and '16' came from?
It's in the "nice diagram" you commented later in this patch :-)
But I can extend the comment.
[...]
@@ -51,26 +145,37 @@ 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);
+ struct ovpn_struct *ovpn;
if (!ovpn_dev_is_valid(dev))
return NOTIFY_DONE;
+ ovpn = netdev_priv(dev);
nit: netdev_priv() returns only a pointer, it is safe to fetch the
pointer in advance, but do not dereference it until we are sure that an
event references the desired interface type. Takin this into
consideration, the assignment of private data pointer can be moved above
to the variable declaration. Just to make code couple of lines shorter.
I do it here because it seems more "logically correct" to retrieve the
priv pointer after having confirmed that this is a ovpn interface with
ovpn_dev_is_valid().
Moving it above kinda says "I already know there is a ovpn object here",
but this is not the case until after the valid() check. So I prefer to
keep it here.
[...]
--- a/drivers/net/ovpn/main.h
+++ b/drivers/net/ovpn/main.h
@@ -12,4 +12,11 @@
bool ovpn_dev_is_valid(const struct net_device *dev);
+#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)
Where is this magic '16' came from?
should be the same 16 af the over head above (it's the auth tag len)
Will make this more explicit with a comment.
+#define OVPN_MAX_PADDING 16
+
#endif /* _NET_OVPN_MAIN_H_ */
diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/
ovpnstruct.h
index
e3e4df6418b081436378fc51d98db5bd7b5d1fbe..211df871538d34fdff90d182f21a0b0fb11b28ad 100644
--- a/drivers/net/ovpn/ovpnstruct.h
+++ b/drivers/net/ovpn/ovpnstruct.h
@@ -11,15 +11,23 @@
#define _NET_OVPN_OVPNSTRUCT_H_
#include <net/net_trackers.h>
+#include <uapi/linux/if_link.h>
+#include <uapi/linux/ovpn.h>
/**
* struct ovpn_struct - per ovpn interface state
* @dev: the actual netdev representing the tunnel
* @dev_tracker: reference tracker for associated dev
+ * @registered: whether dev is still registered with netdev or not
+ * @mode: device operation mode (i.e. p2p, mp, ..)
+ * @dev_list: entry for the module wide device list
*/
struct ovpn_struct {
struct net_device *dev;
netdevice_tracker dev_tracker;
+ bool registered;
+ enum ovpn_mode mode;
+ struct list_head dev_list;
dev_list is no more used and should be deleted.
ACK
[...]
+
+/* OpenVPN nonce size */
+#define NONCE_SIZE 12
nit: is using the common 'OVPN_' prefix here and for other constants any
good idea? E.g. OVPN_NONCE_SIZE. It can give some hints where it comes
from for a code reader.
ACK
And another one question. Could you clarify in the comment to this
constant where it came from? AFAIU, these 12 bytes is the expectation of
the nonce size of AEAD crypto protocol, rigth?
Correct: 12bytes/96bits. Will extend the comment.
+
+/* OpenVPN nonce size reduced by 8-byte nonce tail -- this is the
+ * size of the AEAD Associated Data (AD) sent over the wire
+ * and is normally the head of the IV
+ */
+#define NONCE_WIRE_SIZE (NONCE_SIZE - sizeof(struct ovpn_nonce_tail))
If the headers and IV are defined as structures, we no more need this
constant since the header construction will be done by a compiler
according to the structure layout.
yap yap. Will do this later as explained in the other email.
+/* Last 8 bytes of AEAD nonce
+ * Provided by userspace and usually derived from
+ * key material generated during TLS handshake
+ */
+struct ovpn_nonce_tail {
+ u8 u8[OVPN_NONCE_TAIL_SIZE];
+};
Why do you need a dadicated structure for this array? Can we declare the
corresponding fields like this:
u8 nonce_tail_xmit[OVPN_NONCE_TAIL_SIZE];
u8 nonce_tail_recv[OVPN_NONCE_TAIL_SIZE];
I think the original reason was to have something to pass to sizeof()
without making it harder for the reader.
At some point I also wanted to get rid of the struct,but something
stopped me. Not sure what was though. Will give it a try.
Thanks a lot.
Regards,
--
Antonio Quartulli
OpenVPN Inc.