Re: [PATCH net-next v18 15/25] ovpn: implement multi-peer support

[Sabrina Dubroca]

2025-01-13, 10:31:34 +0100, Antonio Quartulli wrote:
>  static int ovpn_newlink(struct net *src_net, struct net_device *dev,
>  			struct nlattr *tb[], struct nlattr *data[],
>  			struct netlink_ext_ack *extack)
>  {
>  	struct ovpn_priv *ovpn = netdev_priv(dev);
>  	enum ovpn_mode mode = OVPN_MODE_P2P;
> +	int err;
>  
>  	if (data && data[IFLA_OVPN_MODE]) {
>  		mode = nla_get_u8(data[IFLA_OVPN_MODE]);
> @@ -136,6 +183,10 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev,
>  	ovpn->mode = mode;
>  	spin_lock_init(&ovpn->lock);
>  
> +	err = ovpn_mp_alloc(ovpn);

If register_netdevice fails, ovpn->peers won't get freed in some cases
(only if we got past ndo_init). So this should go into ndo_init.

> +	if (err < 0)
> +		return err;
> +
>  	/* turn carrier explicitly off after registration, this way state is
>  	 * clearly defined
>  	 */


[...]
> +static int ovpn_peer_add_mp(struct ovpn_priv *ovpn, struct ovpn_peer *peer)
> +{
[...]
> +	hlist_add_head_rcu(&peer->hash_entry_id,
> +			   ovpn_get_hash_head(ovpn->peers->by_id, &peer->id,
> +					      sizeof(peer->id)));
> +
> +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
> +		nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr,
> +					   &peer->vpn_addrs.ipv4,
> +					   sizeof(peer->vpn_addrs.ipv4));
> +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead);
> +	}
> +
> +	if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) {
> +		nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr,
> +					   &peer->vpn_addrs.ipv6,
> +					   sizeof(peer->vpn_addrs.ipv6));
> +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead);
> +	}

You can't add hash_entry_addr4 and hash_entry_addr6 to the same
hashtable.  ovpn_peer_get_by_vpn_addr{4,6} use those fields as
"member" for hlist_nulls_for_each_entry_rcu, so container_of (in
hlist_nulls_entry) will return a "peer" that's not really a peer
object in memory when we walk past an entry for the wrong address
family:
  container_of(peer_v4->hash_entry_addr4, struct ovpn_peer, hash_entry_addr6)
or
  container_of(peer_v6->hash_entry_addr6, struct ovpn_peer, hash_entry_addr4)

(probably not visible in testing since we'll never really get 2 peers
(and of different families) into the same bucket, and then also get
them to pass the addr_equal test in ovpn_peer_get_by_vpn_addr{4,6}.
easiest way to try to trigger problems would be making the hashtable
single bucket, and even then...)

-- 
Sabrina