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.
+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?
+};
+
+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?
@@ -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.
--- 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?
+#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.
+
+/* 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.
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?
+
+/* 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.
+/* 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];