Re: [Openvpn-devel] [PATCH ovpn net 2/5] ovpn: reject duplicate peer VPN addresses

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