Re: [PATCH ovpn net 3/4] ovpn: reject UDP remotes incompatible with socket family

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