Message-ID: <76191b85-6844-4a85-bb9c-ad19aa5110c5@openvpn.net>
Date: Tue, 12 Nov 2024 15:19:50 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 18/23] ovpn: implement peer
 add/get/dump/delete via netlink

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

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists