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

[Sabrina Dubroca <sd@queasysnail.net>]

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.


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

> +{
> +	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);
> +	}
> +}

-- 
Sabrina