Re: [PATCH net-next v2 03/22] ovpn: add basic netlink support

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 5 Mar 2024 17:23:25 +0100 Andrew Lunn wrote:
> > > > +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).

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

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