On 05/03/2024 20:39, Jakub Kicinski wrote:

On Tue, 5 Mar 2024 17:23:25 +0100 Andrew Lunn wrote:

quoted

quoted

quoted

quoted

+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.

I don't really see why GENL vs RTNL makes a difference. The reply to
the request can contain the ifindex of the newly created interface. If
you set the name to "ovpn%d" before calling register_netdevice() the
kernel will find the next free unique ovpn interface name, race
free. So you could have multiple openvpn daemon running, and not have
to worry about races when creating interfaces.

Jakub has been raising questions about this recently for another
patchset. He might comment on this.

FWIW using ifindex for most ops sounds like the right way to go.
Passing the name to create sounds fine, but as Andrew said, we
should default to "ovpn%d" instead of forcing the user to specify
the name (and you can echo back the allocated name in the reply
to OVPN_CMD_NEW_IFACE).

Ok, it definitely makes sense and actually makes userspace code simpler.

Somewhat related - if you require an attr - GENL_REQ_ATTR_CHECK(),
it does the extact setting for you.

ok, will check it out!

quoted

quoted

quoted

quoted

+	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.

Right, so this in general makes sense. The only question i have now
is, should you be using rtnl_link_stats64. That is the standard
structure for interface statistics.

@Andrew: do you see how I could return/send this object per-peer to 
userspace?
I think the whole interface stats logic is based on the 
one-stats-per-device concept. Hence, I thought it was meaningful to just 
send my own stats via netlink.

quoted

Again, Jakub likes to comment about statistics...

And in general, maybe add more comments. This is the UAPI, it needs to
be clear and unambiguous. Documentation/networking/ethtool-netlink.rst.

Or put enough docs in the YAML spec as those comments get rendered in
the uAPI header and in HTML docs :)

Or I'll do both. In any case, I will add more documentation!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.