Message-ID: <f546e063-a69d-4c77-81d2-045acf7e6e4f@openvpn.net>
Date: Tue, 5 Mar 2024 16:47:09 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next v2 03/22] ovpn: add basic netlink support

On 04/03/2024 22:20, Andrew Lunn wrote:
> On Mon, Mar 04, 2024 at 04:08:54PM +0100, Antonio Quartulli wrote:
>> This commit introduces basic netlink support with
>> registration/unregistration functionalities and stub pre/post-doit.
>>
>> More importantly it introduces the UAPI header file that contains
>> the attributes that are inteded to be used by the netlink API
> 
> intended.
> 
>> implementation.
>>
>> For convience, packet.h is also added containing some macros about
>> the OpenVPN packet format.
>>
>> +/** KEYDIR policy. Can be used for configuring an encryption and a decryption key */
>> +static const struct nla_policy ovpn_nl_policy_keydir[NUM_OVPN_A_KEYDIR] = {
>> +	[OVPN_A_KEYDIR_CIPHER_KEY] = NLA_POLICY_MAX_LEN(U8_MAX),
> 
> I don't know netlink that well. Is this saying keys are limited to 256
> bytes? How future proof is that? I'm not a crypto person, but
> symmetric algorithms, e.g. AES, seem to have reasonably short keys, 32
> bytes, so this seems O.K, to me.

256 bytes should be reasonably large. I don't see anything beyond this 
size appearing anytime soon or at all.

> 
>> +	[OVPN_A_KEYDIR_NONCE_TAIL] = NLA_POLICY_EXACT_LEN(NONCE_TAIL_SIZE),
>> +};
>> +
>> +/** KEYCONF policy */
>> +static const struct nla_policy ovpn_nl_policy_keyconf[NUM_OVPN_A_KEYCONF] = {
>> +	[OVPN_A_KEYCONF_SLOT] = NLA_POLICY_RANGE(NLA_U8, __OVPN_KEY_SLOT_FIRST,
>> +						 NUM_OVPN_KEY_SLOT - 1),
>> +	[OVPN_A_KEYCONF_KEY_ID] = { .type = NLA_U8 },
> 
> Is that 256 keys globally, or just associated to one session?

This is specific to one peer, however, the OpenVPN protocol uses IDs up 
7, therefore U8 is just the smallest unit I could use to fit those few 
values.

> 
>> +	[OVPN_A_KEYCONF_CIPHER_ALG] = { .type = NLA_U16 },
>> +	[OVPN_A_KEYCONF_ENCRYPT_DIR] = NLA_POLICY_NESTED(ovpn_nl_policy_keydir),
>> +	[OVPN_A_KEYCONF_DECRYPT_DIR] = NLA_POLICY_NESTED(ovpn_nl_policy_keydir),
>> +};
>> +
> 
>> +/** Generic message container policy */
>> +static const struct nla_policy ovpn_nl_policy[NUM_OVPN_A] = {
>> +	[OVPN_A_IFINDEX] = { .type = NLA_U32 },
>> +	[OVPN_A_IFNAME] = NLA_POLICY_MAX_LEN(IFNAMSIZ),
> 
> Generally, ifnames are not passed around, only ifindex. An interface
> can have multiple names:
> 
> 12: enlr0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq master br0 state DOWN group default qlen 1000
>      link/ether 3c:ec:ef:7e:0a:90 brd ff:ff:ff:ff:ff:ff
>      altname enp183s0f2
>      altname eno7
> 
> It is better to let userspace figure out the name from the index,
> since the name is mostly a user space concept.

This is strictly related to your next question.
Please see my answer below.

> 
>> +	[OVPN_A_MODE] = NLA_POLICY_RANGE(NLA_U8, __OVPN_MODE_FIRST,
>> +					 NUM_OVPN_MODE - 1),
>> +	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_nl_policy_peer),
>> +};
> 
>> +static int ovpn_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>> +			 struct genl_info *info)
>> +{
>> +	struct net *net = genl_info_net(info);
>> +	struct net_device *dev;
>> +
>> +	/* the OVPN_CMD_NEW_IFACE command is different from the rest as it
>> +	 * just expects an IFNAME, while all the others expect an IFINDEX
>> +	 */
> 
> Could you explain that some more. In general, the name should not
> matter to the kernel, udev/systemd might rename it soon after creation
> etc. If it gets moved into a network namespace it might need renaming
> etc.

In a previous discussion it was agreed that we should create ovpn 
interfaces via GENL and not via RTNL.

For this reason ovpn needs userspace to send the name to give the 
interface upon creation. This name is just passed to the networking 
stack upon creation/registration, but it is not stored anywhere else.

Subsequent netlink calls are then all performed by passing an ifindex.

Hence, OVPN_CMD_NEW_IFACE is the only GENL command that required the 
IFNAME to be specified.

Does it make sense?


> 
>> +enum ovpn_nl_peer_attrs {
>> +	OVPN_A_PEER_UNSPEC = 0,
>> +	OVPN_A_PEER_ID,
>> +	OVPN_A_PEER_RX_STATS,
>> +	OVPN_A_PEER_TX_STATS,
> 
> Probably answered later in the patch series: What sort of statistics
> do you expect here. Don't overlap any of the normal network statistics
> with this here, please use the existing kAPIs for those. Anything you
> return here need to be very specific to ovpn.

Actually you found a leftover from an old approach.
OVPN_A_PEER_RX_STATS and OVPN_A_PEER_TX_STATS shall be removed.

The actual stats we store are those below:

> 
>> +	OVPN_A_PEER_VPN_RX_BYTES,
>> +	OVPN_A_PEER_VPN_TX_BYTES,
>> +	OVPN_A_PEER_VPN_RX_PACKETS,
>> +	OVPN_A_PEER_VPN_TX_PACKETS,
>> +	OVPN_A_PEER_LINK_RX_BYTES,
>> +	OVPN_A_PEER_LINK_TX_BYTES,
>> +	OVPN_A_PEER_LINK_RX_PACKETS,
>> +	OVPN_A_PEER_LINK_TX_PACKETS,
> 
> How do these differ to standard network statistics? e.g. what is in
> /sys/class/net/*/statistics/ ?

The first difference is that these stats are per-peer and not 
per-device. Behind each device there might be multiple peers connected.

This way ovpn is able to tell how much data was sent/received by every 
single connected peer.

LINK and VPN store different values.
LINK stats are recorded at the transport layer (before decapsulation or 
after encapsulation), while VPN stats are recorded at the tunnel layer 
(after decapsulation or before encapsulation).

I didn't see how to convey the same information using the standard 
statistics.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists