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 for Android: free password hash cracker in your pocket

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

Message-ID: <5f7f088c-426a-493b-9840-02f3003a7381@openvpn.net>
Date: Wed, 6 Mar 2024 15:46:37 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.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 05/03/2024 20:39, Jakub Kicinski wrote:
> 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).
> 

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!

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

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