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

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

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