On Fri, 14 Aug 2026 11:52:20 +0200, Sabrina Dubroca <[email protected]>
wrote:
> 2026-07-29, 17:37:40 +0200, Ralf Lici wrote:
> > In MP mode, ovpn uses the peer VPN addresses as lookup keys for
> > selecting the peer that should receive an outgoing tunnel packet.
> > However, the netlink peer configuration path does not currently reject
> > duplicate VPN addresses.
> >
> > If two peers are configured with the same VPN address, both can be
> > inserted in the VPN address hash table and lookups return whichever peer
> > is found first. This makes peer selection ambiguous and dependent on
> > hash insertion order.
> >
> > Reject peer creation or update when the resulting VPN address is already
> > assigned to another peer. Ignore unspecified addresses because those are
> > not inserted in the VPN address hash tables.
>
> Maybe mention that it's a user-visible change of behavior, but it
> doesn't matter since that setup has never worked.
>
>
Will do.
> > @@ -522,22 +524,41 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct
> > genl_info *info)
> > rcu_read_unlock();
> >
> > spin_lock_bh(&ovpn->lock);
> > - ret = ovpn_nl_peer_modify(peer, info, attrs);
> > - if (ret < 0) {
> > - spin_unlock_bh(&ovpn->lock);
> > - ovpn_peer_put(peer);
> > - return ret;
> > +
> > + vpn_addr4 = peer->vpn_addrs.ipv4;
> > + vpn_addr6 = peer->vpn_addrs.ipv6;
> > +
> > + if (attrs[OVPN_A_PEER_VPN_IPV4])
> > + vpn_addr4.s_addr = nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> > +
> > + if (attrs[OVPN_A_PEER_VPN_IPV6])
> > + vpn_addr6 = nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> > +
> > + /* reject peer with conflicting VPN address */
> > + if ((attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6]) &&
> > + ovpn_peer_vpn_addr_conflict(ovpn, peer, &vpn_addr4, &vpn_addr6)) {
> > + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > + "VPN IP is already assigned to another
> > peer");
> > + ret = -EADDRINUSE;
> > + goto unlock;
> > }
>
> nit: maybe splitting conflict() into conflict{4,6}() would be a bit
> cleaner? passing addresses of 2 families feels kind of strange.
>
> if (attrs[OVPN_A_PEER_VPN_IPV4]) {
> struct in_addr vpn_addr4 = {
> .s_addr = nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]),
> };
> if (ovpn_peer_vpn_addr_conflict4(ovpn, peer, &vpn_addr4))
> goto addr_conflict;
> }
>
>
> And then conflict* looks slightly nicer too:
>
> bool ovpn_peer_vpn_addr_conflict4(struct ovpn_priv *ovpn,
> const struct ovpn_peer *peer,
> const struct in_addr *addr)
> {
> struct ovpn_peer *tmp = NULL;
>
> lockdep_assert_held(&ovpn->lock);
>
> /* we don't hash INADDR_ANY, no conflict in that case */
> if (addr->s_addr != htonl(INADDR_ANY))
> tmp = ovpn_peer_get_by_vpn_addr4(ovpn, addr->s_addr);
>
> return tmp && tmp != peer;
> }
>
Yes, it's a nice refactoring. I'll apply it to v2.
--
Ralf Lici
Mandelbit Srl
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
