On 09/11/2024 00:15, Sergey Ryazanov wrote:

On 29.10.2024 12:47, Antonio Quartulli wrote:

...

This commit introduces basic netlink support with family registration/unregistration functionalities and stub pre/post-doit.

More importantly it introduces the YAML uAPI description along with its auto-generated files:

• include/uapi/linux/ovpn.h
• drivers/net/ovpn/netlink-gen.c
• drivers/net/ovpn/netlink-gen.h

Cc: donald.hunter@gmail.com Signed-off-by: Antonio Quartulli antonio@openvpn.net

[skipped]

...

diff --git a/Documentation/netlink/specs/ovpn.yaml b/Documentation/ netlink/specs/ovpn.yaml

[skipped]

...

+attribute-sets: +  - +    name: peer +    attributes: +      - +        name: id +        type: u32 +        doc: | +          The unique ID of the peer. To be used to identify peers during +          operations

nit: could you specify the scope of uniqueness? I believe it is not globally uniq, it is just interface uniq, right?

Yeah it's per interface/instance. Will make it more clear, also for other IDs.

...

+        checks: +          max: 0xFFFFFF

[skipped]

...

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index 369a5a2b2fc1a497c8444e59f9b058eb40e49524..d5bdb0055f4dd3a6e32dc6e792bed1e7fd59e101 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -7,11 +7,15 @@    *        James Yonan james@openvpn.net    */ +#include <linux/genetlink.h>   #include <linux/module.h>   #include <linux/netdevice.h>   #include <net/rtnetlink.h> +#include <uapi/linux/ovpn.h> +#include "ovpnstruct.h"   #include "main.h" +#include "netlink.h"   #include "io.h"   /* Driver info */ @@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev,   }   static struct rtnl_link_ops ovpn_link_ops = { -    .kind = "ovpn", +    .kind = OVPN_FAMILY_NAME,

nit: are you sure that the link kind is the same as the GENL family? I mean, they are both deriviated from the protocol name that is common for both entities, but is it making RTNL kind a derivative of GENL family?

I just want to use the same name everywhere and I thought it doesn't make sense to create a separate define (they can be decoupled later should see any need for that). But I can add:

#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME

to make this relationship explicit?

Regards,

On 19/11/2024 03:05, Sergey Ryazanov wrote:

On 15.11.2024 12:05, Antonio Quartulli wrote:

...

On 09/11/2024 00:15, Sergey Ryazanov wrote:

...

On 29.10.2024 12:47, Antonio Quartulli wrote:

...

