2024-05-28, 22:09:37 +0200, Antonio Quartulli wrote:

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

quoted

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

quoted

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

Ok, thanks. Let's just keep this code.

quoted

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?

Yeah, that's fair.

[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.]

Ok.

quoted

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

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

I understand that, it's just the wording "the dst" that I'm
nitpicking. s/dst/addr/ would help easily-confused people like me (for
both "the dst" and my confusion with source vs destination in
caller/callee), but I can live with this.

-- 
Sabrina