Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink

[Antonio Quartulli]

On 11/11/2024 16:41, Sabrina Dubroca wrote:
> 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> > +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
> > +			       struct nlattr **attrs)
> > +{
> > +	struct sockaddr_storage ss = {};
> > +	u32 sockfd, interv, timeout;
> > +	struct socket *sock = NULL;
> > +	u8 *local_ip = NULL;
> > +	bool rehash = false;
> > +	int ret;
> > +
> > +	if (attrs[OVPN_A_PEER_SOCKET]) {
> > +		/* lookup the fd in the kernel table and extract the socket
> > +		 * object
> > +		 */
> > +		sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]);
> > +		/* sockfd_lookup() increases sock's refcounter */
> > +		sock = sockfd_lookup(sockfd, &ret);
> > +		if (!sock) {
> > +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +					       "cannot lookup peer socket (fd=%u): %d",
> > +					       sockfd, ret);
> > +			return -ENOTSOCK;
> > +		}
> > +
> > +		/* Only when using UDP as transport protocol the remote endpoint
> > +		 * can be configured so that ovpn knows where to send packets
> > +		 * to.
> > +		 *
> > +		 * In case of TCP, the socket is connected to the peer and ovpn
> > +		 * will just send bytes over it, without the need to specify a
> > +		 * destination.
> > +		 */
> > +		if (sock->sk->sk_protocol != IPPROTO_UDP &&
> > +		    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
> > +		     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
> > +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +					       "unexpected remote IP address for non UDP socket");
> > +			sockfd_put(sock);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (peer->sock)
> > +			ovpn_socket_put(peer->sock);
> > +
> > +		peer->sock = ovpn_socket_new(sock, peer);
>
> I don't see anything preventing concurrent updates of peer->sock. I
> think peer->lock should be taken from the start of
> ovpn_nl_peer_modify. Concurrent changes to peer->vpn_addrs and
> peer->keepalive_* are also not prevented with the current code.

Yeah, this came up to my mind as well when checking the keepalive worker 
code.

I'll make sure all updates happen under lock.

> > +		if (IS_ERR(peer->sock)) {
> > +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +					       "cannot encapsulate socket: %ld",
> > +					       PTR_ERR(peer->sock));
> > +			sockfd_put(sock);
> > +			peer->sock = NULL;
> > +			return -ENOTSOCK;
> > +		}
> > +	}
> > +
> > +	if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) {
> > +		/* we carry the local IP in a generic container.
> > +		 * ovpn_peer_reset_sockaddr() will properly interpret it
> > +		 * based on ss.ss_family
> > +		 */
> > +		local_ip = ovpn_nl_attr_local_ip(attrs);
> > +
> > +		spin_lock_bh(&peer->lock);
> > +		/* set peer sockaddr */
> > +		ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip);
> > +		if (ret < 0) {
> > +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +					       "cannot set peer sockaddr: %d",
> > +					       ret);
> > +			spin_unlock_bh(&peer->lock);
> > +			return ret;
> > +		}
> > +		spin_unlock_bh(&peer->lock);
> > +	}
> > +
> > +	if (attrs[OVPN_A_PEER_VPN_IPV4]) {
> > +		rehash = true;
> > +		peer->vpn_addrs.ipv4.s_addr =
> > +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> > +	}
> > +
> > +	if (attrs[OVPN_A_PEER_VPN_IPV6]) {
> > +		rehash = true;
> > +		peer->vpn_addrs.ipv6 =
> > +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> > +	}
> > +
> > +	/* when setting the keepalive, both parameters have to be configured */
> > +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> > +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> > +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> > +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> > +		ovpn_peer_keepalive_set(peer, interv, timeout);
> > +	}
> > +
> > +	netdev_dbg(peer->ovpn->dev,
> > +		   "%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
> > +		   __func__, peer->id, &ss,
> > +		   peer->sock->sock->sk->sk_prot_creator->name,
> > +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> > +
> > +	return rehash ? 1 : 0;
> > +}
> > +
>
> [...]
> > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
> > +	__must_hold(&peer->ovpn->peers->lock)
>
> Changes to peer->vpn_addrs are not protected by peers->lock, so those
> could be getting updated while we're rehashing (and taking peer->lock
> in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent
> that).

/me screams :-D

Indeed peers->lock is only about protecting the lists, not the content 
of the listed objects.

How about acquiring the peers->lock before calling ovpn_nl_peer_modify()?
This way we prevent concurrent updates to interfere with each other, 
while at the same time we avoid concurrent adds/dels of the peer (the 
second part should already be protected as of today).

None of them is time critical and the lock should avoid the issue you 
mentioned.


Thanks a lot.

Regards,

> > +{
> > +	struct hlist_nulls_head *nhead;
> > +
> > +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
> > +		/* remove potential old hashing */
> > +		hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
> > +
> > +		nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
> > +					   &peer->vpn_addrs.ipv4,
> > +					   sizeof(peer->vpn_addrs.ipv4));
> > +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead);
> > +	}
> > +
> > +	if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) {
> > +		/* remove potential old hashing */
> > +		hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
> > +
> > +		nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
> > +					   &peer->vpn_addrs.ipv6,
> > +					   sizeof(peer->vpn_addrs.ipv6));
> > +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead);
> > +	}
> > +}

--
Antonio Quartulli
OpenVPN Inc.