@@ -37,7 +41,7 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev,   }   static struct rtnl_link_ops ovpn_link_ops = { -    .kind = "ovpn", +    .kind = OVPN_FAMILY_NAME,

nit: are you sure that the link kind is the same as the GENL family? I mean, they are both deriviated from the protocol name that is common for both entities, but is it making RTNL kind a derivative of GENL family?

I just want to use the same name everywhere and I thought it doesn't make sense to create a separate define (they can be decoupled later should see any need for that). But I can add:

#define OVPN_RTNL_LINK_KIND OVPN_FAMILY_NAME

to make this relationship explicit?

Can we just leave it as literal? This string is going to be a part of ABI and there will be no chance to change it in the future. So, what the purpose to define it using a macro if it's self-descriptive?

I don't truly have a strong opinion, but the netlink family name is also expected to not change anytime soon.

Anyway, I see that using the literal is pretty common across all other drivers, therefore I'll go for it as well.

People also like to define a macro with a generic name like DRV_NAME and use it everywhere. What also looks reasonable.

Yeah, that's exactly how I am using OVPN_FAMILY_NAME. Anyway, I am switching to literal.

Regards,

-- Sergey

On 09/11/2024 00:31, Sergey Ryazanov wrote:

On 29.10.2024 12:47, Antonio Quartulli wrote:

...

This commit introduces basic netlink support with family registration/unregistration functionalities and stub pre/post-doit.

More importantly it introduces the YAML uAPI description along with its auto-generated files:

• include/uapi/linux/ovpn.h
• drivers/net/ovpn/netlink-gen.c
• drivers/net/ovpn/netlink-gen.h

Cc: donald.hunter@gmail.com Signed-off-by: Antonio Quartulli antonio@openvpn.net

[skipped]

...

diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ ovpnstruct.h --- /dev/null +++ b/drivers/net/ovpn/ovpnstruct.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*  OpenVPN data channel offload

• *  Copyright (C) 2019-2024 OpenVPN, Inc.
• *  Author:    James Yonan james@openvpn.net
• *        Antonio Quartulli antonio@openvpn.net
• */

+#ifndef _NET_OVPN_OVPNSTRUCT_H_ +#define _NET_OVPN_OVPNSTRUCT_H_

+#include <net/net_trackers.h>

+/**

• • struct ovpn_struct - per ovpn interface state
• • @dev: the actual netdev representing the tunnel
• • @dev_tracker: reference tracker for associated dev
• */

+struct ovpn_struct {

There is no standard convention how to entitle such structures, so the question is basically of out-of-curiosity class. For me, having a sturcuture with name 'struct' is like having no name. Did you consider to use such names as ovpn_dev or ovpn_iface? Meaning, using a name that gives a clue regarding the scope of the content.

Yes, I wanted to switch to ovpn_priv, but did not care much for the time being :)

I can still do it now in v12.

Thanks! Regards,

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

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 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 29.10.2024 12:47, Antonio Quartulli wrote:

An ovpn interface will keep carrier always on and let the user decide when an interface should be considered disconnected.

This way, even if an ovpn interface is not connected to any peer, it can still retain all IPs and routes and thus prevent any data leak.

Signed-off-by: Antonio Quartulli antonio@openvpn.net Reviewed-by: Andrew Lunn andrew@lunn.ch

---

drivers/net/ovpn/main.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index eead7677b8239eb3c48bb26ca95492d88512b8d4..eaa83a8662e4ac2c758201008268f9633643c0b6 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -31,6 +31,13 @@ static void ovpn_struct_free(struct net_device *net) static int ovpn_net_open(struct net_device *dev) {

• /* ovpn keeps the carrier always on to avoid losing IP or route

• * configuration upon disconnection. This way it can prevent leaks
  

• * of traffic outside of the VPN tunnel.
  

• * The user may override this behaviour by tearing down the interface
  

• * manually.
  

• */
  

• netif_carrier_on(dev);

If a user cares about the traffic leaking, then he can create a blackhole route with huge metric:

# ip route add blackhole default metric 10000

Why the network interface should implicitly provide this functionality? And on another hand, how a routing daemon can learn a topology change without indication from the interface?

netif_tx_start_all_queues(dev); return 0; }

On 15.11.2024 16:13, Antonio Quartulli wrote:

On 09/11/2024 02:11, Sergey Ryazanov wrote:

...

On 29.10.2024 12:47, Antonio Quartulli wrote:

...

An ovpn interface will keep carrier always on and let the user decide when an interface should be considered disconnected.

This way, even if an ovpn interface is not connected to any peer, it can still retain all IPs and routes and thus prevent any data leak.

Signed-off-by: Antonio Quartulli antonio@openvpn.net Reviewed-by: Andrew Lunn andrew@lunn.ch

---

drivers/net/ovpn/main.c | 7 +++++++   1 file changed, 7 insertions(+)

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index eead7677b8239eb3c48bb26ca95492d88512b8d4..eaa83a8662e4ac2c758201008268f9633643c0b6 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -31,6 +31,13 @@ static void ovpn_struct_free(struct net_device *net)   static int ovpn_net_open(struct net_device *dev)   { +    /* ovpn keeps the carrier always on to avoid losing IP or route +     * configuration upon disconnection. This way it can prevent leaks +     * of traffic outside of the VPN tunnel. +     * The user may override this behaviour by tearing down the interface +     * manually. +     */ +    netif_carrier_on(dev);

If a user cares about the traffic leaking, then he can create a blackhole route with huge metric:

# ip route add blackhole default metric 10000

Why the network interface should implicitly provide this functionality? And on another hand, how a routing daemon can learn a topology change without indication from the interface?

This was discussed loooong ago with Andrew. Here my last response:

https://lore.kernel.org/all/d896bbd8-2709-4834-a637- f982fc51fc57@openvpn.net/

Thank you for sharing the link to the beginning of the conversation. Till the moment we have 3 topics regarding the operational state indication: 1. possible absence of a conception of running state, 2. influence on routing protocol implementations, 3. traffic leaking.

As for conception of the running state, it should exists for tunneling protocols with a state tracking. In this specific case, we can assume interface running when it has configured peer with keys. The protocol even has nice feature for the connection monitoring - keepalive.

Routing protocols on one hand could benefit from the operational state indication. On another hand, hello/hold timer values mentioned in the documentation are comparable with default routing protocols timers. So, actual improvement is debatable.

Regarding the traffic leading, as I mentioned before, the blackhole route or a firewall rule works better then implicit blackholing with a non-running interface.

Long story short, I agree that we might not need a real operational state indication now. Still protecting from a traffic leaking is not good enough justification.

Andrew, what do you think? Is the traffic leaking prevention any good justification or it needs to be updated?

-- Sergey

On 21.11.2024 23:17, Antonio Quartulli wrote:

On 20/11/2024 23:56, Sergey Ryazanov wrote:

...

On 15.11.2024 16:13, Antonio Quartulli wrote:

...

On 09/11/2024 02:11, Sergey Ryazanov wrote:

...

On 29.10.2024 12:47, Antonio Quartulli wrote:

...

An ovpn interface will keep carrier always on and let the user decide when an interface should be considered disconnected.

This way, even if an ovpn interface is not connected to any peer, it can still retain all IPs and routes and thus prevent any data leak.

Signed-off-by: Antonio Quartulli antonio@openvpn.net Reviewed-by: Andrew Lunn andrew@lunn.ch

---

drivers/net/ovpn/main.c | 7 +++++++   1 file changed, 7 insertions(+)

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index eead7677b8239eb3c48bb26ca95492d88512b8d4..eaa83a8662e4ac2c758201008268f9633643c0b6 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -31,6 +31,13 @@ static void ovpn_struct_free(struct net_device *net)   static int ovpn_net_open(struct net_device *dev)   { +    /* ovpn keeps the carrier always on to avoid losing IP or route +     * configuration upon disconnection. This way it can prevent leaks +     * of traffic outside of the VPN tunnel. +     * The user may override this behaviour by tearing down the interface +     * manually. +     */ +    netif_carrier_on(dev);

If a user cares about the traffic leaking, then he can create a blackhole route with huge metric:

# ip route add blackhole default metric 10000

Why the network interface should implicitly provide this functionality? And on another hand, how a routing daemon can learn a topology change without indication from the interface?

This was discussed loooong ago with Andrew. Here my last response:

https://lore.kernel.org/all/d896bbd8-2709-4834-a637- f982fc51fc57@openvpn.net/

Thank you for sharing the link to the beginning of the conversation. Till the moment we have 3 topics regarding the operational state indication:

1. possible absence of a conception of running state,
2. influence on routing protocol implementations,
3. traffic leaking.

As for conception of the running state, it should exists for tunneling protocols with a state tracking. In this specific case, we can assume interface running when it has configured peer with keys. The protocol even has nice feature for the connection monitoring - keepalive.

What about a device in MP mode? It doesn't make sense to turn the carrier off when the MP node has no peers connected. At the same time I don't like having P2P and MP devices behaving differently in this regard.

MP with a single network interface is an endless headache. Indeed. On the other hand, penalizing P2P users just because protocol support MP doesn't look like a solution either.

Therefore keeping the carrier on seemed the most logical way forward (at least for now - we can still come back to this once we have something smarter to implement).

It was shown above how to distinguish between running and non-running cases.

If an author doesn't want to implement operational state indication now, then I'm Ok with it. Not a big deal now. I just don't like the idea to promote the abuse of the running state indicator. Please see below.

...

Routing protocols on one hand could benefit from the operational state indication. On another hand, hello/hold timer values mentioned in the documentation are comparable with default routing protocols timers. So, actual improvement is debatable.

Regarding the traffic leading, as I mentioned before, the blackhole route or a firewall rule works better then implicit blackholing with a non-running interface.

Long story short, I agree that we might not need a real operational state indication now. Still protecting from a traffic leaking is not good enough justification.

Well, it's the so called "persistent interface" concept in VPNs: leave everything as is, even if the connection is lost.

It's called routing framework abuse. The IP router will choose the route and the egress interface not because this route is a good option to deliver a packet, but because someone trick it.

At some circumstance, e.g. Android app, it could be the only way to prevent traffic leaking. But these special circumstances do not make solution generic and eligible for inclusion into the mainline code.

I know it can be implemented in many other different ways..but I don't see a real problem with keeping this way.

At least routing protocols and network monitoring software will not be happy to see a dead interface pretending that it's still running. Generally speaking, saying that interface is running, when module knows for sure that a packet can not be delivered is a user misguiding.

A blackhole/firewall can still be added if the user prefers (and not use the persistent interface).

The solution with false-indication is not so reliable as it might look. Interface shutdown, inability of a user-space application to start, user-space application crash, user-space application restart, each of them will void the trick. Ergo, blackhole/firewall must be employed by a security concerned user. What makes the proposed feature odd.

To summaries, I'm Ok if this change will be merged with a comment like "For future study" or "To be done" or "To be implemented". But a comment like "to prevent traffic leaking" or any other comment implying a "breakthrough security feature" will have a big NACK from my side.

-- Sergey

On 24.11.2024 00:52, Antonio Quartulli wrote:

On 23/11/2024 23:25, Sergey Ryazanov wrote:

...

On 21.11.2024 23:17, Antonio Quartulli wrote:

...

On 20/11/2024 23:56, Sergey Ryazanov wrote:

...

On 15.11.2024 16:13, Antonio Quartulli wrote:

...

On 09/11/2024 02:11, Sergey Ryazanov wrote:

...

On 29.10.2024 12:47, Antonio Quartulli wrote: > An ovpn interface will keep carrier always on and let the user > decide when an interface should be considered disconnected. > > This way, even if an ovpn interface is not connected to any peer, > it can still retain all IPs and routes and thus prevent any data > leak. > > Signed-off-by: Antonio Quartulli antonio@openvpn.net > Reviewed-by: Andrew Lunn andrew@lunn.ch > --- >   drivers/net/ovpn/main.c | 7 +++++++ >   1 file changed, 7 insertions(+) > > diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c > index > eead7677b8239eb3c48bb26ca95492d88512b8d4..eaa83a8662e4ac2c758201008268f9633643c0b6 100644 > --- a/drivers/net/ovpn/main.c > +++ b/drivers/net/ovpn/main.c > @@ -31,6 +31,13 @@ static void ovpn_struct_free(struct net_device > *net) >   static int ovpn_net_open(struct net_device *dev) >   { > +    /* ovpn keeps the carrier always on to avoid losing IP or route > +     * configuration upon disconnection. This way it can prevent > leaks > +     * of traffic outside of the VPN tunnel. > +     * The user may override this behaviour by tearing down the > interface > +     * manually. > +     */ > +    netif_carrier_on(dev);

If a user cares about the traffic leaking, then he can create a blackhole route with huge metric:

# ip route add blackhole default metric 10000

Why the network interface should implicitly provide this functionality? And on another hand, how a routing daemon can learn a topology change without indication from the interface?

This was discussed loooong ago with Andrew. Here my last response:

https://lore.kernel.org/all/d896bbd8-2709-4834-a637- f982fc51fc57@openvpn.net/

Thank you for sharing the link to the beginning of the conversation. Till the moment we have 3 topics regarding the operational state indication:

1. possible absence of a conception of running state,
2. influence on routing protocol implementations,
3. traffic leaking.

As for conception of the running state, it should exists for tunneling protocols with a state tracking. In this specific case, we can assume interface running when it has configured peer with keys. The protocol even has nice feature for the connection monitoring - keepalive.

What about a device in MP mode? It doesn't make sense to turn the carrier off when the MP node has no peers connected. At the same time I don't like having P2P and MP devices behaving differently in this regard.

MP with a single network interface is an endless headache. Indeed. On the other hand, penalizing P2P users just because protocol support MP doesn't look like a solution either.

On the upper side, with "iroutes" implemented using the system routing table, routing protocols will be able to detect new routes only when the related client has connected. (The same for the disconnection)

But this is a bit orthogonal compared to the oper state.

The patch has nothing common with the routes configuration. The main concern is forcing of the running state indication. And more specifically, the concern is the given justification for this activity.

...

...

Therefore keeping the carrier on seemed the most logical way forward (at least for now - we can still come back to this once we have something smarter to implement).

It was shown above how to distinguish between running and non-running cases.

If an author doesn't want to implement operational state indication now, then I'm Ok with it. Not a big deal now. I just don't like the idea to promote the abuse of the running state indicator. Please see below.

...

...

Routing protocols on one hand could benefit from the operational state indication. On another hand, hello/hold timer values mentioned in the documentation are comparable with default routing protocols timers. So, actual improvement is debatable.

Regarding the traffic leading, as I mentioned before, the blackhole route or a firewall rule works better then implicit blackholing with a non-running interface.

Long story short, I agree that we might not need a real operational state indication now. Still protecting from a traffic leaking is not good enough justification.

Well, it's the so called "persistent interface" concept in VPNs: leave everything as is, even if the connection is lost.

It's called routing framework abuse. The IP router will choose the route and the egress interface not because this route is a good option to deliver a packet, but because someone trick it.

This is what the user wants.

Will be happy to see a study on user's preferences.

OpenVPN (userspace) will tear down the P2P interface upon disconnection, assuming the --persist-tun option was not specified by the user.

So the interface is gone in any case.

By keeping the netcarrier on we are just ensuring that, if the user wanted persist-tun, the iface is not actually making decisions on its own.

Regarding a decision on its own. Ethernet interface going to the not-running state upon lost of carrier from a switch. It's hardly could be considered a decision of the interface. It's an indication of the fact.

Similarly, beeping of UPS is not its decision to make user's life miserable, it's the indication of the power line failure. I hope, at least we are both agree on that a UPS should indicate the line failure.

Back to the 'persist-tun' option. I checked the openvpn(8) man page. It gives a reasonable hints to use this option to avoid negative outcomes on internal openvpn process restart. E.g. in case of privilege dropping. It servers the same purpose as 'persist-key'. And there is no word regarding traffic leaking.

If somebody have decided that this option gives the funny side-effect and allows to cut the corners, then I cannot say anything but sorry.

With a tun interface this can be done, now you want to basically drop this feature that existed for long time and break existing setups.

Amicus Plato, sed magis amica veritas

Yes, I don't want to see this interface misbehaviour advertised as a security feature. I hope the previous email gives a detailed explanation why.

If it's going to break existing setup, then end-users can be supported with a changelog notice explaining how to properly address the risk of the traffic leaking.

...

At some circumstance, e.g. Android app, it could be the only way to prevent traffic leaking. But these special circumstances do not make solution generic and eligible for inclusion into the mainline code.

Why not? We are not changing the general rule, but just defining a specific behaviour for a specific driver.

Yeah. This patch is not changing the general rule. The patch breaks it and the comment in the code makes proud of it. Looks like an old joke that documented bug become a feature.

From a system administrator or a firmware developer perspective, the proposed behaviour will look like inconsistency comparing to other interface types. And this inconsistency requires to be addressed with special configuration or a dedicated script in a worst case. And I cannot see justified reason to make their life harder.

For example, I don't think a tun interface goes down when there is no socket attached to it, still packets are just going to be blackhole'd in that case. No?

Nope. Tun interface indeed will go into the non-running state on the detach event. Moreover, the tun module supports running/non-running indication change upon a command from userspace. But not every userspace application feel a desire to implement it.

...

...

I know it can be implemented in many other different ways..but I don't see a real problem with keeping this way.

At least routing protocols and network monitoring software will not be happy to see a dead interface pretending that it's still running.

They won't know that the interface is disconnected, they will possibly just see traffic being dropped.

Packet loss detection is quite complex operation. So yes, they are indeed monitoring the interface operational state to warn operator as soon as possible and take some automatic actions if we are talking about routing protocols. Some sophisticated monitoring systems even capable to generate events like 'link unstable' with higher severity if they see interface operational state flapping in a short period of time.

So yeah, for these kinds of systems, proper operational state indication is essential.

...

Generally speaking, saying that interface is running, when module knows for sure that a packet can not be delivered is a user misguiding.

Or a feature, wanted by the user.

...

...

A blackhole/firewall can still be added if the user prefers (and not use the persistent interface).

The solution with false-indication is not so reliable as it might look. Interface shutdown, inability of a user-space application to start, user-space application crash, user-space application restart, each of them will void the trick. Ergo, blackhole/firewall must be employed by a security concerned user. What makes the proposed feature odd.

Yeah, this is what other VPN clients call "kill switch". Persist-tun is just one piece of the puzzle, yet important.

...

To summaries, I'm Ok if this change will be merged with a comment like "For future study" or "To be done" or "To be implemented". But a comment like "to prevent traffic leaking" or any other comment implying a "breakthrough security feature" will have a big NACK from my side.

What if the comment redirects the user to --persist-tun option in order to clarify the context and the wanted behaviour?

Would that help?

Nope. As it was mentioned above, the are no indication that 'persist-tun' is a 'security' feature even in the current openvpn documentation.

If the openvpn developers want to keep implementation bug-to-bug compatible, then feel free to configure the blackhole route on behalf of end-user by means of the userspace daemon. Nobody will mind.

-- Sergey

On 29.10.2024 12:47, Antonio Quartulli wrote:

An ovpn_peer object holds the whole status of a remote peer (regardless whether it is a server or a client).

This includes status for crypto, tx/rx buffers, napi, etc.

Only support for one peer is introduced (P2P mode). Multi peer support is introduced with a later patch.

Reviewing the peer creation/destroying code I came to a generic question. Did you consider keeping a single P2P peer in the peers table as well?

Looks like such approach can greatly simply the code by dropping all these 'switch (ovpn->mode)' checks and implementing a unified peer management. The 'peer' field in the main private data structure can be kept to accelerate lookups, still using peers table for management tasks like removing all the peers on the interface teardown.

Along with the ovpn_peer, also the ovpn_bind object is introcued as the two are strictly related. An ovpn_bind object wraps a sockaddr representing the local coordinates being used to talk to a specific peer.

Signed-off-by: Antonio Quartulli antonio@openvpn.net

drivers/net/ovpn/Makefile | 2 + drivers/net/ovpn/bind.c | 58 +++++++ drivers/net/ovpn/bind.h | 117 ++++++++++++++

Why do we need these bind.c/bind.h files? They contains a minimum of code and still anyway references the peer object. Can we merge these definitions and code into peer.c/peer.h?

drivers/net/ovpn/main.c | 11 ++ drivers/net/ovpn/main.h | 2 + drivers/net/ovpn/ovpnstruct.h | 4 + drivers/net/ovpn/peer.c | 354 ++++++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/peer.h | 79 ++++++++++ 8 files changed, 627 insertions(+)

[...]

diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c new file mode 100644 index 0000000000000000000000000000000000000000..b4d2ccec2ceddf43bc445b489cc62a578ef0ad0a --- /dev/null +++ b/drivers/net/ovpn/bind.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload

• • Copyright (C) 2012-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#include <linux/netdevice.h> +#include <linux/socket.h>

+#include "ovpnstruct.h" +#include "bind.h" +#include "peer.h"

+/**

• • ovpn_bind_from_sockaddr - retrieve binding matching sockaddr
• • @ss: the sockaddr to match
• • Return: the bind matching the passed sockaddr if found, NULL otherwise

The function returns ERR_PTR() in case of error, the comment should be updated.

• */

+struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss) +{

• struct ovpn_bind *bind;
• size_t sa_len;
• if (ss->ss_family == AF_INET)

• sa_len = sizeof(struct sockaddr_in);
  

• else if (ss->ss_family == AF_INET6)

• sa_len = sizeof(struct sockaddr_in6);
  

• else

• return ERR_PTR(-EAFNOSUPPORT);
  

• bind = kzalloc(sizeof(*bind), GFP_ATOMIC);
• if (unlikely(!bind))

• return ERR_PTR(-ENOMEM);
  

• memcpy(&bind->remote, ss, sa_len);
• return bind;

+}

+/**

• • ovpn_bind_reset - assign new binding to peer
• • @peer: the peer whose binding has to be replaced
• • @new: the new bind to assign
• */

+void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new) +{

• struct ovpn_bind *old;
• spin_lock_bh(&peer->lock);
• old = rcu_replace_pointer(peer->bind, new, true);
• spin_unlock_bh(&peer->lock);

Locking will be removed from this function in the subsequent patch. Should we move the peer->lock usage to ovpn_peer_release() now?

• kfree_rcu(old, rcu);

+} diff --git a/drivers/net/ovpn/bind.h b/drivers/net/ovpn/bind.h new file mode 100644 index 0000000000000000000000000000000000000000..859213d5040deb36c416eafcf5c6ab31c4d52c7a --- /dev/null +++ b/drivers/net/ovpn/bind.h @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload

• • Copyright (C) 2012-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#ifndef _NET_OVPN_OVPNBIND_H_ +#define _NET_OVPN_OVPNBIND_H_

+#include <net/ip.h> +#include <linux/in.h> +#include <linux/in6.h> +#include <linux/rcupdate.h> +#include <linux/skbuff.h> +#include <linux/spinlock.h>

+struct ovpn_peer;

+/**

• • union ovpn_sockaddr - basic transport layer address

Why do we need this dedicated named union? Can we merge this union into the ovpn_bind struct as already done for the local address?

• • @in4: IPv4 address
• • @in6: IPv6 address
• */

+union ovpn_sockaddr {

Family type can be putted here as a dedicated element to make address type check simple:

unsigned short int sa_family;

• struct sockaddr_in in4;
• struct sockaddr_in6 in6;

+};> + +/**

• • struct ovpn_bind - remote peer binding
• • @remote: the remote peer sockaddress
• • @local: local endpoint used to talk to the peer
• • @local.ipv4: local IPv4 used to talk to the peer
• • @local.ipv6: local IPv6 used to talk to the peer
• • @rcu: used to schedule RCU cleanup job
• */

+struct ovpn_bind {

• union ovpn_sockaddr remote; /* remote sockaddr */
• union {

• struct in_addr ipv4;
  

• struct in6_addr ipv6;
  

• } local;
• struct rcu_head rcu;

+};

+/**

• • skb_protocol_to_family - translate skb->protocol to AF_INET or AF_INET6
• • @skb: the packet sk_buff to inspect
• • Return: AF_INET, AF_INET6 or 0 in case of unknown protocol
• */

+static inline unsigned short skb_protocol_to_family(const struct sk_buff *skb)

The function called outside the peer.c file only in ovpn_decrypt_post() and that call is debatable. Considering this, I belive, the skb_protocol_to_family() should be moved inside peer.c as static non-inlined function.

+{

• switch (skb->protocol) {
• case htons(ETH_P_IP):

• return AF_INET;
  

• case htons(ETH_P_IPV6):

• return AF_INET6;
  

• default:

• return 0;
  

• }

+}

+/**

• • ovpn_bind_skb_src_match - match packet source with binding
• • @bind: the binding to match
• • @skb: the packet to match
• • Return: true if the packet source matches the remote peer sockaddr
• • in the binding
• */

+static inline bool ovpn_bind_skb_src_match(const struct ovpn_bind *bind,

• 			   const struct sk_buff *skb)
  

The function is called only from ovpn_peer_float() and probably should be moved into peer.c and un-inlined.

+{

• const unsigned short family = skb_protocol_to_family(skb);
• const union ovpn_sockaddr *remote;
• if (unlikely(!bind))

• return false;
  

The caller ovpn_peer_float() function already verified bind object pointer, why we should redo the same check here?

• remote = &bind->remote;
• if (unlikely(remote->in4.sin_family != family))

• return false;
  

• switch (family) {
• case AF_INET:

• if (unlikely(remote->in4.sin_addr.s_addr != ip_hdr(skb)->saddr))
  

• 	return false;
  

• if (unlikely(remote->in4.sin_port != udp_hdr(skb)->source))
  

• 	return false;
  

• break;
  

• case AF_INET6:

• if (unlikely(!ipv6_addr_equal(&remote->in6.sin6_addr,
  

• 			      &ipv6_hdr(skb)->saddr)))
  

• 	return false;
  

• if (unlikely(remote->in6.sin6_port != udp_hdr(skb)->source))
  

• 	return false;
  

• break;
  

• default:

• return false;
  

• }
• return true;

+}

+struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *sa); +void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *bind);

+#endif /* _NET_OVPN_OVPNBIND_H_ */ diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index eaa83a8662e4ac2c758201008268f9633643c0b6..5492ce07751d135c1484fe1ed8227c646df94969 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -20,6 +20,7 @@ #include "netlink.h" #include "io.h" #include "packet.h" +#include "peer.h" /* Driver info */ #define DRV_DESCRIPTION "OpenVPN data channel offload (ovpn)" @@ -29,6 +30,11 @@ static void ovpn_struct_free(struct net_device *net) { } +static int ovpn_net_init(struct net_device *dev) +{

• return 0;

+}

The function is not required. Can we move its introduction to 'implement basic RX path (UDP)' patch, where its content will be added and where counterpart ovpn_net_uninit() function will be introduced?

• static int ovpn_net_open(struct net_device *dev) { /* ovpn keeps the carrier always on to avoid losing IP or route

@@ -49,6 +55,7 @@ static int ovpn_net_stop(struct net_device *dev) } static const struct net_device_ops ovpn_netdev_ops = {

• .ndo_init = ovpn_net_init, .ndo_open = ovpn_net_open, .ndo_stop = ovpn_net_stop, .ndo_start_xmit = ovpn_net_xmit,

@@ -128,6 +135,7 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev, ovpn->dev = dev; ovpn->mode = mode;

• spin_lock_init(&ovpn->lock);

/* turn carrier explicitly off after registration, this way state is * clearly defined @@ -176,6 +184,9 @@ static int ovpn_netdev_notifier_call(struct notifier_block *nb, netif_carrier_off(dev); ovpn->registered = false;

• if (ovpn->mode == OVPN_MODE_P2P)
  

• 	ovpn_peer_release_p2p(ovpn);
  

  break; case NETDEV_POST_INIT: case NETDEV_GOING_DOWN:

diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h index 0740a05070a817e0daea7b63a1f4fcebd274eb37..28e5c44816e110974333a7a6a9cf18bd15ae84e6 100644 --- a/drivers/net/ovpn/main.h +++ b/drivers/net/ovpn/main.h @@ -19,4 +19,6 @@ bool ovpn_dev_is_valid(const struct net_device *dev); #define OVPN_HEAD_ROOM ALIGN(16 + SKB_HEADER_LEN, 4) #define OVPN_MAX_PADDING 16 +#define OVPN_QUEUE_LEN 1024

This macro is unused, should we drop it?

• #endif /* _NET_OVPN_MAIN_H_ */

diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h index 211df871538d34fdff90d182f21a0b0fb11b28ad..a22c5083381c131db01a28c0f51e661d690d4998 100644 --- a/drivers/net/ovpn/ovpnstruct.h +++ b/drivers/net/ovpn/ovpnstruct.h @@ -20,6 +20,8 @@

• @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, ..)

