| Message ID | 20241029-b4-ovpn-v11-18-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, > + struct genl_info *info, > + struct nlattr **attrs) > +{ > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, > + OVPN_A_PEER_ID)) > + return -EINVAL; > + > + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { > + NL_SET_ERR_MSG_MOD(info->extack, > + "cannot specify both remote IPv4 or IPv6 address"); > + return -EINVAL; > + } > + > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { > + NL_SET_ERR_MSG_MOD(info->extack, > + "cannot specify remote port without IP address"); > + return -EINVAL; > + } > + > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > + attrs[OVPN_A_PEER_LOCAL_IPV4]) { > + NL_SET_ERR_MSG_MOD(info->extack, > + "cannot specify local IPv4 address without remote"); > + return -EINVAL; > + } > + > + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && > + attrs[OVPN_A_PEER_LOCAL_IPV6]) { I think these consistency checks should account for v4mapped addresses. With remote=v4mapped and local=v6 we'll end up with an incorrect ipv4 "local" address (taken out of the ipv6 address's first 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) out of that. > + NL_SET_ERR_MSG_MOD(info->extack, > + "cannot specify local IPV6 address without remote"); > + return -EINVAL; > + } [...] > int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info) > { [...] > + ret = ovpn_nl_peer_modify(peer, info, attrs); > + if (ret < 0) { > + ovpn_peer_put(peer); > + return ret; > + } > + > + /* ret == 1 means that VPN IPv4/6 has been modified and rehashing > + * is required > + */ > + if (ret > 0) { && mode == MP ? I don't see ovpn_nl_peer_modify checking that before returning 1, and in P2P mode ovpn->peers will be NULL. > + spin_lock_bh(&ovpn->peers->lock); > + ovpn_peer_hash_vpn_ip(peer); > + spin_unlock_bh(&ovpn->peers->lock); > + } > + > + ovpn_peer_put(peer); > + > + return 0; > +} > int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > { [...] > + } else { > + rcu_read_lock(); > + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer, > + hash_entry_id) { > + /* skip already dumped peers that were dumped by > + * previous invocations > + */ > + if (last_idx > 0) { > + last_idx--; > + continue; > + } If a peer that was dumped during a previous invocation is removed in between, we'll miss one that's still present in the overall dump. I don't know how much it matters (I guses it depends on how the results of this dump are used by userspace), so I'll let you decide if this needs to be fixed immediately or if it can be ignored for now. > + > + if (ovpn_nl_send_peer(skb, info, peer, > + NETLINK_CB(cb->skb).portid, > + cb->nlh->nlmsg_seq, > + NLM_F_MULTI) < 0) > + break; > + > + /* count peers being dumped during this invocation */ > + dumped++; > + } > + rcu_read_unlock(); > + } > + > +out: > + netdev_put(ovpn->dev, &ovpn->dev_tracker); > + > + /* sum up peers dumped in this message, so that at the next invocation > + * we can continue from where we left > + */ > + cb->args[1] += dumped; > + return skb->len; > } > > int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info) > { > - return -EOPNOTSUPP; > + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; > + struct ovpn_struct *ovpn = info->user_ptr[0]; > + struct ovpn_peer *peer; > + u32 peer_id; > + int ret; > + > + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) > + return -EINVAL; > + > + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], > + ovpn_peer_nl_policy, info->extack); > + if (ret) > + return ret; > + > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, > + OVPN_A_PEER_ID)) > + return -EINVAL; > + > + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]); > + > + peer = ovpn_peer_get_by_id(ovpn, peer_id); > + if (!peer) maybe c/p the extack from ovpn_nl_peer_get_doit? > + return -ENOENT; > + > + netdev_dbg(ovpn->dev, "%s: peer id=%u\n", __func__, peer->id); > + ret = ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_USERSPACE); > + ovpn_peer_put(peer); > + > + return ret; > }
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info, > + struct nlattr **attrs) > +{ > + struct sockaddr_storage ss = {}; > + u32 sockfd, interv, timeout; > + struct socket *sock = NULL; > + u8 *local_ip = NULL; > + bool rehash = false; > + int ret; > + > + if (attrs[OVPN_A_PEER_SOCKET]) { > + /* lookup the fd in the kernel table and extract the socket > + * object > + */ > + sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]); > + /* sockfd_lookup() increases sock's refcounter */ > + sock = sockfd_lookup(sockfd, &ret); > + if (!sock) { > + NL_SET_ERR_MSG_FMT_MOD(info->extack, > + "cannot lookup peer socket (fd=%u): %d", > + sockfd, ret); > + return -ENOTSOCK; > + } > + > + /* 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])) { > + NL_SET_ERR_MSG_FMT_MOD(info->extack, > + "unexpected remote IP address for non UDP socket"); > + sockfd_put(sock); > + return -EINVAL; > + } > + > + if (peer->sock) > + ovpn_socket_put(peer->sock); > + > + peer->sock = ovpn_socket_new(sock, peer); I don't see anything preventing concurrent updates of peer->sock. I think peer->lock should be taken from the start of ovpn_nl_peer_modify. Concurrent changes to peer->vpn_addrs and peer->keepalive_* are also not prevented with the current code. > + if (IS_ERR(peer->sock)) { > + NL_SET_ERR_MSG_FMT_MOD(info->extack, > + "cannot encapsulate socket: %ld", > + PTR_ERR(peer->sock)); > + sockfd_put(sock); > + peer->sock = NULL; > + return -ENOTSOCK; > + } > + } > + > + if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) { > + /* we carry the local IP in a generic container. > + * ovpn_peer_reset_sockaddr() will properly interpret it > + * based on ss.ss_family > + */ > + local_ip = ovpn_nl_attr_local_ip(attrs); > + > + spin_lock_bh(&peer->lock); > + /* set peer sockaddr */ > + ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip); > + if (ret < 0) { > + NL_SET_ERR_MSG_FMT_MOD(info->extack, > + "cannot set peer sockaddr: %d", > + ret); > + spin_unlock_bh(&peer->lock); > + return ret; > + } > + spin_unlock_bh(&peer->lock); > + } > + > + if (attrs[OVPN_A_PEER_VPN_IPV4]) { > + rehash = true; > + peer->vpn_addrs.ipv4.s_addr = > + nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]); > + } > + > + if (attrs[OVPN_A_PEER_VPN_IPV6]) { > + rehash = true; > + peer->vpn_addrs.ipv6 = > + nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]); > + } > + > + /* when setting the keepalive, both parameters have to be configured */ > + if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] && > + attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) { > + interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]); > + timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]); > + ovpn_peer_keepalive_set(peer, interv, timeout); > + } > + > + netdev_dbg(peer->ovpn->dev, > + "%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n", > + __func__, peer->id, &ss, > + peer->sock->sock->sk->sk_prot_creator->name, > + &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6); > + > + return rehash ? 1 : 0; > +} > + [...] > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) > + __must_hold(&peer->ovpn->peers->lock) Changes to peer->vpn_addrs are not protected by peers->lock, so those could be getting updated while we're rehashing (and taking peer->lock in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent that). > +{ > + struct hlist_nulls_head *nhead; > + > + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { > + /* remove potential old hashing */ > + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); > + > + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, > + &peer->vpn_addrs.ipv4, > + sizeof(peer->vpn_addrs.ipv4)); > + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); > + } > + > + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { > + /* remove potential old hashing */ > + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); > + > + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, > + &peer->vpn_addrs.ipv6, > + sizeof(peer->vpn_addrs.ipv6)); > + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); > + } > +}
On 04/11/2024 16:14, Sabrina Dubroca wrote: > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >> +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, >> + struct genl_info *info, >> + struct nlattr **attrs) >> +{ >> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, >> + OVPN_A_PEER_ID)) >> + return -EINVAL; >> + >> + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { >> + NL_SET_ERR_MSG_MOD(info->extack, >> + "cannot specify both remote IPv4 or IPv6 address"); >> + return -EINVAL; >> + } >> + >> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >> + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { >> + NL_SET_ERR_MSG_MOD(info->extack, >> + "cannot specify remote port without IP address"); >> + return -EINVAL; >> + } >> + >> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >> + attrs[OVPN_A_PEER_LOCAL_IPV4]) { >> + NL_SET_ERR_MSG_MOD(info->extack, >> + "cannot specify local IPv4 address without remote"); >> + return -EINVAL; >> + } >> + >> + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && >> + attrs[OVPN_A_PEER_LOCAL_IPV6]) { > > I think these consistency checks should account for v4mapped > addresses. With remote=v4mapped and local=v6 we'll end up with an > incorrect ipv4 "local" address (taken out of the ipv6 address's first > 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, > we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to > ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) > out of that. Right, a v4mapped address would fool this check. How about checking if both or none addresses are v4mapped? This way we should prevent such cases. > >> + NL_SET_ERR_MSG_MOD(info->extack, >> + "cannot specify local IPV6 address without remote"); >> + return -EINVAL; >> + } > > > [...] >> int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info) >> { > [...] >> + ret = ovpn_nl_peer_modify(peer, info, attrs); >> + if (ret < 0) { >> + ovpn_peer_put(peer); >> + return ret; >> + } >> + >> + /* ret == 1 means that VPN IPv4/6 has been modified and rehashing >> + * is required >> + */ >> + if (ret > 0) { > > && mode == MP ? > > I don't see ovpn_nl_peer_modify checking that before returning 1, and > in P2P mode ovpn->peers will be NULL. Right. I was wondering if it's better to add the check on the return statement of ovpn_nl_peer_modify...but I think it's more functional to add it here, as per your suggestion. > >> + spin_lock_bh(&ovpn->peers->lock); >> + ovpn_peer_hash_vpn_ip(peer); >> + spin_unlock_bh(&ovpn->peers->lock); >> + } >> + >> + ovpn_peer_put(peer); >> + >> + return 0; >> +} > >> int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) >> { > [...] >> + } else { >> + rcu_read_lock(); >> + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer, >> + hash_entry_id) { >> + /* skip already dumped peers that were dumped by >> + * previous invocations >> + */ >> + if (last_idx > 0) { >> + last_idx--; >> + continue; >> + } > > If a peer that was dumped during a previous invocation is removed in > between, we'll miss one that's still present in the overall dump. I > don't know how much it matters (I guses it depends on how the results > of this dump are used by userspace), so I'll let you decide if this > needs to be fixed immediately or if it can be ignored for now. True, this is a risk I assumed. Not extremely important if you ask me, but do you have any suggestion how to avoid this in an elegant and lockless way? IIRC I got inspired by the station dump in the mac80211 code, which probably assumes the same risk. > >> + >> + if (ovpn_nl_send_peer(skb, info, peer, >> + NETLINK_CB(cb->skb).portid, >> + cb->nlh->nlmsg_seq, >> + NLM_F_MULTI) < 0) >> + break; >> + >> + /* count peers being dumped during this invocation */ >> + dumped++; >> + } >> + rcu_read_unlock(); >> + } >> + >> +out: >> + netdev_put(ovpn->dev, &ovpn->dev_tracker); >> + >> + /* sum up peers dumped in this message, so that at the next invocation >> + * we can continue from where we left >> + */ >> + cb->args[1] += dumped; >> + return skb->len; >> } >> >> int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info) >> { >> - return -EOPNOTSUPP; >> + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; >> + struct ovpn_struct *ovpn = info->user_ptr[0]; >> + struct ovpn_peer *peer; >> + u32 peer_id; >> + int ret; >> + >> + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) >> + return -EINVAL; >> + >> + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], >> + ovpn_peer_nl_policy, info->extack); >> + if (ret) >> + return ret; >> + >> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, >> + OVPN_A_PEER_ID)) >> + return -EINVAL; >> + >> + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]); >> + >> + peer = ovpn_peer_get_by_id(ovpn, peer_id); >> + if (!peer) > > maybe c/p the extack from ovpn_nl_peer_get_doit? Yes, will do. Thanks a lot. Regards, > >> + return -ENOENT; >> + >> + netdev_dbg(ovpn->dev, "%s: peer id=%u\n", __func__, peer->id); >> + ret = ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_USERSPACE); >> + ovpn_peer_put(peer); >> + >> + return ret; >> } >
On 11/11/2024 16:41, Sabrina Dubroca wrote: > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info, >> + struct nlattr **attrs) >> +{ >> + struct sockaddr_storage ss = {}; >> + u32 sockfd, interv, timeout; >> + struct socket *sock = NULL; >> + u8 *local_ip = NULL; >> + bool rehash = false; >> + int ret; >> + >> + if (attrs[OVPN_A_PEER_SOCKET]) { >> + /* lookup the fd in the kernel table and extract the socket >> + * object >> + */ >> + sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]); >> + /* sockfd_lookup() increases sock's refcounter */ >> + sock = sockfd_lookup(sockfd, &ret); >> + if (!sock) { >> + NL_SET_ERR_MSG_FMT_MOD(info->extack, >> + "cannot lookup peer socket (fd=%u): %d", >> + sockfd, ret); >> + return -ENOTSOCK; >> + } >> + >> + /* 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])) { >> + NL_SET_ERR_MSG_FMT_MOD(info->extack, >> + "unexpected remote IP address for non UDP socket"); >> + sockfd_put(sock); >> + return -EINVAL; >> + } >> + >> + if (peer->sock) >> + ovpn_socket_put(peer->sock); >> + >> + peer->sock = ovpn_socket_new(sock, peer); > > I don't see anything preventing concurrent updates of peer->sock. I > think peer->lock should be taken from the start of > ovpn_nl_peer_modify. Concurrent changes to peer->vpn_addrs and > peer->keepalive_* are also not prevented with the current code. Yeah, this came up to my mind as well when checking the keepalive worker code. I'll make sure all updates happen under lock. > > >> + if (IS_ERR(peer->sock)) { >> + NL_SET_ERR_MSG_FMT_MOD(info->extack, >> + "cannot encapsulate socket: %ld", >> + PTR_ERR(peer->sock)); >> + sockfd_put(sock); >> + peer->sock = NULL; >> + return -ENOTSOCK; >> + } >> + } >> + >> + if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) { >> + /* we carry the local IP in a generic container. >> + * ovpn_peer_reset_sockaddr() will properly interpret it >> + * based on ss.ss_family >> + */ >> + local_ip = ovpn_nl_attr_local_ip(attrs); >> + >> + spin_lock_bh(&peer->lock); >> + /* set peer sockaddr */ >> + ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip); >> + if (ret < 0) { >> + NL_SET_ERR_MSG_FMT_MOD(info->extack, >> + "cannot set peer sockaddr: %d", >> + ret); >> + spin_unlock_bh(&peer->lock); >> + return ret; >> + } >> + spin_unlock_bh(&peer->lock); >> + } >> + >> + if (attrs[OVPN_A_PEER_VPN_IPV4]) { >> + rehash = true; >> + peer->vpn_addrs.ipv4.s_addr = >> + nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]); >> + } >> + >> + if (attrs[OVPN_A_PEER_VPN_IPV6]) { >> + rehash = true; >> + peer->vpn_addrs.ipv6 = >> + nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]); >> + } >> + >> + /* when setting the keepalive, both parameters have to be configured */ >> + if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] && >> + attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) { >> + interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]); >> + timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]); >> + ovpn_peer_keepalive_set(peer, interv, timeout); >> + } >> + >> + netdev_dbg(peer->ovpn->dev, >> + "%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n", >> + __func__, peer->id, &ss, >> + peer->sock->sock->sk->sk_prot_creator->name, >> + &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6); >> + >> + return rehash ? 1 : 0; >> +} >> + > > [...] >> +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) >> + __must_hold(&peer->ovpn->peers->lock) > > Changes to peer->vpn_addrs are not protected by peers->lock, so those > could be getting updated while we're rehashing (and taking peer->lock > in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent > that). > /me screams :-D Indeed peers->lock is only about protecting the lists, not the content of the listed objects. How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? This way we prevent concurrent updates to interfere with each other, while at the same time we avoid concurrent adds/dels of the peer (the second part should already be protected as of today). None of them is time critical and the lock should avoid the issue you mentioned. Thanks a lot. Regards, >> +{ >> + struct hlist_nulls_head *nhead; >> + >> + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { >> + /* remove potential old hashing */ >> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); >> + >> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, >> + &peer->vpn_addrs.ipv4, >> + sizeof(peer->vpn_addrs.ipv4)); >> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); >> + } >> + >> + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { >> + /* remove potential old hashing */ >> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); >> + >> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, >> + &peer->vpn_addrs.ipv6, >> + sizeof(peer->vpn_addrs.ipv6)); >> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); >> + } >> +} >
2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote: > On 11/11/2024 16:41, Sabrina Dubroca wrote: > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > > > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) > > > + __must_hold(&peer->ovpn->peers->lock) > > > > Changes to peer->vpn_addrs are not protected by peers->lock, so those > > could be getting updated while we're rehashing (and taking peer->lock > > in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent > > that). > > > > /me screams :-D Sorry :) > Indeed peers->lock is only about protecting the lists, not the content of > the listed objects. > > How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? It seems like it would work. Maybe a bit weird to have conditional locking (MP mode only), but ok. You already have this lock ordering (hold peers->lock before taking peer->lock) in ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing the same thing in the netlink code. Then I would also do that in ovpn_peer_float to protect that rehash. It feels like peers->lock is turning into a duplicate of ovpn->lock. ovpn->lock used for P2P mode, peers->lock used equivalently for MP mode. You might consider merging them (but I wouldn't see it as necessary for merging the series unless there's a locking issue with the current proposal).
2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote: > On 04/11/2024 16:14, Sabrina Dubroca wrote: > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > > > +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, > > > + struct genl_info *info, > > > + struct nlattr **attrs) > > > +{ > > > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, > > > + OVPN_A_PEER_ID)) > > > + return -EINVAL; > > > + > > > + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > + "cannot specify both remote IPv4 or IPv6 address"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > > > + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > + "cannot specify remote port without IP address"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > > > + attrs[OVPN_A_PEER_LOCAL_IPV4]) { > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > + "cannot specify local IPv4 address without remote"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && > > > + attrs[OVPN_A_PEER_LOCAL_IPV6]) { > > > > I think these consistency checks should account for v4mapped > > addresses. With remote=v4mapped and local=v6 we'll end up with an > > incorrect ipv4 "local" address (taken out of the ipv6 address's first > > 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, > > we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to > > ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) > > out of that. > > Right, a v4mapped address would fool this check. > How about checking if both or none addresses are v4mapped? This way we > should prevent such cases. I don't know when userspace would use v4mapped addresses, but treating a v4mapped address as a "proper" ipv4 address should work with the rest of the code, since you already have the conversion in ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you could do something like (rough idea and completely untested): static int get_family(attr_v4, attr_v6) { if (attr_v4) return AF_INET; if (attr_v6) { if (ipv6_addr_v4mapped(attr_v6) return AF_INET; return AF_INET6; } return AF_UNSPEC; } // in _precheck: // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6 remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]); local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]); if (remote_family != local_family) { extack "incompatible address families"; return -EINVAL; } That would mirror the conversion that ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do. > > > int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > > > { > > [...] > > > + } else { > > > + rcu_read_lock(); > > > + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer, > > > + hash_entry_id) { > > > + /* skip already dumped peers that were dumped by > > > + * previous invocations > > > + */ > > > + if (last_idx > 0) { > > > + last_idx--; > > > + continue; > > > + } > > > > If a peer that was dumped during a previous invocation is removed in > > between, we'll miss one that's still present in the overall dump. I > > don't know how much it matters (I guses it depends on how the results > > of this dump are used by userspace), so I'll let you decide if this > > needs to be fixed immediately or if it can be ignored for now. > > True, this is a risk I assumed. > Not extremely important if you ask me, but do you have any suggestion how to > avoid this in an elegant and lockless way? No, inconsistent dumps are an old problem with netlink, so I'm just mentioning it as something to be aware of. You can add genl_dump_check_consistent to let userspace know that it may have gotten incorrect information (you'll need to keep a counter and increment it when a peer is added/removed). On a very busy server you may never manage to get a consistent dump, if peers are going up and down very fast. There's been some progress for dumping netdevices in commit 759ab1edb56c ("net: store netdevs in an xarray"), but that can still return incorrect data.
On 13/11/2024 17:56, Sabrina Dubroca wrote: > 2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote: >> On 04/11/2024 16:14, Sabrina Dubroca wrote: >>> 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >>>> +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, >>>> + struct genl_info *info, >>>> + struct nlattr **attrs) >>>> +{ >>>> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, >>>> + OVPN_A_PEER_ID)) >>>> + return -EINVAL; >>>> + >>>> + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { >>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>> + "cannot specify both remote IPv4 or IPv6 address"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >>>> + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { >>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>> + "cannot specify remote port without IP address"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >>>> + attrs[OVPN_A_PEER_LOCAL_IPV4]) { >>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>> + "cannot specify local IPv4 address without remote"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && >>>> + attrs[OVPN_A_PEER_LOCAL_IPV6]) { >>> >>> I think these consistency checks should account for v4mapped >>> addresses. With remote=v4mapped and local=v6 we'll end up with an >>> incorrect ipv4 "local" address (taken out of the ipv6 address's first >>> 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, >>> we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to >>> ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) >>> out of that. >> >> Right, a v4mapped address would fool this check. >> How about checking if both or none addresses are v4mapped? This way we >> should prevent such cases. > > I don't know when userspace would use v4mapped addresses, It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY" set to true (you can check ipv6(7) for more details). This socket can receive IPv4 connections, which are implemented using v4mapped addresses. In this case both remote and local are going to be v4mapped. However, the sanity check should make sure nobody can inject bogus combinations. > but treating > a v4mapped address as a "proper" ipv4 address should work with the > rest of the code, since you already have the conversion in > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you > could do something like (rough idea and completely untested): > > static int get_family(attr_v4, attr_v6) > { > if (attr_v4) > return AF_INET; > if (attr_v6) { > if (ipv6_addr_v4mapped(attr_v6) > return AF_INET; > return AF_INET6; > } > return AF_UNSPEC; > } > > > // in _precheck: > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6 the latter is already covered by: 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) { 194 NL_SET_ERR_MSG_MOD(info->extack, 195 "cannot specify local IPv4 address without remote"); 196 return -EINVAL; 197 } 198 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) { 201 NL_SET_ERR_MSG_MOD(info->extack, 202 "cannot specify local IPV6 address without remote"); 203 return -EINVAL; 204 } > > remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]); > local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]); > if (remote_family != local_family) { > extack "incompatible address families"; > return -EINVAL; > } > > That would mirror the conversion that > ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do. Yeah, pretty much what I was suggested, but in a more explicit manner. I like it. > >>>> int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) >>>> { >>> [...] >>>> + } else { >>>> + rcu_read_lock(); >>>> + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer, >>>> + hash_entry_id) { >>>> + /* skip already dumped peers that were dumped by >>>> + * previous invocations >>>> + */ >>>> + if (last_idx > 0) { >>>> + last_idx--; >>>> + continue; >>>> + } >>> >>> If a peer that was dumped during a previous invocation is removed in >>> between, we'll miss one that's still present in the overall dump. I >>> don't know how much it matters (I guses it depends on how the results >>> of this dump are used by userspace), so I'll let you decide if this >>> needs to be fixed immediately or if it can be ignored for now. >> >> True, this is a risk I assumed. >> Not extremely important if you ask me, but do you have any suggestion how to >> avoid this in an elegant and lockless way? > > No, inconsistent dumps are an old problem with netlink, so I'm just > mentioning it as something to be aware of. You can add > genl_dump_check_consistent to let userspace know that it may have > gotten incorrect information (you'll need to keep a counter and > increment it when a peer is added/removed). On a very busy server you > may never manage to get a consistent dump, if peers are going up and > down very fast. > > There's been some progress for dumping netdevices in commit > 759ab1edb56c ("net: store netdevs in an xarray"), but that can still > return incorrect data. Got it. I'll keep it as it is for now, since this is not critical. Thanks a lot. Regards, >
On 13/11/2024 12:05, Sabrina Dubroca wrote: > 2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote: >> On 11/11/2024 16:41, Sabrina Dubroca wrote: >>> 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >>>> +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) >>>> + __must_hold(&peer->ovpn->peers->lock) >>> >>> Changes to peer->vpn_addrs are not protected by peers->lock, so those >>> could be getting updated while we're rehashing (and taking peer->lock >>> in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent >>> that). >>> >> >> /me screams :-D > > Sorry :) > >> Indeed peers->lock is only about protecting the lists, not the content of >> the listed objects. >> >> How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? > > It seems like it would work. Maybe a bit weird to have conditional > locking (MP mode only), but ok. You already have this lock ordering > (hold peers->lock before taking peer->lock) in > ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing > the same thing in the netlink code. Yeah. > > Then I would also do that in ovpn_peer_float to protect that rehash. I am not extremely comfortable with this, because it means acquiring peers->lock on every packet (right now we do so only on peer->lock) and it may defeat the advantage of the RCU locking on the hashtables. Wouldn't you agree? An alternative would be to hold peer->lock for the entire function, but this will lead to dead locks...no go either. > > It feels like peers->lock is turning into a duplicate of > ovpn->lock. ovpn->lock used for P2P mode, peers->lock used > equivalently for MP mode. You might consider merging them (but I > wouldn't see it as necessary for merging the series unless there's a > locking issue with the current proposal). I agree: ovpn->lock was introduced to protect ovpn's fields, but actually the only one e protect is peer. They are truly the same and I could therefore get rid of ovpn->peers->lock and always use ovpn->lock. Will see how invasive this is and decide whether to commit it to v12 or not. Thanks! Regards,
2024-11-14, 10:21:18 +0100, Antonio Quartulli wrote: > On 13/11/2024 17:56, Sabrina Dubroca wrote: > > 2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote: > > > On 04/11/2024 16:14, Sabrina Dubroca wrote: > > > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > > > > > +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, > > > > > + struct genl_info *info, > > > > > + struct nlattr **attrs) > > > > > +{ > > > > > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, > > > > > + OVPN_A_PEER_ID)) > > > > > + return -EINVAL; > > > > > + > > > > > + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { > > > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > > > + "cannot specify both remote IPv4 or IPv6 address"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > > > > > + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { > > > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > > > + "cannot specify remote port without IP address"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > > > > > + attrs[OVPN_A_PEER_LOCAL_IPV4]) { > > > > > + NL_SET_ERR_MSG_MOD(info->extack, > > > > > + "cannot specify local IPv4 address without remote"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && > > > > > + attrs[OVPN_A_PEER_LOCAL_IPV6]) { > > > > > > > > I think these consistency checks should account for v4mapped > > > > addresses. With remote=v4mapped and local=v6 we'll end up with an > > > > incorrect ipv4 "local" address (taken out of the ipv6 address's first > > > > 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, > > > > we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to > > > > ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) > > > > out of that. > > > > > > Right, a v4mapped address would fool this check. > > > How about checking if both or none addresses are v4mapped? This way we > > > should prevent such cases. > > > > I don't know when userspace would use v4mapped addresses, > > It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY" > set to true (you can check ipv6(7) for more details). > This socket can receive IPv4 connections, which are implemented using > v4mapped addresses. In this case both remote and local are going to be > v4mapped. I'm familiar with v4mapped addresses, but I wasn't sure the userspace part would actually passed them as peer. But I guess it would when the peer connects over ipv4 on an ipv6 socket. So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never happen? In that case I guess we just need to check that we got 2 attributes of the same type (both _IPV4 or both _IPV6) and if we got _IPV6, that they're either both v4mapped or both not. Might be a tiny bit simpler than what I was suggesting below. > However, the sanity check should make sure nobody can inject bogus > combinations. > > > but treating > > a v4mapped address as a "proper" ipv4 address should work with the > > rest of the code, since you already have the conversion in > > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you > > could do something like (rough idea and completely untested): > > > > static int get_family(attr_v4, attr_v6) > > { > > if (attr_v4) > > return AF_INET; > > if (attr_v6) { > > if (ipv6_addr_v4mapped(attr_v6) > > return AF_INET; > > return AF_INET6; > > } > > return AF_UNSPEC; > > } > > > > > > // in _precheck: > > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check > > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6 > > the latter is already covered by: > > 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) { > 194 NL_SET_ERR_MSG_MOD(info->extack, > 195 "cannot specify local IPv4 address > without remote"); > 196 return -EINVAL; > 197 } > 198 > 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && > 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) { > 201 NL_SET_ERR_MSG_MOD(info->extack, > 202 "cannot specify local IPV6 address > without remote"); > 203 return -EINVAL; > 204 } LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't ok if remote is v4mapped. So those checks should go away and be replaced with the "get_family" thing, but that requires at most one of the _IPV4/_IPV6 attributes to be present to behave consistently. > > > > remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]); > > local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]); > > if (remote_family != local_family) { > > extack "incompatible address families"; > > return -EINVAL; > > } > > > > That would mirror the conversion that > > ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do. > > Yeah, pretty much what I was suggested, but in a more explicit manner. > I like it. Cool. BTW, I guess scope_id should only be used when it's not a v4mapped address? So the "cannot specify scope id without remote IPv6 address" check should probably use: if (remote_family != AF_INET6) (or split it into !attrs[OVPN_A_PEER_REMOTE_IPV6] and remote_family != AF_INET6 to have a fully specific extack message, but maybe that's overkill)
On 20/11/2024 12:12, Sabrina Dubroca wrote: > 2024-11-14, 10:21:18 +0100, Antonio Quartulli wrote: >> On 13/11/2024 17:56, Sabrina Dubroca wrote: >>> 2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote: >>>> On 04/11/2024 16:14, Sabrina Dubroca wrote: >>>>> 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >>>>>> +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, >>>>>> + struct genl_info *info, >>>>>> + struct nlattr **attrs) >>>>>> +{ >>>>>> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, >>>>>> + OVPN_A_PEER_ID)) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { >>>>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>>>> + "cannot specify both remote IPv4 or IPv6 address"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >>>>>> + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { >>>>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>>>> + "cannot specify remote port without IP address"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >>>>>> + attrs[OVPN_A_PEER_LOCAL_IPV4]) { >>>>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>>>> + "cannot specify local IPv4 address without remote"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && >>>>>> + attrs[OVPN_A_PEER_LOCAL_IPV6]) { >>>>> >>>>> I think these consistency checks should account for v4mapped >>>>> addresses. With remote=v4mapped and local=v6 we'll end up with an >>>>> incorrect ipv4 "local" address (taken out of the ipv6 address's first >>>>> 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped, >>>>> we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to >>>>> ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address) >>>>> out of that. >>>> >>>> Right, a v4mapped address would fool this check. >>>> How about checking if both or none addresses are v4mapped? This way we >>>> should prevent such cases. >>> >>> I don't know when userspace would use v4mapped addresses, >> >> It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY" >> set to true (you can check ipv6(7) for more details). >> This socket can receive IPv4 connections, which are implemented using >> v4mapped addresses. In this case both remote and local are going to be >> v4mapped. > > I'm familiar with v4mapped addresses, but I wasn't sure the userspace > part would actually passed them as peer. But I guess it would when the > peer connects over ipv4 on an ipv6 socket. > > So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never > happen? In that case I guess we just need to check that we got 2 > attributes of the same type (both _IPV4 or both _IPV6) and if we got > _IPV6, that they're either both v4mapped or both not. Might be a tiny > bit simpler than what I was suggesting below. Exactly - this is what I was originally suggesting, but your solution is just a bit cleaner imho. > >> However, the sanity check should make sure nobody can inject bogus >> combinations. >> >>> but treating >>> a v4mapped address as a "proper" ipv4 address should work with the >>> rest of the code, since you already have the conversion in >>> ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you >>> could do something like (rough idea and completely untested): >>> >>> static int get_family(attr_v4, attr_v6) >>> { >>> if (attr_v4) >>> return AF_INET; >>> if (attr_v6) { >>> if (ipv6_addr_v4mapped(attr_v6) >>> return AF_INET; >>> return AF_INET6; >>> } >>> return AF_UNSPEC; >>> } >>> >>> >>> // in _precheck: >>> // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check >>> // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6 >> >> the latter is already covered by: >> >> 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && >> 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) { >> 194 NL_SET_ERR_MSG_MOD(info->extack, >> 195 "cannot specify local IPv4 address >> without remote"); >> 196 return -EINVAL; >> 197 } >> 198 >> 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && >> 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) { >> 201 NL_SET_ERR_MSG_MOD(info->extack, >> 202 "cannot specify local IPV6 address >> without remote"); >> 203 return -EINVAL; >> 204 } > > LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is > v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't > ok if remote is v4mapped. So those checks should go away and be > replaced with the "get_family" thing, but that requires at most one of > the _IPV4/_IPV6 attributes to be present to behave consistently. I don't expect to receive a mix of _IPV4 and _IPV6, because the assumption is that either both addresses are v4mapped or none. Userspace fetches the addresses from the received packet, so I presume they will both be exposed as v4mapped if we are in this special case. Hence, I don't truly want to allow combining them. Does it make sense? > > >>> >>> remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]); >>> local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]); >>> if (remote_family != local_family) { >>> extack "incompatible address families"; >>> return -EINVAL; >>> } >>> >>> That would mirror the conversion that >>> ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do. >> >> Yeah, pretty much what I was suggested, but in a more explicit manner. >> I like it. > > Cool. > > BTW, I guess scope_id should only be used when it's not a v4mapped address? > So the "cannot specify scope id without remote IPv6 address" check > should probably use: > > if (remote_family != AF_INET6) Right! > > (or split it into !attrs[OVPN_A_PEER_REMOTE_IPV6] and remote_family != > AF_INET6 to have a fully specific extack message, but maybe that's > overkill) Yeah, maybe splitting works better. Thanks a lot! Regards, >
2024-11-20, 12:34:08 +0100, Antonio Quartulli wrote: > On 20/11/2024 12:12, Sabrina Dubroca wrote: [...] > > > > I don't know when userspace would use v4mapped addresses, > > > > > > It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY" > > > set to true (you can check ipv6(7) for more details). > > > This socket can receive IPv4 connections, which are implemented using > > > v4mapped addresses. In this case both remote and local are going to be > > > v4mapped. > > > > I'm familiar with v4mapped addresses, but I wasn't sure the userspace > > part would actually passed them as peer. But I guess it would when the > > peer connects over ipv4 on an ipv6 socket. > > > > So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never > > happen? In that case I guess we just need to check that we got 2 > > attributes of the same type (both _IPV4 or both _IPV6) and if we got > > _IPV6, that they're either both v4mapped or both not. Might be a tiny > > bit simpler than what I was suggesting below. > > Exactly - this is what I was originally suggesting, but your solution is > just a bit cleaner imho. Ok. > > > However, the sanity check should make sure nobody can inject bogus > > > combinations. > > > > > > > but treating > > > > a v4mapped address as a "proper" ipv4 address should work with the > > > > rest of the code, since you already have the conversion in > > > > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you > > > > could do something like (rough idea and completely untested): > > > > > > > > static int get_family(attr_v4, attr_v6) > > > > { > > > > if (attr_v4) > > > > return AF_INET; > > > > if (attr_v6) { > > > > if (ipv6_addr_v4mapped(attr_v6) > > > > return AF_INET; > > > > return AF_INET6; > > > > } > > > > return AF_UNSPEC; > > > > } > > > > > > > > > > > > // in _precheck: > > > > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check > > > > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6 > > > > > > the latter is already covered by: > > > > > > 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && > > > 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) { > > > 194 NL_SET_ERR_MSG_MOD(info->extack, > > > 195 "cannot specify local IPv4 address > > > without remote"); > > > 196 return -EINVAL; > > > 197 } > > > 198 > > > 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && > > > 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) { > > > 201 NL_SET_ERR_MSG_MOD(info->extack, > > > 202 "cannot specify local IPV6 address > > > without remote"); > > > 203 return -EINVAL; > > > 204 } > > > > LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is > > v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't > > ok if remote is v4mapped. So those checks should go away and be > > replaced with the "get_family" thing, but that requires at most one of > > the _IPV4/_IPV6 attributes to be present to behave consistently. > > I don't expect to receive a mix of _IPV4 and _IPV6, because the assumption > is that either both addresses are v4mapped or none. > > Userspace fetches the addresses from the received packet, so I presume they > will both be exposed as v4mapped if we are in this special case. > > Hence, I don't truly want to allow combining them. > > Does it make sense? Yup, thanks.
[I'm still thinking about the locking problems for ovpn_peer_float, but just noticed this while staring at the rehash code] 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) > + __must_hold(&peer->ovpn->peers->lock) > +{ > + struct hlist_nulls_head *nhead; > + > + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { > + /* remove potential old hashing */ > + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); s/hash_entry_transp_addr/hash_entry_addr4/ ? > + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, > + &peer->vpn_addrs.ipv4, > + sizeof(peer->vpn_addrs.ipv4)); > + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); > + } > + > + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { > + /* remove potential old hashing */ > + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); s/hash_entry_transp_addr/hash_entry_addr6/ ? > + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, > + &peer->vpn_addrs.ipv6, > + sizeof(peer->vpn_addrs.ipv6)); > + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); > + } > +}
On 21/11/2024 17:02, Sabrina Dubroca wrote: > [I'm still thinking about the locking problems for ovpn_peer_float, > but just noticed this while staring at the rehash code] > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: >> +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) >> + __must_hold(&peer->ovpn->peers->lock) >> +{ >> + struct hlist_nulls_head *nhead; >> + >> + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { >> + /* remove potential old hashing */ >> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); > > s/hash_entry_transp_addr/hash_entry_addr4/ ? cr0p. very good catch! Thanks > > >> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, >> + &peer->vpn_addrs.ipv4, >> + sizeof(peer->vpn_addrs.ipv4)); >> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); >> + } >> + >> + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { >> + /* remove potential old hashing */ >> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); > > s/hash_entry_transp_addr/hash_entry_addr6/ ? Thanks² This is what happens when you copy/paste code around. > > >> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, >> + &peer->vpn_addrs.ipv6, >> + sizeof(peer->vpn_addrs.ipv6)); >> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); >> + } >> +} > Regards,
2024-11-14, 11:32:36 +0100, Antonio Quartulli wrote: > On 13/11/2024 12:05, Sabrina Dubroca wrote: > > 2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote: > > > On 11/11/2024 16:41, Sabrina Dubroca wrote: > > > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > > > > > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) > > > > > + __must_hold(&peer->ovpn->peers->lock) > > > > > > > > Changes to peer->vpn_addrs are not protected by peers->lock, so those > > > > could be getting updated while we're rehashing (and taking peer->lock > > > > in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent > > > > that). > > > > > > > > > > /me screams :-D > > > > Sorry :) > > > > > Indeed peers->lock is only about protecting the lists, not the content of > > > the listed objects. > > > > > > How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? > > > > It seems like it would work. Maybe a bit weird to have conditional > > locking (MP mode only), but ok. You already have this lock ordering > > (hold peers->lock before taking peer->lock) in > > ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing > > the same thing in the netlink code. > > Yeah. > > > > > Then I would also do that in ovpn_peer_float to protect that rehash. > > I am not extremely comfortable with this, because it means acquiring > peers->lock on every packet (right now we do so only on peer->lock) and it > may defeat the advantage of the RCU locking on the hashtables. > Wouldn't you agree? Hmpf, yeah. Then I think you could keep most of the current code, except doing the rehash under both locks (peers + peer), and get ss+sa_len for the rehash directly from peer->bind (instead of using the ones we just defined locally in ovpn_peer_float, since they may have changed while we released peer->lock to grab peers->lock). We may end up "rehashing" twice into the same bucket if we have 2 concurrent peer_float calls (call 1 sets remote r1, call 2 sets a new one r2, call 1 hashes according to r2, call 2 also rehashes based on r2). That should be ok (it can happen anyway that a "real" rehash lands in the same bucket). peer_float { spin_lock(peer) match/update bind spin_unlock(peer) if (MP) { spin_lock(peers) spin_lock(peer) rehash using peer->bind->remote rather than ss spin_unlock(peer) spin_unlock(peers) } } Does that sound reasonable?
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index 2cc34eb1d1d870c6705714cb971c3c5dfb04afda..d504445325ef82db04f87367c858adaf025f6297 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c @@ -7,6 +7,7 @@ */ #include <linux/netdevice.h> +#include <linux/types.h> #include <net/genetlink.h> #include <uapi/linux/ovpn.h> @@ -16,6 +17,10 @@ #include "io.h" #include "netlink.h" #include "netlink-gen.h" +#include "bind.h" +#include "packet.h" +#include "peer.h" +#include "socket.h" MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME); @@ -86,29 +91,592 @@ void ovpn_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, netdev_put(ovpn->dev, &ovpn->dev_tracker); } +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 + */ + return af; +} + +static u8 *ovpn_nl_attr_local_ip(struct nlattr **attrs) +{ + u8 *addr6; + + if (!attrs[OVPN_A_PEER_LOCAL_IPV4] && !attrs[OVPN_A_PEER_LOCAL_IPV6]) + return NULL; + + if (attrs[OVPN_A_PEER_LOCAL_IPV4]) + return nla_data(attrs[OVPN_A_PEER_LOCAL_IPV4]); + + addr6 = nla_data(attrs[OVPN_A_PEER_LOCAL_IPV6]); + /* this is an IPv4-mapped IPv6 address, therefore extract the actual + * v4 address from the last 4 bytes + */ + if (ipv6_addr_v4mapped((struct in6_addr *)addr6)) + return addr6 + 12; + + return addr6; +} + +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn, + struct genl_info *info, + struct nlattr **attrs) +{ + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, + OVPN_A_PEER_ID)) + return -EINVAL; + + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) { + NL_SET_ERR_MSG_MOD(info->extack, + "cannot specify both remote IPv4 or IPv6 address"); + return -EINVAL; + } + + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) { + NL_SET_ERR_MSG_MOD(info->extack, + "cannot specify remote port without IP address"); + return -EINVAL; + } + + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] && + attrs[OVPN_A_PEER_LOCAL_IPV4]) { + NL_SET_ERR_MSG_MOD(info->extack, + "cannot specify local IPv4 address without remote"); + return -EINVAL; + } + + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && + attrs[OVPN_A_PEER_LOCAL_IPV6]) { + NL_SET_ERR_MSG_MOD(info->extack, + "cannot specify local IPV6 address without remote"); + return -EINVAL; + } + + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] && + attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID]) { + NL_SET_ERR_MSG_MOD(info->extack, + "cannot specify scope id without remote IPv6 address"); + return -EINVAL; + } + + /* 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])) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "VPN IP unexpected in P2P mode"); + return -EINVAL; + } + + if ((attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] && + !attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) || + (!attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] && + attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT])) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "keepalive interval and timeout are required together"); + return -EINVAL; + } + + return 0; +} + +/** + * ovpn_nl_peer_modify - modify the peer attributes according to the incoming msg + * @peer: the peer to modify + * @info: generic netlink info from the user request + * @attrs: the attributes from the user request + * + * Return: a negative error code in case of failure, 0 on success or 1 on + * success and the VPN IPs have been modified (requires rehashing in MP + * mode) + */ +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info, + struct nlattr **attrs) +{ + struct sockaddr_storage ss = {}; + u32 sockfd, interv, timeout; + struct socket *sock = NULL; + u8 *local_ip = NULL; + bool rehash = false; + int ret; + + if (attrs[OVPN_A_PEER_SOCKET]) { + /* lookup the fd in the kernel table and extract the socket + * object + */ + sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]); + /* sockfd_lookup() increases sock's refcounter */ + sock = sockfd_lookup(sockfd, &ret); + if (!sock) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "cannot lookup peer socket (fd=%u): %d", + sockfd, ret); + return -ENOTSOCK; + } + + /* 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])) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "unexpected remote IP address for non UDP socket"); + sockfd_put(sock); + return -EINVAL; + } + + if (peer->sock) + ovpn_socket_put(peer->sock); + + peer->sock = ovpn_socket_new(sock, peer); + if (IS_ERR(peer->sock)) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "cannot encapsulate socket: %ld", + PTR_ERR(peer->sock)); + sockfd_put(sock); + peer->sock = NULL; + return -ENOTSOCK; + } + } + + if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) { + /* we carry the local IP in a generic container. + * ovpn_peer_reset_sockaddr() will properly interpret it + * based on ss.ss_family + */ + local_ip = ovpn_nl_attr_local_ip(attrs); + + spin_lock_bh(&peer->lock); + /* set peer sockaddr */ + ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip); + if (ret < 0) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "cannot set peer sockaddr: %d", + ret); + spin_unlock_bh(&peer->lock); + return ret; + } + spin_unlock_bh(&peer->lock); + } + + if (attrs[OVPN_A_PEER_VPN_IPV4]) { + rehash = true; + peer->vpn_addrs.ipv4.s_addr = + nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]); + } + + if (attrs[OVPN_A_PEER_VPN_IPV6]) { + rehash = true; + peer->vpn_addrs.ipv6 = + nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]); + } + + /* when setting the keepalive, both parameters have to be configured */ + if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] && + attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) { + interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]); + timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]); + ovpn_peer_keepalive_set(peer, interv, timeout); + } + + netdev_dbg(peer->ovpn->dev, + "%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n", + __func__, peer->id, &ss, + peer->sock->sock->sk->sk_prot_creator->name, + &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6); + + return rehash ? 1 : 0; +} + int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info) { - return -EOPNOTSUPP; + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; + struct ovpn_struct *ovpn = info->user_ptr[0]; + struct ovpn_peer *peer; + u32 peer_id; + int ret; + + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) + return -EINVAL; + + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], + ovpn_peer_nl_policy, info->extack); + if (ret) + return ret; + + ret = ovpn_nl_peer_precheck(ovpn, info, attrs); + if (ret < 0) + return ret; + + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, + OVPN_A_PEER_SOCKET)) + return -EINVAL; + + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]); + peer = ovpn_peer_new(ovpn, peer_id); + if (IS_ERR(peer)) { + NL_SET_ERR_MSG_FMT_MOD(info->extack, + "cannot create new peer object for peer %u: %ld", + peer_id, PTR_ERR(peer)); + return PTR_ERR(peer); + } + + 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: + /* 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) { - return -EOPNOTSUPP; + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; + struct ovpn_struct *ovpn = info->user_ptr[0]; + struct ovpn_peer *peer; + u32 peer_id; + int ret; + + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) + return -EINVAL; + + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], + ovpn_peer_nl_policy, info->extack); + if (ret) + return ret; + + ret = ovpn_nl_peer_precheck(ovpn, info, attrs); + if (ret < 0) + return ret; + + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]); + peer = ovpn_peer_get_by_id(ovpn, peer_id); + if (!peer) + return -ENOENT; + + ret = ovpn_nl_peer_modify(peer, info, attrs); + if (ret < 0) { + ovpn_peer_put(peer); + return ret; + } + + /* ret == 1 means that VPN IPv4/6 has been modified and rehashing + * is required + */ + if (ret > 0) { + spin_lock_bh(&ovpn->peers->lock); + ovpn_peer_hash_vpn_ip(peer); + spin_unlock_bh(&ovpn->peers->lock); + } + + ovpn_peer_put(peer); + + return 0; +} + +static int ovpn_nl_send_peer(struct sk_buff *skb, const struct genl_info *info, + const struct ovpn_peer *peer, u32 portid, u32 seq, + int flags) +{ + const struct ovpn_bind *bind; + struct nlattr *attr; + void *hdr; + + hdr = genlmsg_put(skb, portid, seq, &ovpn_nl_family, flags, + OVPN_CMD_PEER_GET); + if (!hdr) + return -ENOBUFS; + + attr = nla_nest_start(skb, OVPN_A_PEER); + if (!attr) + goto err; + + if (nla_put_u32(skb, OVPN_A_PEER_ID, peer->id)) + goto err; + + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) + if (nla_put_in_addr(skb, OVPN_A_PEER_VPN_IPV4, + peer->vpn_addrs.ipv4.s_addr)) + goto err; + + if (!ipv6_addr_equal(&peer->vpn_addrs.ipv6, &in6addr_any)) + if (nla_put_in6_addr(skb, OVPN_A_PEER_VPN_IPV6, + &peer->vpn_addrs.ipv6)) + goto err; + + if (nla_put_u32(skb, OVPN_A_PEER_KEEPALIVE_INTERVAL, + peer->keepalive_interval) || + nla_put_u32(skb, OVPN_A_PEER_KEEPALIVE_TIMEOUT, + peer->keepalive_timeout)) + goto err; + + rcu_read_lock(); + bind = rcu_dereference(peer->bind); + if (bind) { + if (bind->remote.in4.sin_family == AF_INET) { + if (nla_put_in_addr(skb, OVPN_A_PEER_REMOTE_IPV4, + bind->remote.in4.sin_addr.s_addr) || + nla_put_net16(skb, OVPN_A_PEER_REMOTE_PORT, + bind->remote.in4.sin_port) || + nla_put_in_addr(skb, OVPN_A_PEER_LOCAL_IPV4, + bind->local.ipv4.s_addr)) + goto err_unlock; + } else if (bind->remote.in4.sin_family == AF_INET6) { + if (nla_put_in6_addr(skb, OVPN_A_PEER_REMOTE_IPV6, + &bind->remote.in6.sin6_addr) || + nla_put_u32(skb, OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID, + bind->remote.in6.sin6_scope_id) || + nla_put_net16(skb, OVPN_A_PEER_REMOTE_PORT, + bind->remote.in6.sin6_port) || + nla_put_in6_addr(skb, OVPN_A_PEER_LOCAL_IPV6, + &bind->local.ipv6)) + goto err_unlock; + } + } + rcu_read_unlock(); + + if (nla_put_net16(skb, OVPN_A_PEER_LOCAL_PORT, + inet_sk(peer->sock->sock->sk)->inet_sport) || + /* VPN RX stats */ + nla_put_uint(skb, OVPN_A_PEER_VPN_RX_BYTES, + atomic64_read(&peer->vpn_stats.rx.bytes)) || + nla_put_uint(skb, OVPN_A_PEER_VPN_RX_PACKETS, + atomic64_read(&peer->vpn_stats.rx.packets)) || + /* VPN TX stats */ + nla_put_uint(skb, OVPN_A_PEER_VPN_TX_BYTES, + atomic64_read(&peer->vpn_stats.tx.bytes)) || + nla_put_uint(skb, OVPN_A_PEER_VPN_TX_PACKETS, + atomic64_read(&peer->vpn_stats.tx.packets)) || + /* link RX stats */ + nla_put_uint(skb, OVPN_A_PEER_LINK_RX_BYTES, + atomic64_read(&peer->link_stats.rx.bytes)) || + nla_put_uint(skb, OVPN_A_PEER_LINK_RX_PACKETS, + atomic64_read(&peer->link_stats.rx.packets)) || + /* link TX stats */ + nla_put_uint(skb, OVPN_A_PEER_LINK_TX_BYTES, + atomic64_read(&peer->link_stats.tx.bytes)) || + nla_put_uint(skb, OVPN_A_PEER_LINK_TX_PACKETS, + atomic64_read(&peer->link_stats.tx.packets))) + goto err; + + nla_nest_end(skb, attr); + genlmsg_end(skb, hdr); + + return 0; +err_unlock: + rcu_read_unlock(); +err: + genlmsg_cancel(skb, hdr); + return -EMSGSIZE; } int ovpn_nl_peer_get_doit(struct sk_buff *skb, struct genl_info *info) { - return -EOPNOTSUPP; + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; + struct ovpn_struct *ovpn = info->user_ptr[0]; + struct ovpn_peer *peer; + struct sk_buff *msg; + u32 peer_id; + int ret; + + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) + return -EINVAL; + + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], + ovpn_peer_nl_policy, info->extack); + if (ret) + return ret; + + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, + OVPN_A_PEER_ID)) + 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; + } + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) { + ret = -ENOMEM; + goto err; + } + + ret = ovpn_nl_send_peer(msg, info, peer, info->snd_portid, + info->snd_seq, 0); + if (ret < 0) { + nlmsg_free(msg); + goto err; + } + + ret = genlmsg_reply(msg, info); +err: + ovpn_peer_put(peer); + return ret; } int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) { - return -EOPNOTSUPP; + const struct genl_info *info = genl_info_dump(cb); + int bkt, last_idx = cb->args[1], dumped = 0; + struct ovpn_struct *ovpn; + struct ovpn_peer *peer; + + ovpn = ovpn_get_dev_from_attrs(sock_net(cb->skb->sk), info); + if (IS_ERR(ovpn)) + return PTR_ERR(ovpn); + + if (ovpn->mode == OVPN_MODE_P2P) { + /* if we already dumped a peer it means we are done */ + if (last_idx) + goto out; + + rcu_read_lock(); + peer = rcu_dereference(ovpn->peer); + if (peer) { + if (ovpn_nl_send_peer(skb, info, peer, + NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, + NLM_F_MULTI) == 0) + dumped++; + } + rcu_read_unlock(); + } else { + rcu_read_lock(); + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer, + hash_entry_id) { + /* skip already dumped peers that were dumped by + * previous invocations + */ + if (last_idx > 0) { + last_idx--; + continue; + } + + if (ovpn_nl_send_peer(skb, info, peer, + NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, + NLM_F_MULTI) < 0) + break; + + /* count peers being dumped during this invocation */ + dumped++; + } + rcu_read_unlock(); + } + +out: + netdev_put(ovpn->dev, &ovpn->dev_tracker); + + /* sum up peers dumped in this message, so that at the next invocation + * we can continue from where we left + */ + cb->args[1] += dumped; + return skb->len; } int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info) { - return -EOPNOTSUPP; + struct nlattr *attrs[OVPN_A_PEER_MAX + 1]; + struct ovpn_struct *ovpn = info->user_ptr[0]; + struct ovpn_peer *peer; + u32 peer_id; + int ret; + + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER)) + return -EINVAL; + + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER], + ovpn_peer_nl_policy, info->extack); + if (ret) + return ret; + + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs, + OVPN_A_PEER_ID)) + return -EINVAL; + + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]); + + peer = ovpn_peer_get_by_id(ovpn, peer_id); + if (!peer) + return -ENOENT; + + netdev_dbg(ovpn->dev, "%s: peer id=%u\n", __func__, peer->id); + ret = ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_USERSPACE); + ovpn_peer_put(peer); + + return ret; } int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info) diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c index da6215bbb643592e4567e61e4b4976d367ed109c..8cfe1997ec116ae4fe74cd7105d228569e2a66a9 100644 --- a/drivers/net/ovpn/peer.c +++ b/drivers/net/ovpn/peer.c @@ -102,9 +102,9 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) * * Return: 0 on success or a negative error code otherwise */ -static int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, - const struct sockaddr_storage *ss, - const u8 *local_ip) +int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, + const struct sockaddr_storage *ss, + const u8 *local_ip) __must_hold(&peer->lock) { struct ovpn_bind *bind; @@ -219,7 +219,7 @@ void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb) rcu_read_unlock(); } -static void ovpn_peer_release(struct ovpn_peer *peer) +void ovpn_peer_release(struct ovpn_peer *peer) { if (peer->sock) ovpn_socket_put(peer->sock); @@ -763,6 +763,32 @@ bool ovpn_peer_check_by_src(struct ovpn_struct *ovpn, struct sk_buff *skb, return match; } +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) + __must_hold(&peer->ovpn->peers->lock) +{ + struct hlist_nulls_head *nhead; + + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { + /* remove potential old hashing */ + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); + + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, + &peer->vpn_addrs.ipv4, + sizeof(peer->vpn_addrs.ipv4)); + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); + } + + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { + /* remove potential old hashing */ + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); + + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr, + &peer->vpn_addrs.ipv6, + sizeof(peer->vpn_addrs.ipv6)); + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); + } +} + /** * ovpn_peer_add_mp - add peer to related tables in a MP instance * @ovpn: the instance to add the peer to @@ -824,19 +850,7 @@ static int ovpn_peer_add_mp(struct ovpn_struct *ovpn, struct ovpn_peer *peer) ovpn_get_hash_head(ovpn->peers->by_id, &peer->id, sizeof(peer->id))); - if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) { - nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr, - &peer->vpn_addrs.ipv4, - sizeof(peer->vpn_addrs.ipv4)); - hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead); - } - - if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) { - nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr, - &peer->vpn_addrs.ipv6, - sizeof(peer->vpn_addrs.ipv6)); - hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead); - } + ovpn_peer_hash_vpn_ip(peer); out: spin_unlock_bh(&ovpn->peers->lock); return ret; diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h index 940cea5372ec0375cfe3e673154a1e0248978409..1adecd0f79f8f4a202110543223fc448c09e5175 100644 --- a/drivers/net/ovpn/peer.h +++ b/drivers/net/ovpn/peer.h @@ -124,6 +124,7 @@ static inline bool ovpn_peer_hold(struct ovpn_peer *peer) return kref_get_unless_zero(&peer->refcount); } +void ovpn_peer_release(struct ovpn_peer *peer); void ovpn_peer_release_kref(struct kref *kref); /** @@ -146,6 +147,7 @@ struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id); struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn, struct sk_buff *skb); +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer); bool ovpn_peer_check_by_src(struct ovpn_struct *ovpn, struct sk_buff *skb, struct ovpn_peer *peer); @@ -156,5 +158,8 @@ void ovpn_peer_update_local_endpoint(struct ovpn_peer *peer, struct sk_buff *skb); void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb); +int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, + const struct sockaddr_storage *ss, + const u8 *local_ip); #endif /* _NET_OVPN_OVPNPEER_H_ */
This change introduces the netlink command needed to add, delete and retrieve/dump known peers. Userspace is expected to use these commands to handle known peer lifecycles. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/netlink.c | 578 ++++++++++++++++++++++++++++++++++++++++++++- drivers/net/ovpn/peer.c | 48 ++-- drivers/net/ovpn/peer.h | 5 + 3 files changed, 609 insertions(+), 22 deletions(-)