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?

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


[...]

+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

[...]

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

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

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)

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

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

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

+ * 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"?
(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)

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)

+	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

-- 
Sabrina