On 28/05/2024 18:42, Sabrina Dubroca wrote:

2024-05-06, 03:16:28 +0200, Antonio Quartulli wrote:

quoted

+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!

quoted

+
+	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

[...]

quoted

+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

[...]

quoted

+	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

quoted

+
+	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.

quoted

+			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...

quoted

+		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!

quoted

+		break;
  	}
  
  	return peer;
  }

[snip the _rt4 variant, comments apply to both]

quoted

+/**
+ * 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.]

quoted

+ * @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"

quoted

+ * 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)

quoted

+ */
+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!

quoted

+
+	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!

quoted

+	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.