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

[Sabrina Dubroca]

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.

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


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

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

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

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

-- 
Sabrina