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

Antonio Quartulli
2025-02-03T09:45:13Z

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.