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

[Antonio Quartulli]

On 03/02/2025 00:00, Sabrina Dubroca wrote:
> 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.

This is something I was investigating during the weekend.
You just confirmed my suspicion. Thanks.

Will move it to 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...)

cr0p.
This has been lurking here for so long.
You can imagine what the original idea was, but I didn't see that issue 
with walking the list.

I have been wondering for so long if using just one hashtable could hide 
some problem, but until now I couldn't come up with any.

Ans yes, this is close to impossible to trigger, but yet, it is not correct.

Will add an extra hashtable in order to separate v4 from v6.

In the background we're already working on moving to rhashtables and 
there we were forced to have v4 and v6 separate anyway.

Thanks a lot.
Regards,

>

--
Antonio Quartulli
OpenVPN Inc.