• • @lock: protect this object
• • @peer: in P2P mode, this is the only remote peer
  • @dev_list: entry for the module wide device list
  */ struct ovpn_struct {

@@ -27,6 +29,8 @@ struct ovpn_struct { netdevice_tracker dev_tracker; bool registered; enum ovpn_mode mode;

• spinlock_t lock; /* protect writing to the ovpn_struct object */
• struct ovpn_peer __rcu *peer; struct list_head dev_list; };

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c new file mode 100644 index 0000000000000000000000000000000000000000..d9788a0cc99b5839c466c35d1b2266cc6b95fb72 --- /dev/null +++ b/drivers/net/ovpn/peer.c @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload

• • Copyright (C) 2020-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#include <linux/skbuff.h> +#include <linux/list.h>

+#include "ovpnstruct.h" +#include "bind.h" +#include "io.h" +#include "main.h" +#include "netlink.h" +#include "peer.h"

+/**

• • ovpn_peer_new - allocate and initialize a new peer object
• • @ovpn: the openvpn instance inside which the peer should be created
• • @id: the ID assigned to this peer
• • Return: a pointer to the new peer on success or an error code otherwise
• */

+struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) +{

• struct ovpn_peer *peer;
• int ret;
• /* alloc and init peer object */
• peer = kzalloc(sizeof(*peer), GFP_KERNEL);
• if (!peer)

• return ERR_PTR(-ENOMEM);
  

• peer->id = id;
• peer->halt = false;
• peer->ovpn = ovpn;
• peer->vpn_addrs.ipv4.s_addr = htonl(INADDR_ANY);
• peer->vpn_addrs.ipv6 = in6addr_any;
• RCU_INIT_POINTER(peer->bind, NULL);
• spin_lock_init(&peer->lock);
• kref_init(&peer->refcount);
• ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
• if (ret < 0) {

• netdev_err(ovpn->dev, "%s: cannot initialize dst cache\n",
  

• 	   __func__);
  

• kfree(peer);
  

• return ERR_PTR(ret);
  

• }
• netdev_hold(ovpn->dev, &ovpn->dev_tracker, GFP_KERNEL);
• return peer;

+}

