On Wed, 03 Jun 2026 19:53:58 +0200, Sabrina Dubroca <[email protected]> wrote: > Sorry Ralf, I wanted to reply to this earlier.
No worries, thanks for taking another look. > > 2026-05-28, 22:44:57 +0200, Ralf Lici wrote: > > On 5/28/26 19:15, Sabrina Dubroca wrote: > > > Hi Ralf, > > > > > > 2026-05-26, 14:45:40 +0200, Ralf Lici wrote: > > > > ovpn accepts a userspace-provided socket and a peer remote endpoint > > > > through netlink. For UDP peers, the remote endpoint family selects the > > > > transmit path used later by ovpn_udp_output. > > > > > > > > An IPv4 UDP socket cannot be used with an IPv6 remote endpoint. If > > > > accepted, the transmit path may reach IPv6 routing and UDP tunnel > > > > helpers with an IPv4 socket. Similarly, an IPv6-only UDP socket cannot > > > > be used with an IPv4 remote endpoint. Otherwise ovpn may enter the IPv4 > > > > UDP transmit path, where IPv4 routing treats the unspecified source as a > > > > normal source-selection request and may choose an IPv4 source address, > > > > bypassing the check that udpv6_sendmsg would normally enforce. > > > > > > > > Parse the remote endpoint once in the peer new/set paths and reject UDP > > > > remotes when the provided socket cannot send to them. Pass the parsed > > > > endpoint into the common peer modify helper so the validation and the > > > > stored endpoint use the same normalized sockaddr. > > > > > > Since this can be changed at any time by userspace, I'm not convinced > > > checking it at setup time is very useful. > > > > > > AFAIU, the next patch fixes the actual issue. This one seems to mainly > > > help userspace avoid drops in case they incorrectly set up their > > > socket (as long as they do that in the "right order", ie all > > > setsockopt before passing the socket to ovpn). > > > > > > > Hi Sabrina, > > > > Yes, your understanding is correct: the new/set peer netlink messages give > > us a chance to detect a broken configuration at setup time, but we cannot > > prevent userspace from changing the socket properties later on. In this > > sense this patch is not the actual correctness fix. > > Thanks for confirming. > > > The next patch enforces the real gate on TX, but the error is ultimately > > swallowed by ovpn_udp_send_skb after freeing the skb. Antonio suggested > > adding a netdev_warn_once on these new socket-based failure points > > (including the sport == 0 case), and I think that would be useful too. But, > > aside from that, I don't think there's a clean way to report an error to > > userspace once such misconfigurations are hit from the TX path. > > I think adding stats counters could help (probably per-socket, and > maybe a separate one for each case in patch 4/4). A _once/_ratelimited > message also sounds reasonable. > I agree that counters would improve observability, and I am not opposed to adding them later if we find they are useful in practice. For this series, though, I am not sure they are worth adding UAPI for (netlink operations are currently all peer-related). These drops are not expected to be normal packet-loss events: they mean userspace handed ovpn a socket, then left it in a state where that peer cannot use it anymore. In that case the most useful immediate signal is the reason why the peer became unusable, rather than a counter of how many packets were already dropped because of it. Antonio and I discussed this more in depth and our plan for the respin is to delete the peer with TRANSPORT_ERROR via deferred work for socket/remote address family mismatches, instead of keeping a peer around that can no longer transmit. We would also add a _ratelimited warning on those TX-side failures (_once appears to be once per call site, so one bad peer/socket would silence the warning for all later cases). For sport == 0 I'd keep dropping and warning for now and think about a more systematic approach in net-next. For example, we could reject this at socket setup time and override the UDP socket protocol callbacks to intercept and prevent disconnect (though that needs some caution because IPV6_ADDRFORM can apparently switch the sk_prot back to udp_prot). Does that make sense to you? > > So the rationale for the current patch is to at least report the specific > > socket/remote mismatch through extack when it is already present in the > > netlink request, and avoid installing a peer that would immediately drop > > traffic. > > Yup, that makes sense. > > > That said, I see that the commit message may be selling the patch as solving > > the issue at its core. I can reword it to make clear that this is only early > > validation/reporting, or drop the patch if you think the value is too low. > > A small note to explain that this patch doesn't fully solve the issue > would be useful. Sure, I will add it. > > This patch is a bit large, but I guess it would solve the issue > entirely for "well-behaved" userspace (not doing weird setsockopts > after "gifting" the socket to the kernel module). Is the current > openvpn client hitting the code in next patch once this one is applied > (ie finishing config, and then doing V6ONLY/ADDRFORM afterwards)? No, luckily current openvpn userspace does not change these socket properties after handing the socket to ovpn. The setup time checks should therefore be enough for well-behaved userspace. The TX-side checks are still needed because the socket remains mutable after attach. > > -- > Sabrina > -- Ralf Lici Mandelbit Srl _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
