Message-ID: <ZpZ4cF7hLTIxBiej@hog>
Date: Tue, 16 Jul 2024 15:41:04 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, ryazanov.s.a@...il.com,
	pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 20/25] ovpn: implement peer add/dump/delete
 via netlink

2024-06-27, 15:08:38 +0200, Antonio Quartulli wrote:
> @@ -29,7 +34,7 @@ MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME);
>   * Return: the netdevice, if found, or an error otherwise
>   */
>  static struct net_device *
> -ovpn_get_dev_from_attrs(struct net *net, struct genl_info *info)
> +ovpn_get_dev_from_attrs(struct net *net, const struct genl_info *info)

nit: this should be squashed into "add basic netlink support"


[...]
>  int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return -EOPNOTSUPP;
> +	bool keepalive_set = false, new_peer = false;
> +	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
> +	struct ovpn_struct *ovpn = info->user_ptr[0];
> +	struct sockaddr_storage *ss = NULL;
> +	u32 sockfd, id, interv, timeout;
> +	struct socket *sock = NULL;
> +	struct sockaddr_in mapped;
> +	struct sockaddr_in6 *in6;
> +	struct ovpn_peer *peer;
> +	u8 *local_ip = NULL;
> +	size_t sa_len;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER))
> +		return -EINVAL;
> +
> +	ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],
> +			       ovpn_peer_nl_policy, info->extack);
> +	if (ret)
> +		return ret;
> +
> +	if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
> +			      OVPN_A_PEER_ID))
> +		return -EINVAL;
> +
> +	id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
> +	/* check if the peer exists first, otherwise create a new one */
> +	peer = ovpn_peer_get_by_id(ovpn, id);
> +	if (!peer) {
> +		peer = ovpn_peer_new(ovpn, id);
> +		new_peer = true;
> +		if (IS_ERR(peer)) {
> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +					       "cannot create new peer object for peer %u (sockaddr=%pIScp): %ld",
> +					       id, ss, PTR_ERR(peer));

ss hasn't been set yet at this point, including it in the extack
message is not useful.

> +			return PTR_ERR(peer);
> +		}
> +	}
> +
> +	if (new_peer && NL_REQ_ATTR_CHECK(info->extack,
> +					  info->attrs[OVPN_A_PEER], attrs,
> +					  OVPN_A_PEER_SOCKET)) {

This can be checked at the start of the previous block (!peer), we'd
avoid a pointless peer allocation.

(and the linebreaks in NL_REQ_ATTR_CHECK end up being slightly better
because you don't need the "new_peer &&" test that is wider than the
tab used to indent the !peer block :))

> +		ret = -EINVAL;
> +		goto peer_release;
> +	}
> +
> +	if (new_peer && ovpn->mode == OVPN_MODE_MP &&
> +	    !attrs[OVPN_A_PEER_VPN_IPV4] && !attrs[OVPN_A_PEER_VPN_IPV6]) {

Same for this check.

> +		NL_SET_ERR_MSG_MOD(info->extack,
> +				   "a VPN IP is required when adding a peer in MP mode");
> +		ret = -EINVAL;
> +		goto peer_release;
> +	}
> +
> +	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);
> +			ret = -ENOTSOCK;
> +			goto peer_release;
> +		}
> +
> +		if (peer->sock)
> +			ovpn_socket_put(peer->sock);
> +
> +		peer->sock = ovpn_socket_new(sock, peer);
> +		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;

Is there any value for the client in keeping the old peer->sock
assigned if we fail here?

ie something like:

    tmp = ovpn_socket_new(sock, peer);
    if (IS_ERR(tmp)) {
        ...
        goto peer_release;
    }
    if (peer->sock)
        ovpn_socket_put(peer->sock);
    peer->sock = tmp;


But if it's just going to get rid of the old socket and the whole
association/peer on failure, probably not.