+/**

• • ovpn_peer_release - release peer private members
• • @peer: the peer to release
• */

+static void ovpn_peer_release(struct ovpn_peer *peer) +{

• ovpn_bind_reset(peer, NULL);
• dst_cache_destroy(&peer->dst_cache);
• netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
• kfree_rcu(peer, rcu);

+}

+/**

• • ovpn_peer_release_kref - callback for kref_put
• • @kref: the kref object belonging to the peer
• */

+void ovpn_peer_release_kref(struct kref *kref) +{

• struct ovpn_peer *peer = container_of(kref, struct ovpn_peer, refcount);
• ovpn_peer_release(peer);

+}

+/**

• • ovpn_peer_skb_to_sockaddr - fill sockaddr with skb source address
• • @skb: the packet to extract data from
• • @ss: the sockaddr to fill
• • Return: true on success or false otherwise
• */

+static bool ovpn_peer_skb_to_sockaddr(struct sk_buff *skb,

• 		      struct sockaddr_storage *ss)
  

+{

• struct sockaddr_in6 *sa6;
• struct sockaddr_in *sa4;
• ss->ss_family = skb_protocol_to_family(skb);

Why do we need skb_protocol_to_family() call? Can we directly use skb->protocol and ETH_P_IP/ETH_P_IPV6 in the switch?

• switch (ss->ss_family) {
• case AF_INET:

• sa4 = (struct sockaddr_in *)ss;
  

• sa4->sin_family = AF_INET;
  

• sa4->sin_addr.s_addr = ip_hdr(skb)->saddr;
  

• sa4->sin_port = udp_hdr(skb)->source;
  

• break;
  

• case AF_INET6:

• sa6 = (struct sockaddr_in6 *)ss;
  

• sa6->sin6_family = AF_INET6;
  

• sa6->sin6_addr = ipv6_hdr(skb)->saddr;
  

• sa6->sin6_port = udp_hdr(skb)->source;
  

• break;
  

• default:

• return false;
  

• }
• return true;

+}

