Message-ID: <3a6ce780-4532-4823-a326-d10e09688894@openvpn.net>
Date: Wed, 17 Jul 2024 16:04:25 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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



On 16/07/2024 15:41, Sabrina Dubroca wrote:
> 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"

Right, thanks

> 
> 
> [...]
>>   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.

argh. you are correct. I'll remove it.

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

Right - will move it up.

> 
> (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 :))

good point! :-D

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

ACK

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

Right. if attaching the new socket fails, we are entering some broken 
status which is not worth keeping around.

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

True, but those are not used in openvpn. In case of UDP, userspace just 
creates one socket and uses it for all peers.
I will add a note about 'connected UDP socket' in the comment, to clear 
this out.

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

right.

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

judging from the comments below, it seems you prefer to reject unneeded 
attributes. OTOH I took the opposite approach (just ignore those).

However, I was actually looking for some preference/indication regarding 
this point and I now I got one :-)

I will be strict and return -EINVAL when unneded attributes are present.

> 
> 
>> +	/* 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)

ACK

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

yup, like I commented above.

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

ok

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

I am pretty sure there was a reason...but it may have faded away after 
the 95-th rebase hehe. Thanks for spotting this!

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

Because then we would send the notification to userspace, but it is not 
correct to do so, because the new() is just about to return an error.

I presume I should just move netdev_put(peer->ovpn->dev, NULL); to 
ovpn_peer_release(). That will take care of this case too.

> 
>> +	} 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?

indeed. have to set ret and goto err;

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


Thanks!

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists