Message-ID: <75ff57a6-dcd8-47f7-99bf-f46a1daee4b0@openvpn.net>
Date: Tue, 28 May 2024 22:09:37 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
 Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 15/24] ovpn: implement peer lookup logic

On 28/05/2024 18:42, Sabrina Dubroca wrote:
> 2024-05-06, 03:16:28 +0200, Antonio Quartulli wrote:
>> +static struct in6_addr ovpn_nexthop_from_skb6(struct sk_buff *skb)
>> +{
>> +	struct rt6_info *rt = (struct rt6_info *)skb_rtable(skb);
> 
> skb_rt6_info?

Yes! I have been looking for this guy all over the place in 
sk_buff.h....it was just in another header :) thanks!

> 
>> +
>> +	if (!rt || !(rt->rt6i_flags & RTF_GATEWAY))
>> +		return ipv6_hdr(skb)->daddr;
>> +
>> +	return rt->rt6i_gateway;
>> +}
>> +
>> +/**
>> + * ovpn_peer_get_by_vpn_addr4 - retrieve peer by its VPN IPv4 address
>> + * @head: list head to search
>> + * @addr: VPN IPv4 to use as search key
>> + *
>> + * Return: the peer if found or NULL otherwise
> 
> The doc for all those ovpn_peer_get_* functions could indicate that on
> success, a reference on the peer is held.

ACK

> 
> 
> [...]
>> +static struct ovpn_peer *ovpn_peer_get_by_vpn_addr6(struct hlist_head *head,
>> +						    struct in6_addr *addr)
>> +{
>> +	struct ovpn_peer *tmp, *peer = NULL;
>> +	int i;
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(tmp, head, hash_entry_addr6) {
>> +		for (i = 0; i < 4; i++) {
>> +			if (addr->s6_addr32[i] !=
>> +			    tmp->vpn_addrs.ipv6.s6_addr32[i])
>> +				continue;
>> +		}
> 
> ipv6_addr_equal

Thanks

> 
> [...]
>> +	default:
>> +		return NULL;
>> +	}
>> +
>> +	index = ovpn_peer_index(ovpn->peers.by_transp_addr, &ss, sa_len);
>> +	head = &ovpn->peers.by_transp_addr[index];
> 
> Maybe worth adding a get_bucket helper (with a better name :)) instead
> of ovpn_peer_index, since all uses of ovpn_peer_index are followed by
> a "head = TBL[index]" (or direct use in some hlist iterator), but the
> index itself is not used later on, only the bucket.

yup, good idea

> 
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(tmp, head, hash_entry_transp_addr) {
>> +		found = ovpn_peer_transp_match(tmp, &ss);
>> +		if (!found)
> 
> nit: call ovpn_peer_transp_match directly and drop the found variable

ACK.
I presume it's a leftover from the past, otherwise it wouldn't make much 
sense.

> 
>> +			continue;
>> +
>> +		if (!ovpn_peer_hold(tmp))
>> +			continue;
>> +
>> +		peer = tmp;
>> +		break;
>> +	}
>> +	rcu_read_unlock();
>>   
>>   	return peer;
>>   }
>> @@ -303,10 +427,28 @@ static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_struct *ovpn,
>>   
>>   struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id)
>>   {
>> -	struct ovpn_peer *peer = NULL;
>> +	struct ovpn_peer *tmp, *peer = NULL;
>> +	struct hlist_head *head;
>> +	u32 index;
>>   
>>   	if (ovpn->mode == OVPN_MODE_P2P)
>> -		peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id);
>> +		return ovpn_peer_get_by_id_p2p(ovpn, peer_id);
>> +
>> +	index = ovpn_peer_index(ovpn->peers.by_id, &peer_id, sizeof(peer_id));
>> +	head = &ovpn->peers.by_id[index];
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(tmp, head, hash_entry_id) {
>> +		if (tmp->id != peer_id)
>> +			continue;
>> +
>> +		if (!ovpn_peer_hold(tmp))
>> +			continue;
> 
> Can there ever be multiple peers with the same id? (ie, is it worth
> continuing the loop if this fails? the same question probably applies
> to ovpn_peer_get_by_transp_addr as well)

Well, not at the same time, but theoretically we could re-use the ID of 
a peer that is being released (i.e. still in the list but refcnt at 0) 
because it won't be returned by this lookup.

This said, I truly believe it's impossible for a peer to have refcnt 0 
and still being in the list:
Either
* delete on the peer was not yet called, thus peer is in the list and 
the last reference wasn't yet dropped
* delete on the peer was called, thus peer cannot be in the list anymore 
and refcnt may or may not be 0...


> 
> 
>> +		peer = tmp;
>> +		break;
>> +	}
>> +	rcu_read_unlock();
>>   
>>   	return peer;
>>   }
>> @@ -328,6 +470,11 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn,
>>   				       struct sk_buff *skb)
>>   {
>>   	struct ovpn_peer *tmp, *peer = NULL;
>> +	struct hlist_head *head;
>> +	sa_family_t sa_fam;
>> +	struct in6_addr addr6;
>> +	__be32 addr4;
>> +	u32 index;
>>   
>>   	/* in P2P mode, no matter the destination, packets are always sent to
>>   	 * the single peer listening on the other side
>> @@ -338,15 +485,123 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn,
>>   		if (likely(tmp && ovpn_peer_hold(tmp)))
>>   			peer = tmp;
>>   		rcu_read_unlock();
>> +		return peer;
>> +	}
>> +
>> +	sa_fam = skb_protocol_to_family(skb);
>> +
>> +	switch (sa_fam) {
>> +	case AF_INET:
>> +		addr4 = ovpn_nexthop_from_skb4(skb);
>> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr4,
>> +					sizeof(addr4));
>> +		head = &ovpn->peers.by_vpn_addr[index];
>> +
>> +		peer = ovpn_peer_get_by_vpn_addr4(head, &addr4);
>> +		break;
>> +	case AF_INET6:
>> +		addr6 = ovpn_nexthop_from_skb6(skb);
>> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr6,
>> +					sizeof(addr6));
>> +		head = &ovpn->peers.by_vpn_addr[index];
>> +
>> +		peer = ovpn_peer_get_by_vpn_addr6(head, &addr6);
> 
> The index -> head -> peer code is identical in get_by_dst and
> get_by_src, it could be stuffed into ovpn_peer_get_by_vpn_addr{4,6}.

hm yeah, you're right. I'll do it!

> 
>> +		break;
>>   	}
>>   
>>   	return peer;
>>   }
> 
> 
> [snip the _rt4 variant, comments apply to both]
>> +/**
>> + * ovpn_nexthop_from_rt6 - look up the IPv6 nexthop for the given destination
> 
> I'm a bit confused by this talk about "destination" when those two
> functions are then used with the source address from the packet, from
> a function called "get_by_src".

well, in my brain a next hop can exists only when I want to reach a 
certain destination. Therefore, at a low level, the terms nextop and 
destination always need to go hand in hand.

This said, when implementing RPF (Reverse Path Filtering) I need to 
imagine that I want to route to the source IP of the incoming packet. If 
the nexthop I looked up matches the peer the packet came from, then 
everything is fine.

makes sense?

[FTR I have already renamed/changed get_by_src into check_by_src, 
because I don't need to truly extract a peer and get a reference, but I 
only need to perform the aforementioned comparison.]

> 
>> + * @ovpn: the private data representing the current VPN session
>> + * @dst: the destination to be looked up
>> + *
>> + * Looks up in the IPv6 system routing table the IO of the nexthop to be used
> 
> "the IO"?

typ0: "the IP"

> 
>> + * to reach the destination passed as argument. IF no nexthop can be found, the
>> + * destination itself is returned as it probably has to be used as nexthop.
>> + *
>> + * Return: the IP of the next hop if found or the dst itself otherwise
> 
> "the dst" tends to refer to a dst_entry, maybe "or @dst otherwise"?

it refers to @dst (the function argument). That's basically the case 
where the destination is "onlink" and thus it is the nexthop (basically 
the destination is the connected peer).

> (though I'm not sure that's valid kdoc)
> 
> (also for ovpn_nexthop_from_rt4)
> 
>> + */
>> +static struct in6_addr ovpn_nexthop_from_rt6(struct ovpn_struct *ovpn,
>> +					     struct in6_addr dst)
>> +{
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	struct dst_entry *entry;
>> +	struct rt6_info *rt;
>> +	struct flowi6 fl = {
>> +		.daddr = dst,
>> +	};
>> +
>> +	entry = ipv6_stub->ipv6_dst_lookup_flow(dev_net(ovpn->dev), NULL, &fl,
>> +						NULL);
>> +	if (IS_ERR(entry)) {
>> +		net_dbg_ratelimited("%s: no route to host %pI6c\n", __func__,
>> +				    &dst);
>> +		/* if we end up here this packet is probably going to be
>> +		 * thrown away later
>> +		 */
>> +		return dst;
>> +	}
>> +
>> +	rt = container_of(entry, struct rt6_info, dst);
> 
> dst_rt6_info(entry)

Oh, I see this just came to life in 6.10-rc1. Thanks!

> 
>> +
>> +	if (!(rt->rt6i_flags & RTF_GATEWAY))
>> +		goto out;
>> +
>> +	dst = rt->rt6i_gateway;
>> +out:
>> +	dst_release((struct dst_entry *)rt);
>> +#endif
>> +	return dst;
>> +}
>> +
>>   struct ovpn_peer *ovpn_peer_get_by_src(struct ovpn_struct *ovpn,
>>   				       struct sk_buff *skb)
>>   {
>>   	struct ovpn_peer *tmp, *peer = NULL;
>> +	struct hlist_head *head;
>> +	sa_family_t sa_fam;
>> +	struct in6_addr addr6;
>> +	__be32 addr4;
>> +	u32 index;
>>   
>>   	/* in P2P mode, no matter the destination, packets are always sent to
>>   	 * the single peer listening on the other side
>> @@ -357,6 +612,28 @@ struct ovpn_peer *ovpn_peer_get_by_src(struct ovpn_struct *ovpn,
>>   		if (likely(tmp && ovpn_peer_hold(tmp)))
>>   			peer = tmp;
>>   		rcu_read_unlock();
>> +		return peer;
>> +	}
>> +
>> +	sa_fam = skb_protocol_to_family(skb);
>> +
>> +	switch (sa_fam) {
> 
> nit:
> 	switch (skb_protocol_to_family(skb))
> seems a bit more readable to me (also in ovpn_peer_get_by_dst) - and
> saves you from reverse xmas tree complaints (sa_fam should have been
> after addr6)

ACK, thanks!

> 
>> +	case AF_INET:
>> +		addr4 = ovpn_nexthop_from_rt4(ovpn, ip_hdr(skb)->saddr);
>> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr4,
>> +					sizeof(addr4));
>> +		head = &ovpn->peers.by_vpn_addr[index];
>> +
>> +		peer = ovpn_peer_get_by_vpn_addr4(head, &addr4);
>> +		break;
>> +	case AF_INET6:
>> +		addr6 = ovpn_nexthop_from_rt6(ovpn, ipv6_hdr(skb)->saddr);
>> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr6,
>> +					sizeof(addr6));
>> +		head = &ovpn->peers.by_vpn_addr[index];
>> +
>> +		peer = ovpn_peer_get_by_vpn_addr6(head, &addr6);
>> +		break;
>>   	}
>>   
>>   	return peer;
>> -- 
>> 2.43.2
>>
>>
> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists