| Message ID | 20241029-b4-ovpn-v11-4-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
On 29.10.2024 12:47, Antonio Quartulli wrote: > Add basic infrastructure for handling ovpn interfaces. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > 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? > + sizeof(struct udphdr) + > + max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); > + > + netdev_features_t feat = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM | > + NETIF_F_GSO | NETIF_F_GSO_SOFTWARE | > + NETIF_F_HIGHDMA; > + > + dev->needs_free_netdev = true; > + > + dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS; > + > + dev->netdev_ops = &ovpn_netdev_ops; > + > + dev->priv_destructor = ovpn_struct_free; > + > + dev->hard_header_len = 0; > + dev->addr_len = 0; > + dev->mtu = ETH_DATA_LEN - overhead; > + dev->min_mtu = IPV4_MIN_MTU; > + dev->max_mtu = IP_MAX_MTU - overhead; > + > + dev->type = ARPHRD_NONE; > + dev->flags = IFF_POINTOPOINT | IFF_NOARP; > + dev->priv_flags |= IFF_NO_QUEUE; > + > + dev->lltx = true; > + dev->features |= feat; > + dev->hw_features |= feat; > + dev->hw_enc_features |= feat; > + > + dev->needed_headroom = OVPN_HEAD_ROOM; > + dev->needed_tailroom = OVPN_MAX_PADDING; > + > + SET_NETDEV_DEVTYPE(dev, &ovpn_type); > +} > + > static int ovpn_newlink(struct net *src_net, struct net_device *dev, > struct nlattr *tb[], struct nlattr *data[], > struct netlink_ext_ack *extack) > { > - return -EOPNOTSUPP; > + struct ovpn_struct *ovpn = netdev_priv(dev); > + enum ovpn_mode mode = OVPN_MODE_P2P; > + > + if (data && data[IFLA_OVPN_MODE]) { > + mode = nla_get_u8(data[IFLA_OVPN_MODE]); > + netdev_dbg(dev, "setting device mode: %u\n", mode); > + } > + > + ovpn->dev = dev; > + ovpn->mode = mode; > + > + /* turn carrier explicitly off after registration, this way state is > + * clearly defined > + */ > + netif_carrier_off(dev); > + > + return register_netdevice(dev); > } > > static struct rtnl_link_ops ovpn_link_ops = { > .kind = OVPN_FAMILY_NAME, > .netns_refund = false, > + .priv_size = sizeof(struct ovpn_struct), > + .setup = ovpn_setup, > + .policy = ovpn_policy, > + .maxtype = IFLA_OVPN_MAX, > .newlink = ovpn_newlink, > .dellink = unregister_netdevice_queue, > }; > @@ -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. > + > switch (state) { > case NETDEV_REGISTER: > - /* add device to internal list for later destruction upon > - * unregistration > - */ > + ovpn->registered = true; > break; > case NETDEV_UNREGISTER: > + /* twiddle thumbs on netns device moves */ > + if (dev->reg_state != NETREG_UNREGISTERING) > + break; > + > /* can be delivered multiple times, so check registered flag, > * then destroy the interface > */ > + if (!ovpn->registered) > + return NOTIFY_DONE; > + > + netif_carrier_off(dev); > + ovpn->registered = false; > break; > case NETDEV_POST_INIT: > case NETDEV_GOING_DOWN: > case NETDEV_DOWN: > case NETDEV_UP: > case NETDEV_PRE_UP: > + break; > default: > return NOTIFY_DONE; > } > diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h > index a3215316c49bfcdf2496590bac878f145b8b27fd..0740a05070a817e0daea7b63a1f4fcebd274eb37 100644 > --- 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. > }; > > #endif /* _NET_OVPN_OVPNSTRUCT_H_ */ > diff --git a/drivers/net/ovpn/packet.h b/drivers/net/ovpn/packet.h > new file mode 100644 > index 0000000000000000000000000000000000000000..7ed146f5932a25f448af6da58738a7eae81007fe > --- /dev/null > +++ b/drivers/net/ovpn/packet.h > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2020-2024 OpenVPN, Inc. > + * > + * Author: Antonio Quartulli <antonio@openvpn.net> > + * James Yonan <james@openvpn.net> > + */ > + > +#ifndef _NET_OVPN_PACKET_H_ > +#define _NET_OVPN_PACKET_H_ > + > +/* When the OpenVPN protocol is ran in AEAD mode, use > + * the OpenVPN packet ID as the AEAD nonce: > + * > + * 00000005 521c3b01 4308c041 > + * [seq # ] [ nonce_tail ] > + * [ 12-byte full IV ] -> NONCE_SIZE > + * [4-bytes -> NONCE_WIRE_SIZE > + * on wire] > + */ Nice diagram! Can we go futher and define the OpenVPN packet header as a stucture? Referencing the structure instead of using magic sizes and offsets can greatly improve the code readability. Especially when it comes to header construction/parsing in the encryption/decryption code. E.g. define a structures like this: struct ovpn_pkt_hdr { __be32 op; __be32 pktid; u8 auth[]; } __attribute__((packed)); struct ovpn_aead_iv { __be32 pktid; u8 nonce[OVPN_NONCE_TAIL_SIZE]; } __attribute__((packed)); > + > +/* 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]; > +#endif /* _NET_OVPN_PACKET_H_ */ > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -1975,4 +1975,19 @@ enum { > > #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) > > +/* OVPN section */ > + > +enum ovpn_mode { > + OVPN_MODE_P2P, > + OVPN_MODE_MP, > +}; Mode min/max values can be defined here and the netlink policy can reference these values: enum ovpn_mode { OVPN_MODE_P2P, OVPN_MODE_MP, __OVPN_MODE_MAX }; #define OVPN_MODE_MIN OVPN_MODE_P2P #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1) ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX) > + > +enum { > + IFLA_OVPN_UNSPEC, > + IFLA_OVPN_MODE, > + __IFLA_OVPN_MAX, > +}; > + > +#define IFLA_OVPN_MAX (__IFLA_OVPN_MAX - 1) > + > #endif /* _UAPI_LINUX_IF_LINK_H */ > -- Sergey
Missed the most essential note regarding this patch :) On 29.10.2024 12:47, Antonio Quartulli wrote: > +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); Here we stop a user generated traffic in downlink. Shall we take care about other kinds of traffic: keepalive, uplink? I believe we should remove all the peers here or at least stop the keepalive generation. But peers removing is better since administratively down is administratively down, meaning user expected full traffic stop in any direction. And even if we only stop the keepalive generation then peer(s) anyway will destroy the tunnel on their side. This way we even should not care about peers removing on the device unregistering. What do you think? > + return 0; > +}
2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote: > On 29.10.2024 12:47, Antonio Quartulli wrote: > > +/* When the OpenVPN protocol is ran in AEAD mode, use > > + * the OpenVPN packet ID as the AEAD nonce: > > + * > > + * 00000005 521c3b01 4308c041 > > + * [seq # ] [ nonce_tail ] > > + * [ 12-byte full IV ] -> NONCE_SIZE > > + * [4-bytes -> NONCE_WIRE_SIZE > > + * on wire] > > + */ > > Nice diagram! Can we go futher and define the OpenVPN packet header as a > stucture? Referencing the structure instead of using magic sizes and offsets > can greatly improve the code readability. Especially when it comes to header > construction/parsing in the encryption/decryption code. > > E.g. define a structures like this: > > struct ovpn_pkt_hdr { > __be32 op; > __be32 pktid; > u8 auth[]; > } __attribute__((packed)); > > struct ovpn_aead_iv { > __be32 pktid; > u8 nonce[OVPN_NONCE_TAIL_SIZE]; > } __attribute__((packed)); __attribute__((packed)) should not be needed here as the fields in both structs look properly aligned, and IIRC using packed can cause the compiler to generate worse code. > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > > index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644 > > --- a/include/uapi/linux/if_link.h > > +++ b/include/uapi/linux/if_link.h > > @@ -1975,4 +1975,19 @@ enum { > > #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) > > +/* OVPN section */ > > + > > +enum ovpn_mode { > > + OVPN_MODE_P2P, > > + OVPN_MODE_MP, > > +}; > > Mode min/max values can be defined here and the netlink policy can reference > these values: > > enum ovpn_mode { > OVPN_MODE_P2P, > OVPN_MODE_MP, > __OVPN_MODE_MAX > }; > > #define OVPN_MODE_MIN OVPN_MODE_P2P > #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1) > > ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX) I don't think there's much benefit to that, other than making the diff smaller on a (very unlikely) patch that would add a new mode in the future. It even looks more inconvenient to me when reading the code ("ok what are _MIN and _MAX? the code is using _P2P and _MP, do they match?").
On 12.11.2024 18:47, Sabrina Dubroca wrote: > 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote: >> On 29.10.2024 12:47, Antonio Quartulli wrote: >>> +/* When the OpenVPN protocol is ran in AEAD mode, use >>> + * the OpenVPN packet ID as the AEAD nonce: >>> + * >>> + * 00000005 521c3b01 4308c041 >>> + * [seq # ] [ nonce_tail ] >>> + * [ 12-byte full IV ] -> NONCE_SIZE >>> + * [4-bytes -> NONCE_WIRE_SIZE >>> + * on wire] >>> + */ >> >> Nice diagram! Can we go futher and define the OpenVPN packet header as a >> stucture? Referencing the structure instead of using magic sizes and offsets >> can greatly improve the code readability. Especially when it comes to header >> construction/parsing in the encryption/decryption code. >> >> E.g. define a structures like this: >> >> struct ovpn_pkt_hdr { >> __be32 op; >> __be32 pktid; >> u8 auth[]; >> } __attribute__((packed)); >> >> struct ovpn_aead_iv { >> __be32 pktid; >> u8 nonce[OVPN_NONCE_TAIL_SIZE]; >> } __attribute__((packed)); > > __attribute__((packed)) should not be needed here as the fields in > both structs look properly aligned, and IIRC using packed can cause > the compiler to generate worse code. True, the fields are pretty good aligned and from code generation perspective packed indication is unneeded. I suggested to mark structs as packed mostly as a documentation to clearly state that these structures represent specific memory layout. >>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h >>> index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644 >>> --- a/include/uapi/linux/if_link.h >>> +++ b/include/uapi/linux/if_link.h >>> @@ -1975,4 +1975,19 @@ enum { >>> #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) >>> +/* OVPN section */ >>> + >>> +enum ovpn_mode { >>> + OVPN_MODE_P2P, >>> + OVPN_MODE_MP, >>> +}; >> >> Mode min/max values can be defined here and the netlink policy can reference >> these values: >> >> enum ovpn_mode { >> OVPN_MODE_P2P, >> OVPN_MODE_MP, >> __OVPN_MODE_MAX >> }; >> >> #define OVPN_MODE_MIN OVPN_MODE_P2P >> #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1) >> >> ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX) > > I don't think there's much benefit to that, other than making the diff > smaller on a (very unlikely) patch that would add a new mode in the > future. It even looks more inconvenient to me when reading the code > ("ok what are _MIN and _MAX? the code is using _P2P and _MP, do they > match?"). I would answer yes. Just prefer to trust these kind of statements until it crashes badly. Honestly, I never thought that referring to a max value might raise such a question. Can you give an example why it should be meaningful to know exact min/max values of an unordered set? I suggested to define boundaries indeed for documentation purpose. Diff reduction is also desirable, but as you already mentioned, here it is not the case. Using specific values in a range declaration assigns them with extra semantic. Like, MODE_P2P is also a minimal possible value while MODE_MP has this extra meaning of minimal possible value. And we can learn this only from the policy which is specified far way from the modes declarations. I also see policies declaration as referring to already defined information rather than creating new meanings. On another hand the NL policy is the only user, so maybe we should left it as-is for the sake of simplicity. -- Sergey
On 12/11/2024 17:47, Sabrina Dubroca wrote: > 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote: >> On 29.10.2024 12:47, Antonio Quartulli wrote: >>> +/* When the OpenVPN protocol is ran in AEAD mode, use >>> + * the OpenVPN packet ID as the AEAD nonce: >>> + * >>> + * 00000005 521c3b01 4308c041 >>> + * [seq # ] [ nonce_tail ] >>> + * [ 12-byte full IV ] -> NONCE_SIZE >>> + * [4-bytes -> NONCE_WIRE_SIZE >>> + * on wire] >>> + */ >> >> Nice diagram! Can we go futher and define the OpenVPN packet header as a >> stucture? Referencing the structure instead of using magic sizes and offsets >> can greatly improve the code readability. Especially when it comes to header >> construction/parsing in the encryption/decryption code. >> >> E.g. define a structures like this: >> >> struct ovpn_pkt_hdr { >> __be32 op; >> __be32 pktid; >> u8 auth[]; >> } __attribute__((packed)); >> >> struct ovpn_aead_iv { >> __be32 pktid; >> u8 nonce[OVPN_NONCE_TAIL_SIZE]; >> } __attribute__((packed)); > > __attribute__((packed)) should not be needed here as the fields in > both structs look properly aligned, and IIRC using packed can cause > the compiler to generate worse code. Agreed. Using packed will make certain architecture read every field byte by byte (I remember David M. biting us on this in batman-adv :)) This said, I like the idea of using a struct, but I don't feel confident enough to change the code now that we are hitting v12. This kind of change will be better implemented later and tested carefully. (and patches are always welcome! :)) > > >>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h >>> index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644 >>> --- a/include/uapi/linux/if_link.h >>> +++ b/include/uapi/linux/if_link.h >>> @@ -1975,4 +1975,19 @@ enum { >>> #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) >>> +/* OVPN section */ >>> + >>> +enum ovpn_mode { >>> + OVPN_MODE_P2P, >>> + OVPN_MODE_MP, >>> +}; >> >> Mode min/max values can be defined here and the netlink policy can reference >> these values: >> >> enum ovpn_mode { >> OVPN_MODE_P2P, >> OVPN_MODE_MP, >> __OVPN_MODE_MAX >> }; >> >> #define OVPN_MODE_MIN OVPN_MODE_P2P >> #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1) >> >> ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX) > > I don't think there's much benefit to that, other than making the diff > smaller on a (very unlikely) patch that would add a new mode in the > future. It even looks more inconvenient to me when reading the code > ("ok what are _MIN and _MAX? the code is using _P2P and _MP, do they > match?"). I agree with Sabrina here. I also initially thought about having MIN/MAX, but it wouldn't make things simpler for the ovpn_mode. Regards,
On 14.11.2024 10:07, Antonio Quartulli wrote: > On 12/11/2024 17:47, Sabrina Dubroca wrote: >> 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote: >>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>> +/* When the OpenVPN protocol is ran in AEAD mode, use >>>> + * the OpenVPN packet ID as the AEAD nonce: >>>> + * >>>> + * 00000005 521c3b01 4308c041 >>>> + * [seq # ] [ nonce_tail ] >>>> + * [ 12-byte full IV ] -> NONCE_SIZE >>>> + * [4-bytes -> NONCE_WIRE_SIZE >>>> + * on wire] >>>> + */ >>> >>> Nice diagram! Can we go futher and define the OpenVPN packet header as a >>> stucture? Referencing the structure instead of using magic sizes and >>> offsets >>> can greatly improve the code readability. Especially when it comes to >>> header >>> construction/parsing in the encryption/decryption code. >>> >>> E.g. define a structures like this: >>> >>> struct ovpn_pkt_hdr { >>> __be32 op; >>> __be32 pktid; >>> u8 auth[]; >>> } __attribute__((packed)); >>> >>> struct ovpn_aead_iv { >>> __be32 pktid; >>> u8 nonce[OVPN_NONCE_TAIL_SIZE]; >>> } __attribute__((packed)); >> >> __attribute__((packed)) should not be needed here as the fields in >> both structs look properly aligned, and IIRC using packed can cause >> the compiler to generate worse code. > > Agreed. Using packed will make certain architecture read every field > byte by byte (I remember David M. biting us on this in batman-adv :)) Still curious to see an example of that strange architecture/compiler combination. Anyway, as Sabrina mentioned, the header is already pretty aligned. So it's up to you how to document the structure. > This said, I like the idea of using a struct, but I don't feel confident > enough to change the code now that we are hitting v12. > This kind of change will be better implemented later and tested > carefully. (and patches are always welcome! :)) The main reason behind the structure introduction is to improve the code readability. To reduce a shadow, where bugs can reside. I wonder how many people have invested their time to dig through the encryption preparation function? As for risk of breaking something I should say that it can be addressed by connecting the kernel implementation to pure usespace implementation, which can be assumed the reference. And, I believe, it worth the benefit of merging easy to understand code. -- Sergey
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@openvpn.net> >> --- >> 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,
On 14/11/2024 23:57, Sergey Ryazanov wrote: > On 14.11.2024 10:07, Antonio Quartulli wrote: >> On 12/11/2024 17:47, Sabrina Dubroca wrote: >>> 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote: >>>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>>> +/* When the OpenVPN protocol is ran in AEAD mode, use >>>>> + * the OpenVPN packet ID as the AEAD nonce: >>>>> + * >>>>> + * 00000005 521c3b01 4308c041 >>>>> + * [seq # ] [ nonce_tail ] >>>>> + * [ 12-byte full IV ] -> NONCE_SIZE >>>>> + * [4-bytes -> NONCE_WIRE_SIZE >>>>> + * on wire] >>>>> + */ >>>> >>>> Nice diagram! Can we go futher and define the OpenVPN packet header >>>> as a >>>> stucture? Referencing the structure instead of using magic sizes and >>>> offsets >>>> can greatly improve the code readability. Especially when it comes >>>> to header >>>> construction/parsing in the encryption/decryption code. >>>> >>>> E.g. define a structures like this: >>>> >>>> struct ovpn_pkt_hdr { >>>> __be32 op; >>>> __be32 pktid; >>>> u8 auth[]; >>>> } __attribute__((packed)); >>>> >>>> struct ovpn_aead_iv { >>>> __be32 pktid; >>>> u8 nonce[OVPN_NONCE_TAIL_SIZE]; >>>> } __attribute__((packed)); >>> >>> __attribute__((packed)) should not be needed here as the fields in >>> both structs look properly aligned, and IIRC using packed can cause >>> the compiler to generate worse code. >> >> Agreed. Using packed will make certain architecture read every field >> byte by byte (I remember David M. biting us on this in batman-adv :)) > > Still curious to see an example of that strange architecture/compiler > combination. Anyway, as Sabrina mentioned, the header is already pretty > aligned. So it's up to you how to document the structure. IIRC MIPS was one of those, but don't take my word for granted. > >> This said, I like the idea of using a struct, but I don't feel >> confident enough to change the code now that we are hitting v12. >> This kind of change will be better implemented later and tested >> carefully. (and patches are always welcome! :)) > > The main reason behind the structure introduction is to improve the code > readability. To reduce a shadow, where bugs can reside. I wonder how > many people have invested their time to dig through the encryption > preparation function? > > As for risk of breaking something I should say that it can be addressed > by connecting the kernel implementation to pure usespace implementation, > which can be assumed the reference. And, I believe, it worth the benefit > of merging easy to understand code. > I understand your point, but this is something I need to spend time on because the openvpn packet format is not "very stable", as in "it can vary depending on negotiated features". When implementing ovpn I decided what was the supported set of features so to create a stable packet header, but this may change moving forward (there is already some work going on in userspace regarding new features that ovpn will have to support). Therefore I want to take some time thinking about what's best. Regards,
On 10/11/2024 21:42, Sergey Ryazanov wrote: > Missed the most essential note regarding this patch :) > > On 29.10.2024 12:47, Antonio Quartulli wrote: >> +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); > > Here we stop a user generated traffic in downlink. Shall we take care > about other kinds of traffic: keepalive, uplink? Keepalive is "metadata" and should continue to flow, regardless of whether the user interface is brought down. Uplink traffic directed to *this* device should just be dropped at delivery time. Incoming traffic directed to other peers will continue to work. > > I believe we should remove all the peers here or at least stop the > keepalive generation. But peers removing is better since > administratively down is administratively down, meaning user expected > full traffic stop in any direction. And even if we only stop the > keepalive generation then peer(s) anyway will destroy the tunnel on > their side. Uhm, I don't think the user expects all "protocol" traffic (and client to client) to stop by simply bringing down the interface. > > This way we even should not care about peers removing on the device > unregistering. What do you think? I think you are now mixing data plane and control plane. The fact that the user is stopping payload traffic does not imply we want to stop the VPN. The user may just be doing something with the interface (and on an MP node client-to-client traffic will still continue to flow). This would also be a non-negligible (and user faving) change in behaviour compared to the current openvpn implementation. Thanks for your input though, I can imagine coming from different angles things may look not the same. Regards, > >> + return 0; >> +}
On 15.11.2024 16:03, Antonio Quartulli wrote: > On 10/11/2024 21:42, Sergey Ryazanov wrote: >> Missed the most essential note regarding this patch :) >> >> On 29.10.2024 12:47, Antonio Quartulli wrote: >>> +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); >> >> Here we stop a user generated traffic in downlink. Shall we take care >> about other kinds of traffic: keepalive, uplink? > > Keepalive is "metadata" and should continue to flow, regardless of > whether the user interface is brought down. > > Uplink traffic directed to *this* device should just be dropped at > delivery time. > > Incoming traffic directed to other peers will continue to work. How it's possible? AFAIU, the module uses the kernel IP routing subsystem. Putting the interface down will effectively block a client-to-client packet to reenter the interface. >> I believe we should remove all the peers here or at least stop the >> keepalive generation. But peers removing is better since >> administratively down is administratively down, meaning user expected >> full traffic stop in any direction. And even if we only stop the >> keepalive generation then peer(s) anyway will destroy the tunnel on >> their side. > > Uhm, I don't think the user expects all "protocol" traffic (and client > to client) to stop by simply bringing down the interface. > >> >> This way we even should not care about peers removing on the device >> unregistering. What do you think? > > I think you are now mixing data plane and control plane. > > The fact that the user is stopping payload traffic does not imply we > want to stop the VPN. > The user may just be doing something with the interface (and on an MP > node client-to-client traffic will still continue to flow). > > This would also be a non-negligible (and user faving) change in > behaviour compared to the current openvpn implementation. It's not about previous implementation, it's about the interface management procedures. I just cannot image how the proposed approach can be aligned with RFC 2863 section 3.1.13. IfAdminStatus and IfOperStatus. And if we are talking about a user experience, I cannot imagine my WLAN interface maintaining a connection to the access point after shutting it down. Or even better, a WLAN interface in the AP mode still forwarding traffic between wireless clients. Or a bridge interface switching traffic between ports and sending STP frames. > Thanks for your input though, I can imagine coming from different angles > things may look not the same. I believe nobody will mind if a userspace service will do a failover to continue serving connected clients. But from the kernel perspective, when user says 'ip link set down' the party is over. -- Sergey
On 19/11/2024 04:08, Sergey Ryazanov wrote: > On 15.11.2024 16:03, Antonio Quartulli wrote: >> On 10/11/2024 21:42, Sergey Ryazanov wrote: >>> Missed the most essential note regarding this patch :) >>> >>> On 29.10.2024 12:47, Antonio Quartulli wrote: >>>> +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); >>> >>> Here we stop a user generated traffic in downlink. Shall we take care >>> about other kinds of traffic: keepalive, uplink? >> >> Keepalive is "metadata" and should continue to flow, regardless of >> whether the user interface is brought down. >> >> Uplink traffic directed to *this* device should just be dropped at >> delivery time. >> >> Incoming traffic directed to other peers will continue to work. > > How it's possible? AFAIU, the module uses the kernel IP routing > subsystem. Putting the interface down will effectively block a client- > to-client packet to reenter the interface. True. At least part of the traffic is stopped (traffic directed to the VPN IP of a peer will still flow as it does not require a routing table lookup). I circled this discussion through the other devs to see what perspective they would bring and we also agree that if something is stopping, better stop the entire infra. Also, if a user is fumbling with the link state, they are probably trying to bring the VPN down. I will go that way and basically perform the same cleanup as if the interface is being deleted. "the party is over"[cit.] :) Regards,
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) +{ +} + +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, +}; + +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 + + sizeof(struct udphdr) + + max(sizeof(struct ipv6hdr), sizeof(struct iphdr)); + + netdev_features_t feat = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM | + NETIF_F_GSO | NETIF_F_GSO_SOFTWARE | + NETIF_F_HIGHDMA; + + dev->needs_free_netdev = true; + + dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS; + + dev->netdev_ops = &ovpn_netdev_ops; + + dev->priv_destructor = ovpn_struct_free; + + dev->hard_header_len = 0; + dev->addr_len = 0; + dev->mtu = ETH_DATA_LEN - overhead; + dev->min_mtu = IPV4_MIN_MTU; + dev->max_mtu = IP_MAX_MTU - overhead; + + dev->type = ARPHRD_NONE; + dev->flags = IFF_POINTOPOINT | IFF_NOARP; + dev->priv_flags |= IFF_NO_QUEUE; + + dev->lltx = true; + dev->features |= feat; + dev->hw_features |= feat; + dev->hw_enc_features |= feat; + + dev->needed_headroom = OVPN_HEAD_ROOM; + dev->needed_tailroom = OVPN_MAX_PADDING; + + SET_NETDEV_DEVTYPE(dev, &ovpn_type); +} + static int ovpn_newlink(struct net *src_net, struct net_device *dev, struct nlattr *tb[], struct nlattr *data[], struct netlink_ext_ack *extack) { - return -EOPNOTSUPP; + struct ovpn_struct *ovpn = netdev_priv(dev); + enum ovpn_mode mode = OVPN_MODE_P2P; + + if (data && data[IFLA_OVPN_MODE]) { + mode = nla_get_u8(data[IFLA_OVPN_MODE]); + netdev_dbg(dev, "setting device mode: %u\n", mode); + } + + ovpn->dev = dev; + ovpn->mode = mode; + + /* turn carrier explicitly off after registration, this way state is + * clearly defined + */ + netif_carrier_off(dev); + + return register_netdevice(dev); } static struct rtnl_link_ops ovpn_link_ops = { .kind = OVPN_FAMILY_NAME, .netns_refund = false, + .priv_size = sizeof(struct ovpn_struct), + .setup = ovpn_setup, + .policy = ovpn_policy, + .maxtype = IFLA_OVPN_MAX, .newlink = ovpn_newlink, .dellink = unregister_netdevice_queue, }; @@ -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); + switch (state) { case NETDEV_REGISTER: - /* add device to internal list for later destruction upon - * unregistration - */ + ovpn->registered = true; break; case NETDEV_UNREGISTER: + /* twiddle thumbs on netns device moves */ + if (dev->reg_state != NETREG_UNREGISTERING) + break; + /* can be delivered multiple times, so check registered flag, * then destroy the interface */ + if (!ovpn->registered) + return NOTIFY_DONE; + + netif_carrier_off(dev); + ovpn->registered = false; break; case NETDEV_POST_INIT: case NETDEV_GOING_DOWN: case NETDEV_DOWN: case NETDEV_UP: case NETDEV_PRE_UP: + break; default: return NOTIFY_DONE; } diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h index a3215316c49bfcdf2496590bac878f145b8b27fd..0740a05070a817e0daea7b63a1f4fcebd274eb37 100644 --- 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) +#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; }; #endif /* _NET_OVPN_OVPNSTRUCT_H_ */ diff --git a/drivers/net/ovpn/packet.h b/drivers/net/ovpn/packet.h new file mode 100644 index 0000000000000000000000000000000000000000..7ed146f5932a25f448af6da58738a7eae81007fe --- /dev/null +++ b/drivers/net/ovpn/packet.h @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: Antonio Quartulli <antonio@openvpn.net> + * James Yonan <james@openvpn.net> + */ + +#ifndef _NET_OVPN_PACKET_H_ +#define _NET_OVPN_PACKET_H_ + +/* When the OpenVPN protocol is ran in AEAD mode, use + * the OpenVPN packet ID as the AEAD nonce: + * + * 00000005 521c3b01 4308c041 + * [seq # ] [ nonce_tail ] + * [ 12-byte full IV ] -> NONCE_SIZE + * [4-bytes -> NONCE_WIRE_SIZE + * on wire] + */ + +/* OpenVPN nonce size */ +#define NONCE_SIZE 12 + +/* 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)) + +/* 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]; +}; + +#endif /* _NET_OVPN_PACKET_H_ */ diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -1975,4 +1975,19 @@ enum { #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) +/* OVPN section */ + +enum ovpn_mode { + OVPN_MODE_P2P, + OVPN_MODE_MP, +}; + +enum { + IFLA_OVPN_UNSPEC, + IFLA_OVPN_MODE, + __IFLA_OVPN_MAX, +}; + +#define IFLA_OVPN_MAX (__IFLA_OVPN_MAX - 1) + #endif /* _UAPI_LINUX_IF_LINK_H */
Add basic infrastructure for handling ovpn interfaces. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- 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(-)