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@xxxxxxxxxxx/
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.
