Re: [PATCH net-next v5 19/25] ovpn: add support for peer floating

[Sabrina Dubroca <sd@queasysnail.net>]

2024-06-27, 15:08:37 +0200, Antonio Quartulli wrote:
> +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.

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

ovpn_peer_update_local_endpoint would also need something like that, I
think.

> +		goto unlock;
> +
> +	family = skb_protocol_to_family(skb);
> +
> +	if (bind->sa.in4.sin_family == family)
> +		local_ip = (u8 *)&bind->local;
> +
> +	switch (family) {
> +	case AF_INET:
> +		sa = (struct sockaddr_in *)&ss;
> +		sa->sin_family = AF_INET;
> +		sa->sin_addr.s_addr = ip_hdr(skb)->saddr;
> +		sa->sin_port = udp_hdr(skb)->source;
> +		salen = sizeof(*sa);
> +		break;
> +	case AF_INET6:
> +		sa6 = (struct sockaddr_in6 *)&ss;
> +		sa6->sin6_family = AF_INET6;
> +		sa6->sin6_addr = ipv6_hdr(skb)->saddr;
> +		sa6->sin6_port = udp_hdr(skb)->source;
> +		sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr,
> +							 skb->skb_iif);
> +		salen = sizeof(*sa6);
> +		break;
> +	default:
> +		goto unlock;
> +	}
> +
> +	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.

This kind of re-hash can be handled with nulls, and re-trying the
lookup if we ended up on the wrong chain. See for example
__inet_lookup_established in net/ipv4/inet_hashtables.c (Thanks to
Paolo for the pointer).

> +	spin_unlock_bh(&peer->ovpn->peers->lock);
> +
> +unlock:
> +	rcu_read_unlock();
> +}

-- 
Sabrina