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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d52c6ff5-dd0d-41d1-be6f-272d58ccf010@lunn.ch>
Date: Tue, 5 Mar 2024 17:23:25 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Antonio Quartulli <antonio@...nvpn.net>
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

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

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.

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

       Andrew