Re: [PATCH net-next v12 17/22] ovpn: implement peer add/get/dump/delete via netlink

On 12/2/24 16:07, Antonio Quartulli wrote:

+/**

• • 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]) {

• if (peer->sock) {
  

• 	NL_SET_ERR_MSG_FMT_MOD(info->extack,
  

• 			       "peer socket can't be modified");
  

• 	return -EINVAL;
  

• }
  

• /* 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;
  

• }
  

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

This looks race-prone. If any other CPU can do concurrent read access to peer->sock it could observe an invalid pointer Even if such race does not exist, it would be cleaner store ovpn_socket_new() return value in a local variable and set peer->sock only on successful creation.

/P