Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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;
+}
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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:
+ *
+ * 0005 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,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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
---
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
#include
#include
+#include
+#include
#include
-#include
+#include
#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
+#include
+#include
/**
* struct ovpn_struct - per ovpn interface state
* @dev: the actual netdev representing the tunnel
* @dev_tracker: reference tracker fo
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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:
+ *
+ * 0005 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
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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:
+ *
+ *0005 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,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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:
+ *
+ *0005 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
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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:
> > + *
> > + *0005 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?").
--
Sabrina
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
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;
+}
Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines
On 29.10.2024 12:47, Antonio Quartulli wrote:
Add basic infrastructure for handling ovpn interfaces.
Signed-off-by: Antonio Quartulli
---
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
#include
#include
+#include
+#include
#include
-#include
+#include
#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
