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

[Antonio Quartulli]

On 03/12/2024 18:46, Paolo Abeni wrote:
> 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.

Yeah, this race is not possible because at this time the peer is not 
hashed yet, so it only exists in this local 'peer' variable.

However, you're not the first one to comment this piece of code.
I'll definitely switch to using a local variable for the sock.

Thanks!

Regards,

--
Antonio Quartulli
OpenVPN Inc.