netdev - 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: <709d415d-859d-4342-80b7-908c04b28621@gmail.com>
Date: Tue, 19 Nov 2024 05:08:42 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
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 15.11.2024 16:03, Antonio Quartulli wrote:
> 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.

How it's possible? AFAIU, the module uses the kernel IP routing 
subsystem. Putting the interface down will effectively block a 
client-to-client packet to reenter the interface.

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

It's not about previous implementation, it's about the interface 
management procedures. I just cannot image how the proposed approach can 
be aligned with RFC 2863 section 3.1.13. IfAdminStatus and IfOperStatus.

And if we are talking about a user experience, I cannot imagine my WLAN 
interface maintaining a connection to the access point after shutting it 
down. Or even better, a WLAN interface in the AP mode still forwarding 
traffic between wireless clients. Or a bridge interface switching 
traffic between ports and sending STP frames.

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

I believe nobody will mind if a userspace service will do a failover to 
continue serving connected clients. But from the kernel perspective, 
when user says 'ip link set down' the party is over.

--
Sergey