> +			ret = -ENOTSOCK;
> +			goto peer_release;
> +		}
> +	}
> +
> +	/* 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.

(that should also work with UDP "connected" sockets)


> +	 */
> +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP &&
> +	    attrs[OVPN_A_PEER_SOCKADDR_REMOTE]) {
[...]
> +
> +		if (attrs[OVPN_A_PEER_LOCAL_IP]) {
> +			local_ip = ovpn_nl_attr_local_ip(info, ovpn,
> +							 attrs,
> +							 ss->ss_family);
> +			if (IS_ERR(local_ip)) {
> +				ret = PTR_ERR(local_ip);
> +				NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +						       "cannot retrieve local IP: %d",
> +						       ret);

ovpn_nl_attr_local_ip already sets a more specific extack message,
this is unnecessary.

> +				goto peer_release;
> +			}
> +		}
> +
> +		/* 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);
> +			goto peer_release;
> +		}
> +	}

I would reject OVPN_A_PEER_SOCKADDR_REMOTE for a non-UDP socket.


> +	/* VPN IPs cannot be updated, because they are hashed */

Then I think there should be something like

    if (!new_peer && (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
        NL_SET_ERR_MSG_FMT_MOD(... "can't update ip");
        ret = -EINVAL;
        goto peer_release;
    }

(just after getting the peer, before any changes have actually been
made)

And if they are only used in MP mode, I would maybe also reject
requests where mode==P2P and OVPN_A_PEER_VPN_IPV* is provided.


> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
> +		peer->vpn_addrs.ipv4.s_addr =
> +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> +
> +	/* VPN IPs cannot be updated, because they are hashed */
> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
> +		peer->vpn_addrs.ipv6 =
> +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> +
> +	/* when setting the keepalive, both parameters have to be configured */

Then I would also reject a config where only one is set (also before any
changes have been made).

> +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> +		keepalive_set = true;
> +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> +	}
> +
> +	if (keepalive_set)
> +		ovpn_peer_keepalive_set(peer, interv, timeout);

Why not skip the bool and just do this in the previous block?

> +	netdev_dbg(ovpn->dev,
> +		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
> +		   __func__, (new_peer ? "adding" : "modifying"), ss,
> +		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
> +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> +
> +	if (new_peer) {
> +		ret = ovpn_peer_add(ovpn, peer);
> +		if (ret < 0) {
> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +					       "cannot add new peer (id=%u) to hashtable: %d\n",
> +					       peer->id, ret);
> +			goto peer_release;
> +		}
> +	} else {
> +		ovpn_peer_put(peer);
> +	}
> +
> +	return 0;
> +
> +peer_release:
> +	if (new_peer) {
> +		/* release right away because peer is not really used in any
> +		 * context
> +		 */
> +		ovpn_peer_release(peer);
> +		kfree(peer);

I don't think that's correct, the new peer was created with
ovpn_peer_new, so it took a reference on the netdevice
(netdev_hold(ovpn->dev, ...)), which isn't released by
ovpn_peer_release. Why not just go through ovpn_peer_put?

> +	} else {
> +		ovpn_peer_put(peer);
> +	}
> +
> +	return ret;
> +}
> +

[...]
>  int ovpn_nl_get_peer_doit(struct sk_buff *skb, struct genl_info *info)
>  {
[...]
> +	peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
> +	peer = ovpn_peer_get_by_id(ovpn, peer_id);
> +	if (!peer) {
> +		NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +				       "cannot find peer with id %u", peer_id);
> +		return -ENOENT;
> +	}
> +
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)

Missing ovpn_peer_put?

> +		return -ENOMEM;
> +
> +	ret = ovpn_nl_send_peer(msg, info, peer, info->snd_portid,
> +				info->snd_seq, 0);
> +	if (ret < 0) {
> +		nlmsg_free(msg);
> +		goto err;
> +	}
> +
> +	ret = genlmsg_reply(msg, info);
> +err:
> +	ovpn_peer_put(peer);
> +	return ret;
>  }

-- 
Sabrina

Powered by blists - more mailing lists