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

[Sabrina Dubroca <sd@queasysnail.net>]

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.

-- 
Sabrina