linux-kernel - Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines

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: <0f19828c-9808-427f-b620-fd3bc9f2e5db@openvpn.net>
Date: Fri, 15 Nov 2024 15:03:37 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: 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>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 04/23] ovpn: add basic interface
 creation/destruction/management routines

On 10/11/2024 21:42, Sergey Ryazanov wrote:
> Missed the most essential note regarding this patch :)
> 
> On 29.10.2024 12:47, Antonio Quartulli wrote:
>> +static int ovpn_net_open(struct net_device *dev)
>> +{
>> +    netif_tx_start_all_queues(dev);
>> +    return 0;
>> +}
>> +
>> +static int ovpn_net_stop(struct net_device *dev)
>> +{
>> +    netif_tx_stop_all_queues(dev);
> 
> Here we stop a user generated traffic in downlink. Shall we take care 
> about other kinds of traffic: keepalive, uplink?

Keepalive is "metadata" and should continue to flow, regardless of 
whether the user interface is brought down.

Uplink traffic directed to *this* device should just be dropped at 
delivery time.

Incoming traffic directed to other peers will continue to work.

> 
> I believe we should remove all the peers here or at least stop the 
> keepalive generation. But peers removing is better since 
> administratively down is administratively down, meaning user expected 
> full traffic stop in any direction. And even if we only stop the 
> keepalive generation then peer(s) anyway will destroy the tunnel on 
> their side.

Uhm, I don't think the user expects all "protocol" traffic (and client 
to client) to stop by simply bringing down the interface.

> 
> This way we even should not care about peers removing on the device 
> unregistering. What do you think?

I think you are now mixing data plane and control plane.

The fact that the user is stopping payload traffic does not imply we 
want to stop the VPN.
The user may just be doing something with the interface (and on an MP 
node client-to-client traffic will still continue to flow).

This would also be a non-negligible (and user faving) change in 
behaviour compared to the current openvpn implementation.

Thanks for your input though, I can imagine coming from different angles 
things may look not the same.

Regards,

> 
>> +    return 0;
>> +}

-- 
Antonio Quartulli
OpenVPN Inc.