Re: [PATCH net-next v18 20/25] ovpn: implement peer add/get/dump/delete via netlink

From: Antonio Quartulli
Date: Mon Feb 03 2025 - 04:45:41 EST

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.