Message-ID: <4a539c83-f436-4b1e-9707-64c05dcfdbd2@openvpn.net>
Date: Mon, 3 Feb 2025 10:46:19 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v18 20/25] ovpn: implement peer
 add/get/dump/delete via netlink

On 03/02/2025 00:07, Sabrina Dubroca wrote:
> 2025-01-13, 10:31:39 +0100, Antonio Quartulli wrote:
>> +static int ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
>> +					struct sockaddr_storage *ss)
>> +{
>> +	struct sockaddr_in6 *sin6;
>> +	struct sockaddr_in *sin;
>> +	struct in6_addr *in6;
>> +	__be16 port = 0;
>> +	__be32 *in;
>> +	int af;
>> +
>> +	ss->ss_family = AF_UNSPEC;
>> +
>> +	if (attrs[OVPN_A_PEER_REMOTE_PORT])
>> +		port = nla_get_be16(attrs[OVPN_A_PEER_REMOTE_PORT]);
>> +
>> +	if (attrs[OVPN_A_PEER_REMOTE_IPV4]) {
>> +		af = AF_INET;
>> +		ss->ss_family = AF_INET;
>> +		in = nla_data(attrs[OVPN_A_PEER_REMOTE_IPV4]);
>> +	} else if (attrs[OVPN_A_PEER_REMOTE_IPV6]) {
>> +		af = AF_INET6;
>> +		ss->ss_family = AF_INET6;
>> +		in6 = nla_data(attrs[OVPN_A_PEER_REMOTE_IPV6]);
>> +	} else {
>> +		return AF_UNSPEC;
>> +	}
>> +
>> +	switch (ss->ss_family) {
>> +	case AF_INET6:
>> +		/* If this is a regular IPv6 just break and move on,
>> +		 * otherwise switch to AF_INET and extract the IPv4 accordingly
>> +		 */
>> +		if (!ipv6_addr_v4mapped(in6)) {
>> +			sin6 = (struct sockaddr_in6 *)ss;
>> +			sin6->sin6_port = port;
>> +			memcpy(&sin6->sin6_addr, in6, sizeof(*in6));
>> +			break;
>> +		}
>> +
>> +		/* v4-mapped-v6 address */
>> +		ss->ss_family = AF_INET;
>> +		in = &in6->s6_addr32[3];
>> +		fallthrough;
>> +	case AF_INET:
>> +		sin = (struct sockaddr_in *)ss;
>> +		sin->sin_port = port;
>> +		sin->sin_addr.s_addr = *in;
>> +		break;
>> +	}
>> +
>> +	/* don't return ss->ss_family as it may have changed in case of
>> +	 * v4-mapped-v6 address
>> +	 */
> 
> nit: I'm not sure that matters since the only thing the caller checks
> is ret != AF_UNSPEC, and at this point, while ss_family could have
> been changed, it would have changed from AF_INET6 to AF_INET, so it's
> != AF_UNSPEC.

I am pretty sure at some point the return value was used for some 
reason, but now it is indeed useless.

Well, I think I wiil just convert the return type to bool:
true -> we have a remote
false -> we don't

> 
>> +	return af;
>> +}
> 
> [...]
>> +static int ovpn_nl_peer_precheck(struct ovpn_priv *ovpn,
>> +				 struct genl_info *info,
>> +				 struct nlattr **attrs)
>> +{
> [...]
>> +
>> +	/* VPN IPs are needed only in MP mode for selecting the right peer */
>> +	if (ovpn->mode == OVPN_MODE_P2P && (attrs[OVPN_A_PEER_VPN_IPV4] ||
>> +					    attrs[OVPN_A_PEER_VPN_IPV6])) {
> 
> And in MP mode, at least one VPN_IP* is required?

Yeah. I'll add a check for this requirement too.

> 
> 
> [...]
>>   int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> [...]
>> +	/* 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])) {
> 
> Is a peer on a UDP socket without any remote (neither
> OVPN_A_PEER_REMOTE_IPV4 nor OVPN_A_PEER_REMOTE_IPV6) valid? We just
> wait until we get data from it to update the endpoint?
> 
> Or should there be a check to make sure that one was provided?

Yeah, I'll add a check.

> 
>> +		NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +				       "unexpected remote IP address for non UDP socket");
>> +		sockfd_put(sock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ovpn_sock = ovpn_socket_new(sock, peer);
>> +	if (IS_ERR(ovpn_sock)) {
>> +		NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +				       "cannot encapsulate socket: %ld",
>> +				       PTR_ERR(ovpn_sock));
>> +		sockfd_put(sock);
>> +		return -ENOTSOCK;
> 
> Maybe s/-ENOTSOCK/PTR_ERR(ovpn_sock)/ ?
> Overwriting ovpn_socket_new's -EBUSY etc with -ENOTSOCK is a bit
> misleading to the caller.

This is the error code that userspace will see.
Returning -EBUSY/-EALREADY for a socket error from the PEER_NEW call 
would be too vague IMHO (the user wouldn't know this is coming from the 
socket processing subroutine).

Hence the decision to explicitly return -ENOSOCK (something's wrong with 
the socket you passed) and then send the underling error in the ERR_MSG 
(which the user can inspect if he wants to learn more about what exactly 
went wrong).
Doesn't it make sense?

> 
>> +	}
>> +
>> +	peer->sock = ovpn_sock;
>> +
>> +	ret = ovpn_nl_peer_modify(peer, info, attrs);
>> +	if (ret < 0)
>> +		goto peer_release;
>> +
>> +	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;
>> +	}
>> +
>> +	return 0;
>> +
>> +peer_release:
> 
> I think you need to add:
> 
> 	ovpn_socket_release(peer);
> 
> If ovpn_socket_new succeeded, ovpn_peer_release only takes care of the
> peer but not its socket.

You're right, because now the socket is released only in ovpn_peer_remove().

Will add a call to ovpn_socket_release(). Thanks!

> 
>> +	/* release right away because peer is not used in any context */
>> +	ovpn_peer_release(peer);
>> +
>> +	return ret;
>>   }
>>   
>>   int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> [...]
>> +	if (attrs[OVPN_A_PEER_SOCKET]) {
>> +		NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +				       "socket cannot be modified");
>> +		return -EINVAL;
>> +	}
>> +
>> +	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;
>> +	}
> 
> The check for non-UDP socket with a remote address configured should
> be replicated here, no?

ah, good catch! we may be adding a remote while using a TCP socket.
Will add check here.

Thanks!

> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists