Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink

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);
  

• }

+}