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