+/**

• • ovpn_peer_transp_match - check if sockaddr and peer binding match
• • @peer: the peer to get the binding from
• • @ss: the sockaddr to match
• • Return: true if sockaddr and binding match or false otherwise
• */

+static bool ovpn_peer_transp_match(const struct ovpn_peer *peer,

• 		   const struct sockaddr_storage *ss)
  

+{

• struct ovpn_bind *bind = rcu_dereference(peer->bind);
• struct sockaddr_in6 *sa6;
• struct sockaddr_in *sa4;
• if (unlikely(!bind))

• return false;
  

• if (ss->ss_family != bind->remote.in4.sin_family)

nit: if the dedicated 'sa_family' element is added into the union, the check can be more straighforward (without 'in4' access):

if (ss->ss_family != bind->remote.sa_family)

• return false;
  

• switch (ss->ss_family) {
• case AF_INET:

• sa4 = (struct sockaddr_in *)ss;
  

• if (sa4->sin_addr.s_addr != bind->remote.in4.sin_addr.s_addr)
  

• 	return false;
  

• if (sa4->sin_port != bind->remote.in4.sin_port)
  

• 	return false;
  

• break;
  

• case AF_INET6:

• sa6 = (struct sockaddr_in6 *)ss;
  

• if (!ipv6_addr_equal(&sa6->sin6_addr,
  

• 		     &bind->remote.in6.sin6_addr))
  

• 	return false;
  

• if (sa6->sin6_port != bind->remote.in6.sin6_port)
  

• 	return false;
  

• break;
  

• default:

• return false;
  

• }
• return true;

+}

+/**

• • ovpn_peer_get_by_transp_addr_p2p - get peer by transport address in a P2P

• •                                instance
    

• • @ovpn: the openvpn instance to search
• • @ss: the transport socket address
• • Return: the peer if found or NULL otherwise
• */

+static struct ovpn_peer * +ovpn_peer_get_by_transp_addr_p2p(struct ovpn_struct *ovpn,

• 		 struct sockaddr_storage *ss)
  

+{

• struct ovpn_peer *tmp, *peer = NULL;
• rcu_read_lock();
• tmp = rcu_dereference(ovpn->peer);
• if (likely(tmp && ovpn_peer_transp_match(tmp, ss) &&

•    ovpn_peer_hold(tmp)))
  

• peer = tmp;
  

• rcu_read_unlock();
• return peer;

+}

+/**

• • ovpn_peer_get_by_transp_addr - retrieve peer by transport address
• • @ovpn: the openvpn instance to search
• • @skb: the skb to retrieve the source transport address from
• • Return: a pointer to the peer if found or NULL otherwise
• */

+struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn,

• 			       struct sk_buff *skb)
  

+{

• struct ovpn_peer *peer = NULL;
• struct sockaddr_storage ss = { 0 };

nit: reverse x-mas tree order, please.

• if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss)))

• return NULL;
  

• if (ovpn->mode == OVPN_MODE_P2P)

• peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss);
  

• return peer;

+}

+/**

• • ovpn_peer_get_by_id_p2p - get peer by ID in a P2P instance
• • @ovpn: the openvpn instance to search
• • @peer_id: the ID of the peer to find
• • Return: the peer if found or NULL otherwise
• */

+static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_struct *ovpn,

• 				 u32 peer_id)
  

+{

• struct ovpn_peer *tmp, *peer = NULL;
• rcu_read_lock();
• tmp = rcu_dereference(ovpn->peer);
• if (likely(tmp && tmp->id == peer_id && ovpn_peer_hold(tmp)))

• peer = tmp;
  

• rcu_read_unlock();
• return peer;

+}

+/**

• • ovpn_peer_get_by_id - retrieve peer by ID
• • @ovpn: the openvpn instance to search
• • @peer_id: the unique peer identifier to match
• • Return: a pointer to the peer if found or NULL otherwise
• */

+struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id) +{

• struct ovpn_peer *peer = NULL;
• if (ovpn->mode == OVPN_MODE_P2P)

• peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id);
  

• return peer;

+}

+/**

• • ovpn_peer_add_p2p - add peer to related tables in a P2P instance
• • @ovpn: the instance to add the peer to
• • @peer: the peer to add
• • Return: 0 on success or a negative error code otherwise
• */

+static int ovpn_peer_add_p2p(struct ovpn_struct *ovpn, struct ovpn_peer *peer) +{

• struct ovpn_peer *tmp;
• spin_lock_bh(&ovpn->lock);
• /* in p2p mode it is possible to have a single peer only, therefore the

• * old one is released and substituted by the new one
  

• */
  

• tmp = rcu_dereference_protected(ovpn->peer,

• 			lockdep_is_held(&ovpn->lock));
  

• if (tmp) {

• tmp->delete_reason = OVPN_DEL_PEER_REASON_TEARDOWN;
  

• ovpn_peer_put(tmp);
  

• }
• rcu_assign_pointer(ovpn->peer, peer);

nit: the rcu_dereference_protected() + rcu_assign_pointer() calls can be replaced with a single rcu_replace_pointer() call.

• spin_unlock_bh(&ovpn->lock);
• return 0;

+}

+/**

• • ovpn_peer_add - add peer to the related tables
• • @ovpn: the openvpn instance the peer belongs to
• • @peer: the peer object to add
• • Assume refcounter was increased by caller
• • Return: 0 on success or a negative error code otherwise
• */

