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?