2024-07-18, 11:37:38 +0200, Antonio Quartulli wrote:

On 17/07/2024 19:15, Sabrina Dubroca wrote:

quoted

2024-06-27, 15:08:37 +0200, Antonio Quartulli wrote:

quoted

+void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb)
+{
+	struct sockaddr_storage ss;
+	const u8 *local_ip = NULL;
+	struct sockaddr_in6 *sa6;
+	struct sockaddr_in *sa;
+	struct ovpn_bind *bind;
+	sa_family_t family;
+	size_t salen;
+
+	rcu_read_lock();
+	bind = rcu_dereference(peer->bind);
+	if (unlikely(!bind))
+		goto unlock;

Why are you aborting here? ovpn_bind_skb_src_match considers
bind==NULL to be "no match" (reasonable), then we would create a new
bind for the current address.

(NOTE: float and the following explanation assume connection via UDP)

peer->bind is assigned right after peer creation in ovpn_nl_set_peer_doit().

ovpn_peer_float() is called while the peer is exchanging traffic.

If we got to this point and bind is NULL, then the peer was being released,
because there is no way we are going to NULLify bind during the peer life
cycle, except upon ovpn_peer_release().

Does it make sense?

Alright, thanks, I missed that.

quoted

quoted

+	if (likely(ovpn_bind_skb_src_match(bind, skb)))

This could be running in parallel on two CPUs, because ->encap_rcv
isn't protected against that. So the bind could be getting updated in
parallel. I would move spin_lock_bh above this check to make sure it
doesn't happen.

hm, I should actually use peer->lock for this, which is currently only used
in ovpn_bind_reset() to avoid multiple concurrent assignments...but you're
right we should include the call to skb_src_check() as well.

Ok, sounds good.

quoted

ovpn_peer_update_local_endpoint would also need something like that, I
think.

at least the test-and-set part should be protected, if we can truly invoke
ovpn_peer_update_local_endpoint() multiple times concurrently.

Yes.

How do I test running encap_rcv in parallel?
This is actually an interesting case that I thought to not be possible (no
specific reason for this..).

It should happen when the packets come from different source IPs and
the NIC has multiple queues, then they can be spread over different
CPUs. But it's probably not going to be easy to land multiple packets
in ovpn_peer_float at the same time to trigger this issue.

quoted

quoted

+	netdev_dbg(peer->ovpn->dev, "%s: peer %d floated to %pIScp", __func__,
+		   peer->id, &ss);
+	ovpn_peer_reset_sockaddr(peer, (struct sockaddr_storage *)&ss,
+				 local_ip);
+
+	spin_lock_bh(&peer->ovpn->peers->lock);
+	/* remove old hashing */
+	hlist_del_init_rcu(&peer->hash_entry_transp_addr);
+	/* re-add with new transport address */
+	hlist_add_head_rcu(&peer->hash_entry_transp_addr,
+			   ovpn_get_hash_head(peer->ovpn->peers->by_transp_addr,
+					      &ss, salen));

That could send a concurrent reader onto the wrong hash bucket, if
it's going through peer's old bucket, finds peer before the update,
then continues reading after peer is moved to the new bucket.

I haven't fully grasped this scenario.
I am imagining we are running ovpn_peer_get_by_transp_addr() in parallel:
reader gets the old bucket and finds peer, because ovpn_peer_transp_match()
will still return true (update wasn't performed yet), and will return it.

The other reader isn't necessarily looking for peer, but maybe another
item that landed in the same bucket (though your hashtables are so
large, it would be a bit unlucky).

At this point, what do you mean with "continues reading after peer is moved
to the new bucket"?

Continues iterating, in hlist_for_each_entry_rcu inside
ovpn_peer_get_by_transp_addr.

ovpn_peer_float                          ovpn_peer_get_by_transp_addr

                                         start lookup
                                         head = ovpn_get_hash_head(...)
                                         hlist_for_each_entry_rcu
                                         ...
                                         find peer on head

peer moved from head to head2

                                         continue hlist_for_each_entry_rcu with peer->next
                                         but peer->next is now on head2
                                         keep walking ->next on head2 instead of head

-- 
Sabrina