netdev - Re: [PATCH net-next v18 20/25] ovpn: implement peer add/get/dump/delete via netlink

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: <62213f34-b5c4-46a9-b634-4ce6adbf9786@openvpn.net>
Date: Tue, 21 Jan 2025 10:48:30 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: ryazanov.s.a@...il.com, netdev@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v18 20/25] ovpn: implement peer
 add/get/dump/delete via netlink

On 21/01/2025 10:39, Sabrina Dubroca wrote:
> 2025-01-20, 11:45:55 +0100, Antonio Quartulli wrote:
>> On 20/01/2025 11:09, Sabrina Dubroca wrote:
>>> 2025-01-19, 14:12:05 +0100, Antonio Quartulli wrote:
>>>> On 17/01/2025 18:12, Sabrina Dubroca wrote:
>>>>> 2025-01-17, 13:59:35 +0100, Antonio Quartulli wrote:
>>>>>> On 17/01/2025 12:48, Sabrina Dubroca wrote:
>>>>>>> 2025-01-13, 10:31:39 +0100, Antonio Quartulli wrote:
>>>>>>>>      int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>>>>      {
>>>>>>>> -	return -EOPNOTSUPP;
>>>>>>>> +	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
>>>>>>>> +	struct ovpn_priv *ovpn = info->user_ptr[0];
>>>>>>>> +	struct ovpn_socket *ovpn_sock;
>>>>>>>> +	struct socket *sock = NULL;
>>>>>>>> +	struct ovpn_peer *peer;
>>>>>>>> +	u32 sockfd, peer_id;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	/* peers can only be added when the interface is up and running */
>>>>>>>> +	if (!netif_running(ovpn->dev))
>>>>>>>> +		return -ENETDOWN;
>>>>>>>
>>>>>>> Since we're not under rtnl_lock here, the device could go down while
>>>>>>> we're creating this peer, and we may end up with a down device that
>>>>>>> has a peer anyway.
>>>>>>
>>>>>> hmm, indeed. This means we must hold the rtnl_lock to prevent ending up in
>>>>>> an inconsistent state.
>>>>>>
>>>>>>>
>>>>>>> I'm not sure what this (and the peer flushing on NETDEV_DOWN) is
>>>>>>> trying to accomplish. Is it a problem to keep peers when the netdevice
>>>>>>> is down?
>>>>>>
>>>>>> This is the result of my discussion with Sergey that started in v23 5/23:
>>>>>>
>>>>>> https://lore.kernel.org/r/netdev/20241029-b4-ovpn-v11-5-de4698c73a25@openvpn.net/
>>>>>>
>>>>>> The idea was to match operational state with actual connectivity to peer(s).
>>>>>>
>>>>>> Originally I wanted to simply kee the carrier always on, but after further
>>>>>> discussion (including the meaning of the openvpn option --persist-tun) we
>>>>>> agreed on following the logic where an UP device has a peer connected (logic
>>>>>> is slightly different between MP and P2P).
>>>>>>
>>>>>> I am not extremely happy with the resulting complexity, but it seemed to be
>>>>>> blocker for Sergey.
>>>>>
>>>>> [after re-reading that discussion with Sergey]
>>>>>
>>>>> I don't understand why "admin does 'ip link set tun0 down'" means "we
>>>>> should get rid of all peers. For me the carrier situation goes the
>>>>> other way: no peer, no carrier (as if I unplugged the cable from my
>>>>> ethernet card), and it's independent of what the user does (ip link
>>>>> set XXX up/down). You have that with netif_carrier_{on,off}, but
>>>>> flushing peers when the admin does "ip link set tun0 down" is separate
>>>>> IMO.
>>>>
>>>> The reasoning was "the user is asking the VPN to go down - it should be
>>>> assumed that from that moment on no VPN traffic whatsoever should flow in
>>>> either direction".
>>>> Similarly to when you bring an Eth interface dwn - the phy link goes down as
>>>> well.
>>>>
>>>> Does it make sense?
>>>
>>> I'm not sure. If I turn the ovpn interface down for a second, the
>>> peers are removed. Will they come back when I bring the interface back
>>> up?  That'd have to be done by userspace (which could also watch for
>>> the DOWN events and tell the kernel to flush the peers) - but some of
>>> the peers could have timed out in the meantime.
>>>
>>> If I set the VPN interface down, I expect no packets flowing through
>>> that interface (dropping the peers isn't necessary for that), but all
>>> non-data (key exchange etc sent by openvpn's userspace) should still
>>> go through, and IMO peer keepalive fits in that "non-data" category.
>>
>> This was my original thought too and my original proposal followed this idea
>> :-)
>>
>> However Sergey had a strong opinion about "the user expect no traffic
>> whatsoever".
>>
>> I'd be happy about going again with your proposed approach, but I need to be
>> sure that on the next revision nobody will come asking to revert this logic
>> again :(
> 
> Sure.
> 
>>> What does openvpn currently do if I do
>>>       ip link set tun0 down ; sleep 5 ; ip link set tun0 up
>>> with a tuntap interface?
>>
>> I think nothing happens, because userspace doesn't monitor the netdev
>> status. Therefore, unless tun closed the socket (which I think it does only
>> when the interface is destroyed), userspace does not even realize that the
>> interface went down.
> 
> So if this behavior changes once users switch from tuntap to ovpn,
> they may be surprised/unhappy.
> 

I agree here too.
I'll go back to keeping peers connected even if the iface goes down then.

Thanks

-- 
Antonio Quartulli
OpenVPN Inc.