+int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) +{

• switch (ovpn->mode) {
• case OVPN_MODE_P2P:

• return ovpn_peer_add_p2p(ovpn, peer);
  

• default:

• return -EOPNOTSUPP;
  

• }

+}

+/**

• • ovpn_peer_del_p2p - delete peer from related tables in a P2P instance
• • @peer: the peer to delete
• • @reason: reason why the peer was deleted (sent to userspace)
• • Return: 0 on success or a negative error code otherwise
• */

+static int ovpn_peer_del_p2p(struct ovpn_peer *peer,

• 	     enum ovpn_del_peer_reason reason)
  

• __must_hold(&peer->ovpn->lock)

+{

• struct ovpn_peer *tmp;
• tmp = rcu_dereference_protected(peer->ovpn->peer,

• 			lockdep_is_held(&peer->ovpn->lock));
  

• if (tmp != peer) {

• DEBUG_NET_WARN_ON_ONCE(1);
  

nit: the above two lines can be simplified to:

if (DEBUG_NET_WARN_ON_ONCE(tmp != peer)) {

• if (tmp)
  

• 	ovpn_peer_put(tmp);
  

• return -ENOENT;
  

• }
• tmp->delete_reason = reason;
• RCU_INIT_POINTER(peer->ovpn->peer, NULL);
• ovpn_peer_put(tmp);
• return 0;

+}

+/**

• • ovpn_peer_release_p2p - release peer upon P2P device teardown
• • @ovpn: the instance being torn down
• */

+void ovpn_peer_release_p2p(struct ovpn_struct *ovpn) +{

• struct ovpn_peer *tmp;
• spin_lock_bh(&ovpn->lock);
• tmp = rcu_dereference_protected(ovpn->peer,

• 			lockdep_is_held(&ovpn->lock));
  

• if (tmp)

• ovpn_peer_del_p2p(tmp, OVPN_DEL_PEER_REASON_TEARDOWN);
  

• spin_unlock_bh(&ovpn->lock);

+}

+/**

• • ovpn_peer_del - delete peer from related tables
• • @peer: the peer object to delete
• • @reason: reason for deleting peer (will be sent to userspace)
• • Return: 0 on success or a negative error code otherwise
• */

+int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason) +{

• switch (peer->ovpn->mode) {
• case OVPN_MODE_P2P:

• return ovpn_peer_del_p2p(peer, reason);
  

• default:

• return -EOPNOTSUPP;
  

• }

+} diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h new file mode 100644 index 0000000000000000000000000000000000000000..6e0c6b14559de886d0677117f5a7ae029214e1f8 --- /dev/null +++ b/drivers/net/ovpn/peer.h @@ -0,0 +1,79 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload

• • Copyright (C) 2020-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#ifndef _NET_OVPN_OVPNPEER_H_ +#define _NET_OVPN_OVPNPEER_H_

+#include <net/dst_cache.h>

+/**

• • struct ovpn_peer - the main remote peer object
• • @ovpn: main openvpn instance this peer belongs to
• • @id: unique identifier
• • @vpn_addrs: IP addresses assigned over the tunnel
• • @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel
• • @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel
• • @dst_cache: cache for dst_entry used to send to peer
• • @bind: remote peer binding
• • @halt: true if ovpn_peer_mark_delete was called
• • @delete_reason: why peer was deleted (i.e. timeout, transport error, ..)
• • @lock: protects binding to peer (bind)
• • @refcount: reference counter
• • @rcu: used to free peer in an RCU safe way
• • @delete_work: deferred cleanup work, used to notify userspace
• */

+struct ovpn_peer {

• struct ovpn_struct *ovpn;
• u32 id;
• struct {

• struct in_addr ipv4;
  

• struct in6_addr ipv6;
  

• } vpn_addrs;
• struct dst_cache dst_cache;
• struct ovpn_bind __rcu *bind;
• bool halt;
• enum ovpn_del_peer_reason delete_reason;
• spinlock_t lock; /* protects bind */
• struct kref refcount;
• struct rcu_head rcu;
• struct work_struct delete_work;

+};

+/**

• • ovpn_peer_hold - increase reference counter
• • @peer: the peer whose counter should be increased
• • Return: true if the counter was increased or false if it was zero already
• */

+static inline bool ovpn_peer_hold(struct ovpn_peer *peer) +{

• return kref_get_unless_zero(&peer->refcount);

+}

+void ovpn_peer_release_kref(struct kref *kref);

+/**

• • ovpn_peer_put - decrease reference counter
• • @peer: the peer whose counter should be decreased
• */

+static inline void ovpn_peer_put(struct ovpn_peer *peer) +{

• kref_put(&peer->refcount, ovpn_peer_release_kref);

+}

+struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id); +int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer); +int ovpn_peer_del(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason); +void ovpn_peer_release_p2p(struct ovpn_struct *ovpn);

+struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn,

• 			       struct sk_buff *skb);
  

+struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id);

+#endif /* _NET_OVPN_OVPNPEER_H_ */

On 29.10.2024 12:47, Antonio Quartulli wrote:

This specific structure is used in the ovpn kernel module to wrap and carry around a standard kernel socket.

ovpn takes ownership of passed sockets and therefore an ovpn specific objects is attached to them for status tracking purposes.

Initially only UDP support is introduced. TCP will come in a later patch.

Signed-off-by: Antonio Quartulli antonio@openvpn.net

[...]

diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c new file mode 100644 index 0000000000000000000000000000000000000000..090a3232ab0ec19702110f1a90f45c7f10889f6f --- /dev/null +++ b/drivers/net/ovpn/socket.c @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload

• • Copyright (C) 2020-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#include <linux/net.h> +#include <linux/netdevice.h>

+#include "ovpnstruct.h" +#include "main.h" +#include "io.h" +#include "peer.h" +#include "socket.h" +#include "udp.h"

+static void ovpn_socket_detach(struct socket *sock) +{

• if (!sock)

• return;
  

• sockfd_put(sock);

+}

+/**

• • ovpn_socket_release_kref - kref_put callback
• • @kref: the kref object
• */

+void ovpn_socket_release_kref(struct kref *kref) +{

• struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,

• 				refcount);
  

• ovpn_socket_detach(sock->sock);
• kfree_rcu(sock, rcu);

+}

+static bool ovpn_socket_hold(struct ovpn_socket *sock) +{

• return kref_get_unless_zero(&sock->refcount);

Why do we need to wrap this kref acquiring call into the function. Why we cannot simply call kref_get_unless_zero() from ovpn_socket_get()?

+}

+static struct ovpn_socket *ovpn_socket_get(struct socket *sock) +{

• struct ovpn_socket *ovpn_sock;
• rcu_read_lock();
• ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
• if (!ovpn_socket_hold(ovpn_sock)) {

• pr_warn("%s: found ovpn_socket with ref = 0\n", __func__);
  

Should we be more specific here and print warning with netdev_warn(ovpn_sock->ovpn->dev, ...)?

And, BTW, how we can pick-up a half-destroyed socket?

• ovpn_sock = NULL;
  

• }
• rcu_read_unlock();
• return ovpn_sock;

+}

+static int ovpn_socket_attach(struct socket *sock, struct ovpn_peer *peer) +{

• int ret = -EOPNOTSUPP;
• if (!sock || !peer)

• return -EINVAL;
  

• if (sock->sk->sk_protocol == IPPROTO_UDP)

• ret = ovpn_udp_socket_attach(sock, peer->ovpn);
  

• return ret;

+}

+/**

• • ovpn_socket_new - create a new socket and initialize it
• • @sock: the kernel socket to embed
• • @peer: the peer reachable via this socket
• • Return: an openvpn socket on success or a negative error code otherwise
• */

+struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) +{

• struct ovpn_socket *ovpn_sock;
• int ret;
• ret = ovpn_socket_attach(sock, peer);
• if (ret < 0 && ret != -EALREADY)

• return ERR_PTR(ret);
  

• /* if this socket is already owned by this interface, just increase the

• * refcounter and use it as expected.
  

• *
  

• * Since UDP sockets can be used to talk to multiple remote endpoints,
  

• * openvpn normally instantiates only one socket and shares it among all
  

• * its peers. For this reason, when we find out that a socket is already
  

• * used for some other peer in *this* instance, we can happily increase
  

• * its refcounter and use it normally.
  

• */
  

• if (ret == -EALREADY) {

• /* caller is expected to increase the sock refcounter before
  

•  * passing it to this function. For this reason we drop it if
  

•  * not needed, like when this socket is already owned.
  

•  */
  

• ovpn_sock = ovpn_socket_get(sock);
  

• sockfd_put(sock);
  

• return ovpn_sock;
  

• }
• ovpn_sock = kzalloc(sizeof(*ovpn_sock), GFP_KERNEL);
• if (!ovpn_sock)

• return ERR_PTR(-ENOMEM);
  

• ovpn_sock->ovpn = peer->ovpn;
• ovpn_sock->sock = sock;
• kref_init(&ovpn_sock->refcount);
• rcu_assign_sk_user_data(sock->sk, ovpn_sock);
• return ovpn_sock;

+} diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h new file mode 100644 index 0000000000000000000000000000000000000000..5ad9c5073b085482da95ee8ebf40acf20bf2e4b3 --- /dev/null +++ b/drivers/net/ovpn/socket.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload

• • Copyright (C) 2020-2024 OpenVPN, Inc.
• • Author: James Yonan james@openvpn.net

• • Antonio Quartulli <antonio@openvpn.net>
    

• */

+#ifndef _NET_OVPN_SOCK_H_ +#define _NET_OVPN_SOCK_H_

+#include <linux/net.h> +#include <linux/kref.h> +#include <net/sock.h>

+struct ovpn_struct; +struct ovpn_peer;

+/**

• • struct ovpn_socket - a kernel socket referenced in the ovpn code
• • @ovpn: ovpn instance owning this socket (UDP only)
• • @sock: the low level sock object
• • @refcount: amount of contexts currently referencing this object
• • @rcu: member used to schedule RCU destructor callback
• */

+struct ovpn_socket {

• struct ovpn_struct *ovpn;
• struct socket *sock;
• struct kref refcount;
• struct rcu_head rcu;

+};

+void ovpn_socket_release_kref(struct kref *kref);

+/**

• • ovpn_socket_put - decrease reference counter
• • @sock: the socket whose reference counter should be decreased
• */

+static inline void ovpn_socket_put(struct ovpn_socket *sock) +{

• kref_put(&sock->refcount, ovpn_socket_release_kref);

+}

+struct ovpn_socket *ovpn_socket_new(struct socket *sock,

• 		    struct ovpn_peer *peer);
  

+#endif /* _NET_OVPN_SOCK_H_ */ diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c new file mode 100644 index 0000000000000000000000000000000000000000..c10474d252e19a0626d17a6f5dd328a5e5811551 --- /dev/null +++ b/drivers/net/ovpn/udp.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload

• • Copyright (C) 2019-2024 OpenVPN, Inc.
• • Author: Antonio Quartulli antonio@openvpn.net
• */

+#include <linux/netdevice.h> +#include <linux/socket.h> +#include <net/udp.h>

+#include "ovpnstruct.h" +#include "main.h" +#include "socket.h" +#include "udp.h"

+/**

• • ovpn_udp_socket_attach - set udp-tunnel CBs on socket and link it to ovpn
• • @sock: socket to configure
• • @ovpn: the openvp instance to link
• • After invoking this function, the sock will be controlled by ovpn so that
• • any incoming packet may be processed by ovpn first.
• • Return: 0 on success or a negative error code otherwise
• */

+int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn) +{

• struct ovpn_socket *old_data;
• int ret = 0;
• /* sanity check */
• if (sock->sk->sk_protocol != IPPROTO_UDP) {

The function will be called only for a UDP socket. The caller makes sure this is truth. So, why do we need this check?

• DEBUG_NET_WARN_ON_ONCE(1);
  

• return -EINVAL;
  

• }
• /* make sure no pre-existing encapsulation handler exists */
• rcu_read_lock();
• old_data = rcu_dereference_sk_user_data(sock->sk);
• if (!old_data) {

• /* socket is currently unused - we can take it */
  

• rcu_read_unlock();
  

• return 0;
  

• }
• /* socket is in use. We need to understand if it's owned by this ovpn

• * instance or by something else.
  

• * In the former case, we can increase the refcounter and happily
  

• * use it, because the same UDP socket is expected to be shared among
  

• * different peers.
  

• *
  

• * Unlikely TCP, a single UDP socket can be used to talk to many remote
  

• * hosts and therefore openvpn instantiates one only for all its peers
  

• */
  

• if ((READ_ONCE(udp_sk(sock->sk)->encap_type) == UDP_ENCAP_OVPNINUDP) &&

•    old_data->ovpn == ovpn) {
  

• netdev_dbg(ovpn->dev,
  

• 	   "%s: provided socket already owned by this interface\n",
  

• 	   __func__);
  

Why do we need the function name being printed here?

• ret = -EALREADY;
  

• } else {

• netdev_err(ovpn->dev,
  

• 	   "%s: provided socket already taken by other user\n",
  

• 	   __func__);
  

The same comment regarding the function name printing.

And why 'error' level? There is a few ways to fall into this case and each of them implies a user-space screw up. But why we consider these user-space screw ups our (kernel) problem? I suggesting to reduce level at least to 'warning' or maybe even 'notice'. See level definitions in include/linux/kern_levels.h

• ret = -EBUSY;
  

• }
• rcu_read_unlock();
• return ret;

+}

-- Sergey