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:

quoted

+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

quoted

+	return af;
+}

[...]

quoted

+static int ovpn_nl_peer_precheck(struct ovpn_priv *ovpn,
+				 struct genl_info *info,
+				 struct nlattr **attrs)
+{

[...]

quoted

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

[...]

quoted

  int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
  {

[...]

quoted

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

quoted

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

quoted

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

quoted

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

[...]

quoted

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