diff mbox series

[net-next,v5,20/25] ovpn: implement peer add/dump/delete via netlink

Message ID 20240627130843.21042-21-antonio@openvpn.net (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introducing OpenVPN Data Channel Offload | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 2612 insertions(+);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 842 this patch: 842
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: openvpn-devel@lists.sourceforge.net
netdev/build_clang success Errors and warnings before: 849 this patch: 849
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 846 this patch: 846
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 560 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-28--06-00 (tests: 666)

Commit Message

Antonio Quartulli June 27, 2024, 1:08 p.m. UTC
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 | 487 ++++++++++++++++++++++++++++++++++++-
 drivers/net/ovpn/peer.c    |   8 +-
 drivers/net/ovpn/peer.h    |   4 +
 3 files changed, 490 insertions(+), 9 deletions(-)

Comments

Sabrina Dubroca July 16, 2024, 1:41 p.m. UTC | #1
2024-06-27, 15:08:38 +0200, Antonio Quartulli wrote:
> @@ -29,7 +34,7 @@ MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME);
>   * Return: the netdevice, if found, or an error otherwise
>   */
>  static struct net_device *
> -ovpn_get_dev_from_attrs(struct net *net, struct genl_info *info)
> +ovpn_get_dev_from_attrs(struct net *net, const struct genl_info *info)

nit: this should be squashed into "add basic netlink support"


[...]
>  int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return -EOPNOTSUPP;
> +	bool keepalive_set = false, new_peer = false;
> +	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
> +	struct ovpn_struct *ovpn = info->user_ptr[0];
> +	struct sockaddr_storage *ss = NULL;
> +	u32 sockfd, id, interv, timeout;
> +	struct socket *sock = NULL;
> +	struct sockaddr_in mapped;
> +	struct sockaddr_in6 *in6;
> +	struct ovpn_peer *peer;
> +	u8 *local_ip = NULL;
> +	size_t sa_len;
> +	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;
> +
> +	id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
> +	/* check if the peer exists first, otherwise create a new one */
> +	peer = ovpn_peer_get_by_id(ovpn, id);
> +	if (!peer) {
> +		peer = ovpn_peer_new(ovpn, id);
> +		new_peer = true;
> +		if (IS_ERR(peer)) {
> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +					       "cannot create new peer object for peer %u (sockaddr=%pIScp): %ld",
> +					       id, ss, PTR_ERR(peer));

ss hasn't been set yet at this point, including it in the extack
message is not useful.

> +			return PTR_ERR(peer);
> +		}
> +	}
> +
> +	if (new_peer && NL_REQ_ATTR_CHECK(info->extack,
> +					  info->attrs[OVPN_A_PEER], attrs,
> +					  OVPN_A_PEER_SOCKET)) {

This can be checked at the start of the previous block (!peer), we'd
avoid a pointless peer allocation.

(and the linebreaks in NL_REQ_ATTR_CHECK end up being slightly better
because you don't need the "new_peer &&" test that is wider than the
tab used to indent the !peer block :))

> +		ret = -EINVAL;
> +		goto peer_release;
> +	}
> +
> +	if (new_peer && ovpn->mode == OVPN_MODE_MP &&
> +	    !attrs[OVPN_A_PEER_VPN_IPV4] && !attrs[OVPN_A_PEER_VPN_IPV6]) {

Same for this check.

> +		NL_SET_ERR_MSG_MOD(info->extack,
> +				   "a VPN IP is required when adding a peer in MP mode");
> +		ret = -EINVAL;
> +		goto peer_release;
> +	}
> +
> +	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);
> +			ret = -ENOTSOCK;
> +			goto peer_release;
> +		}
> +
> +		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;

Is there any value for the client in keeping the old peer->sock
assigned if we fail here?

ie something like:

    tmp = ovpn_socket_new(sock, peer);
    if (IS_ERR(tmp)) {
        ...
        goto peer_release;
    }
    if (peer->sock)
        ovpn_socket_put(peer->sock);
    peer->sock = tmp;


But if it's just going to get rid of the old socket and the whole
association/peer on failure, probably not.

> +			ret = -ENOTSOCK;
> +			goto peer_release;
> +		}
> +	}
> +
> +	/* 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.

(that should also work with UDP "connected" sockets)


> +	 */
> +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP &&
> +	    attrs[OVPN_A_PEER_SOCKADDR_REMOTE]) {
[...]
> +
> +		if (attrs[OVPN_A_PEER_LOCAL_IP]) {
> +			local_ip = ovpn_nl_attr_local_ip(info, ovpn,
> +							 attrs,
> +							 ss->ss_family);
> +			if (IS_ERR(local_ip)) {
> +				ret = PTR_ERR(local_ip);
> +				NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +						       "cannot retrieve local IP: %d",
> +						       ret);

ovpn_nl_attr_local_ip already sets a more specific extack message,
this is unnecessary.

> +				goto peer_release;
> +			}
> +		}
> +
> +		/* 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);
> +			goto peer_release;
> +		}
> +	}

I would reject OVPN_A_PEER_SOCKADDR_REMOTE for a non-UDP socket.


> +	/* VPN IPs cannot be updated, because they are hashed */

Then I think there should be something like

    if (!new_peer && (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
        NL_SET_ERR_MSG_FMT_MOD(... "can't update ip");
        ret = -EINVAL;
        goto peer_release;
    }

(just after getting the peer, before any changes have actually been
made)

And if they are only used in MP mode, I would maybe also reject
requests where mode==P2P and OVPN_A_PEER_VPN_IPV* is provided.


> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
> +		peer->vpn_addrs.ipv4.s_addr =
> +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> +
> +	/* VPN IPs cannot be updated, because they are hashed */
> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
> +		peer->vpn_addrs.ipv6 =
> +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> +
> +	/* when setting the keepalive, both parameters have to be configured */

Then I would also reject a config where only one is set (also before any
changes have been made).

> +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> +		keepalive_set = true;
> +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> +	}
> +
> +	if (keepalive_set)
> +		ovpn_peer_keepalive_set(peer, interv, timeout);

Why not skip the bool and just do this in the previous block?

> +	netdev_dbg(ovpn->dev,
> +		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
> +		   __func__, (new_peer ? "adding" : "modifying"), ss,
> +		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
> +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> +
> +	if (new_peer) {
> +		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;
> +		}
> +	} else {
> +		ovpn_peer_put(peer);
> +	}
> +
> +	return 0;
> +
> +peer_release:
> +	if (new_peer) {
> +		/* release right away because peer is not really used in any
> +		 * context
> +		 */
> +		ovpn_peer_release(peer);
> +		kfree(peer);

I don't think that's correct, the new peer was created with
ovpn_peer_new, so it took a reference on the netdevice
(netdev_hold(ovpn->dev, ...)), which isn't released by
ovpn_peer_release. Why not just go through ovpn_peer_put?

> +	} else {
> +		ovpn_peer_put(peer);
> +	}
> +
> +	return ret;
> +}
> +

[...]
>  int ovpn_nl_get_peer_doit(struct sk_buff *skb, struct genl_info *info)
>  {
[...]
> +	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)

Missing ovpn_peer_put?

> +		return -ENOMEM;
> +
> +	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;
>  }
Antonio Quartulli July 17, 2024, 2:04 p.m. UTC | #2
On 16/07/2024 15:41, Sabrina Dubroca wrote:
> 2024-06-27, 15:08:38 +0200, Antonio Quartulli wrote:
>> @@ -29,7 +34,7 @@ MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME);
>>    * Return: the netdevice, if found, or an error otherwise
>>    */
>>   static struct net_device *
>> -ovpn_get_dev_from_attrs(struct net *net, struct genl_info *info)
>> +ovpn_get_dev_from_attrs(struct net *net, const struct genl_info *info)
> 
> nit: this should be squashed into "add basic netlink support"

Right, thanks

> 
> 
> [...]
>>   int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -EOPNOTSUPP;
>> +	bool keepalive_set = false, new_peer = false;
>> +	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
>> +	struct ovpn_struct *ovpn = info->user_ptr[0];
>> +	struct sockaddr_storage *ss = NULL;
>> +	u32 sockfd, id, interv, timeout;
>> +	struct socket *sock = NULL;
>> +	struct sockaddr_in mapped;
>> +	struct sockaddr_in6 *in6;
>> +	struct ovpn_peer *peer;
>> +	u8 *local_ip = NULL;
>> +	size_t sa_len;
>> +	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;
>> +
>> +	id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
>> +	/* check if the peer exists first, otherwise create a new one */
>> +	peer = ovpn_peer_get_by_id(ovpn, id);
>> +	if (!peer) {
>> +		peer = ovpn_peer_new(ovpn, id);
>> +		new_peer = true;
>> +		if (IS_ERR(peer)) {
>> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +					       "cannot create new peer object for peer %u (sockaddr=%pIScp): %ld",
>> +					       id, ss, PTR_ERR(peer));
> 
> ss hasn't been set yet at this point, including it in the extack
> message is not useful.

argh. you are correct. I'll remove it.

> 
>> +			return PTR_ERR(peer);
>> +		}
>> +	}
>> +
>> +	if (new_peer && NL_REQ_ATTR_CHECK(info->extack,
>> +					  info->attrs[OVPN_A_PEER], attrs,
>> +					  OVPN_A_PEER_SOCKET)) {
> 
> This can be checked at the start of the previous block (!peer), we'd
> avoid a pointless peer allocation.

Right - will move it up.

> 
> (and the linebreaks in NL_REQ_ATTR_CHECK end up being slightly better
> because you don't need the "new_peer &&" test that is wider than the
> tab used to indent the !peer block :))

good point! :-D

> 
>> +		ret = -EINVAL;
>> +		goto peer_release;
>> +	}
>> +
>> +	if (new_peer && ovpn->mode == OVPN_MODE_MP &&
>> +	    !attrs[OVPN_A_PEER_VPN_IPV4] && !attrs[OVPN_A_PEER_VPN_IPV6]) {
> 
> Same for this check.

ACK

> 
>> +		NL_SET_ERR_MSG_MOD(info->extack,
>> +				   "a VPN IP is required when adding a peer in MP mode");
>> +		ret = -EINVAL;
>> +		goto peer_release;
>> +	}
>> +
>> +	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);
>> +			ret = -ENOTSOCK;
>> +			goto peer_release;
>> +		}
>> +
>> +		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;
> 
> Is there any value for the client in keeping the old peer->sock
> assigned if we fail here?
> 
> ie something like:
> 
>      tmp = ovpn_socket_new(sock, peer);
>      if (IS_ERR(tmp)) {
>          ...
>          goto peer_release;
>      }
>      if (peer->sock)
>          ovpn_socket_put(peer->sock);
>      peer->sock = tmp;
> 
> 
> But if it's just going to get rid of the old socket and the whole
> association/peer on failure, probably not.

Right. if attaching the new socket fails, we are entering some broken 
status which is not worth keeping around.

> 
>> +			ret = -ENOTSOCK;
>> +			goto peer_release;
>> +		}
>> +	}
>> +
>> +	/* 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.
> 
> (that should also work with UDP "connected" sockets)

True, but those are not used in openvpn. In case of UDP, userspace just 
creates one socket and uses it for all peers.
I will add a note about 'connected UDP socket' in the comment, to clear 
this out.

> 
> 
>> +	 */
>> +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP &&
>> +	    attrs[OVPN_A_PEER_SOCKADDR_REMOTE]) {
> [...]
>> +
>> +		if (attrs[OVPN_A_PEER_LOCAL_IP]) {
>> +			local_ip = ovpn_nl_attr_local_ip(info, ovpn,
>> +							 attrs,
>> +							 ss->ss_family);
>> +			if (IS_ERR(local_ip)) {
>> +				ret = PTR_ERR(local_ip);
>> +				NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +						       "cannot retrieve local IP: %d",
>> +						       ret);
> 
> ovpn_nl_attr_local_ip already sets a more specific extack message,
> this is unnecessary.

right.

> 
>> +				goto peer_release;
>> +			}
>> +		}
>> +
>> +		/* 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);
>> +			goto peer_release;
>> +		}
>> +	}
> 
> I would reject OVPN_A_PEER_SOCKADDR_REMOTE for a non-UDP socket.

judging from the comments below, it seems you prefer to reject unneeded 
attributes. OTOH I took the opposite approach (just ignore those).

However, I was actually looking for some preference/indication regarding 
this point and I now I got one :-)

I will be strict and return -EINVAL when unneded attributes are present.

> 
> 
>> +	/* VPN IPs cannot be updated, because they are hashed */
> 
> Then I think there should be something like
> 
>      if (!new_peer && (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
>          NL_SET_ERR_MSG_FMT_MOD(... "can't update ip");
>          ret = -EINVAL;
>          goto peer_release;
>      }
> 
> (just after getting the peer, before any changes have actually been
> made)

ACK

> 
> And if they are only used in MP mode, I would maybe also reject
> requests where mode==P2P and OVPN_A_PEER_VPN_IPV* is provided.

yup, like I commented above.

> 
> 
>> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
>> +		peer->vpn_addrs.ipv4.s_addr =
>> +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
>> +
>> +	/* VPN IPs cannot be updated, because they are hashed */
>> +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
>> +		peer->vpn_addrs.ipv6 =
>> +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
>> +
>> +	/* when setting the keepalive, both parameters have to be configured */
> 
> Then I would also reject a config where only one is set (also before any
> changes have been made).

ok

> 
>> +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
>> +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
>> +		keepalive_set = true;
>> +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
>> +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
>> +	}
>> +
>> +	if (keepalive_set)
>> +		ovpn_peer_keepalive_set(peer, interv, timeout);
> 
> Why not skip the bool and just do this in the previous block?

I am pretty sure there was a reason...but it may have faded away after 
the 95-th rebase hehe. Thanks for spotting this!

> 
>> +	netdev_dbg(ovpn->dev,
>> +		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
>> +		   __func__, (new_peer ? "adding" : "modifying"), ss,
>> +		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
>> +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
>> +
>> +	if (new_peer) {
>> +		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;
>> +		}
>> +	} else {
>> +		ovpn_peer_put(peer);
>> +	}
>> +
>> +	return 0;
>> +
>> +peer_release:
>> +	if (new_peer) {
>> +		/* release right away because peer is not really used in any
>> +		 * context
>> +		 */
>> +		ovpn_peer_release(peer);
>> +		kfree(peer);
> 
> I don't think that's correct, the new peer was created with
> ovpn_peer_new, so it took a reference on the netdevice
> (netdev_hold(ovpn->dev, ...)), which isn't released by
> ovpn_peer_release. Why not just go through ovpn_peer_put?

Because then we would send the notification to userspace, but it is not 
correct to do so, because the new() is just about to return an error.

I presume I should just move netdev_put(peer->ovpn->dev, NULL); to 
ovpn_peer_release(). That will take care of this case too.

> 
>> +	} else {
>> +		ovpn_peer_put(peer);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
> 
> [...]
>>   int ovpn_nl_get_peer_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> [...]
>> +	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)
> 
> Missing ovpn_peer_put?

indeed. have to set ret and goto err;

> 
>> +		return -ENOMEM;
>> +
>> +	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;
>>   }
> 


Thanks!
Sabrina Dubroca July 17, 2024, 3:37 p.m. UTC | #3
2024-07-17, 16:04:25 +0200, Antonio Quartulli wrote:
> On 16/07/2024 15:41, Sabrina Dubroca wrote:
> > 2024-06-27, 15:08:38 +0200, Antonio Quartulli wrote:
> > > +	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);
> > > +			ret = -ENOTSOCK;
> > > +			goto peer_release;
> > > +		}
> > > +
> > > +		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;
> > 
> > Is there any value for the client in keeping the old peer->sock
> > assigned if we fail here?
> > 
> > ie something like:
> > 
> >      tmp = ovpn_socket_new(sock, peer);
> >      if (IS_ERR(tmp)) {
> >          ...
> >          goto peer_release;
> >      }
> >      if (peer->sock)
> >          ovpn_socket_put(peer->sock);
> >      peer->sock = tmp;
> > 
> > 
> > But if it's just going to get rid of the old socket and the whole
> > association/peer on failure, probably not.
> 
> Right. if attaching the new socket fails, we are entering some broken status
> which is not worth keeping around.

Ok, then the current code is fine, thanks.


> > > +	/* 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.
> > 
> > (that should also work with UDP "connected" sockets)
> 
> True, but those are not used in openvpn. In case of UDP, userspace just
> creates one socket and uses it for all peers.
> I will add a note about 'connected UDP socket' in the comment, to clear this
> out.

If you want. I was being pedantic, I don't think it's really necessary
to mention this.


> > > +				goto peer_release;
> > > +			}
> > > +		}
> > > +
> > > +		/* 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);
> > > +			goto peer_release;
> > > +		}
> > > +	}
> > 
> > I would reject OVPN_A_PEER_SOCKADDR_REMOTE for a non-UDP socket.
> 
> judging from the comments below, it seems you prefer to reject unneeded
> attributes. OTOH I took the opposite approach (just ignore those).

Yes.

> However, I was actually looking for some preference/indication regarding
> this point and I now I got one :-)

I don't think there's an established rule, and a lot of the old code
is very tolerant. That's my preference (in part because I think
refusing bogus combinations allows to enable them in the future with a
new behavior), but maybe the maintainers have a different opinion?
OTOH ignoring those attributes can let a modern client run on an old
kernel (possibly without some features, depending on what the
attribute is).

(leaving a few other examples of stricter validation for context:)

> I will be strict and return -EINVAL when unneded attributes are present.
> 
> > 
> > 
> > > +	/* VPN IPs cannot be updated, because they are hashed */
> > 
> > Then I think there should be something like
> > 
> >      if (!new_peer && (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
> >          NL_SET_ERR_MSG_FMT_MOD(... "can't update ip");
> >          ret = -EINVAL;
> >          goto peer_release;
> >      }
> > 
> > (just after getting the peer, before any changes have actually been
> > made)
> 
> ACK
> 
> > 
> > And if they are only used in MP mode, I would maybe also reject
> > requests where mode==P2P and OVPN_A_PEER_VPN_IPV* is provided.
> 
> yup, like I commented above.
> 
> > 
> > 
> > > +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
> > > +		peer->vpn_addrs.ipv4.s_addr =
> > > +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> > > +
> > > +	/* VPN IPs cannot be updated, because they are hashed */
> > > +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
> > > +		peer->vpn_addrs.ipv6 =
> > > +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> > > +
> > > +	/* when setting the keepalive, both parameters have to be configured */
> > 
> > Then I would also reject a config where only one is set (also before any
> > changes have been made).
> 
> ok


[...]
> > > +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> > > +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> > > +		keepalive_set = true;
> > > +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> > > +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> > > +	}
> > > +
> > > +	if (keepalive_set)
> > > +		ovpn_peer_keepalive_set(peer, interv, timeout);
> > 
> > Why not skip the bool and just do this in the previous block?
> 
> I am pretty sure there was a reason...but it may have faded away after the
> 95-th rebase hehe. Thanks for spotting this!

:)

> 
> > 
> > > +	netdev_dbg(ovpn->dev,
> > > +		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
> > > +		   __func__, (new_peer ? "adding" : "modifying"), ss,
> > > +		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
> > > +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> > > +
> > > +	if (new_peer) {
> > > +		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;
> > > +		}
> > > +	} else {
> > > +		ovpn_peer_put(peer);
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +peer_release:
> > > +	if (new_peer) {
> > > +		/* release right away because peer is not really used in any
> > > +		 * context
> > > +		 */
> > > +		ovpn_peer_release(peer);
> > > +		kfree(peer);
> > 
> > I don't think that's correct, the new peer was created with
> > ovpn_peer_new, so it took a reference on the netdevice
> > (netdev_hold(ovpn->dev, ...)), which isn't released by
> > ovpn_peer_release. Why not just go through ovpn_peer_put?
> 
> Because then we would send the notification to userspace, but it is not
> correct to do so, because the new() is just about to return an error.

Oh, right.

> I presume I should just move netdev_put(peer->ovpn->dev, NULL); to
> ovpn_peer_release(). That will take care of this case too.

Ok.
diff mbox series

Patch

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index ecac4721f0c6..e0d35c4ac2fb 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -8,6 +8,7 @@ 
 
 #include <linux/netdevice.h>
 #include <linux/rtnetlink.h>
+#include <linux/types.h>
 #include <net/genetlink.h>
 
 #include <uapi/linux/ovpn.h>
@@ -17,6 +18,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);
 
@@ -29,7 +34,7 @@  MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME);
  * Return: the netdevice, if found, or an error otherwise
  */
 static struct net_device *
-ovpn_get_dev_from_attrs(struct net *net, struct genl_info *info)
+ovpn_get_dev_from_attrs(struct net *net, const struct genl_info *info)
 {
 	struct net_device *dev;
 	int ifindex;
@@ -141,24 +146,496 @@  int ovpn_nl_del_iface_doit(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
+static u8 *ovpn_nl_attr_local_ip(struct genl_info *info,
+				 struct ovpn_struct *ovpn,
+				 struct nlattr **attrs, int sock_fam)
+{
+	size_t ip_len = nla_len(attrs[OVPN_A_PEER_LOCAL_IP]);
+	u8 *local_ip = nla_data(attrs[OVPN_A_PEER_LOCAL_IP]);
+	bool is_mapped;
+
+	if (ip_len == sizeof(struct in_addr)) {
+		if (sock_fam != AF_INET) {
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "mismatching AF between local IP (v4) and peer");
+			return ERR_PTR(-EINVAL);
+		}
+	} else if (ip_len == sizeof(struct in6_addr)) {
+		is_mapped = ipv6_addr_v4mapped((struct in6_addr *)local_ip);
+
+		if (sock_fam != AF_INET6 && !is_mapped) {
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "mismatching AF between local IP (v6) and peer");
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (is_mapped)
+			/* this is an IPv6-mapped IPv4
+			 * address, therefore extract
+			 * the actual v4 address from
+			 * the last 4 bytes
+			 */
+			local_ip += 12;
+	} else {
+		NL_SET_ERR_MSG_FMT_MOD(info->extack,
+				       "invalid local IP length: %zu", ip_len);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return local_ip;
+}
+
 int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return -EOPNOTSUPP;
+	bool keepalive_set = false, new_peer = false;
+	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
+	struct ovpn_struct *ovpn = info->user_ptr[0];
+	struct sockaddr_storage *ss = NULL;
+	u32 sockfd, id, interv, timeout;
+	struct socket *sock = NULL;
+	struct sockaddr_in mapped;
+	struct sockaddr_in6 *in6;
+	struct ovpn_peer *peer;
+	u8 *local_ip = NULL;
+	size_t sa_len;
+	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;
+
+	id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
+	/* check if the peer exists first, otherwise create a new one */
+	peer = ovpn_peer_get_by_id(ovpn, id);
+	if (!peer) {
+		peer = ovpn_peer_new(ovpn, id);
+		new_peer = true;
+		if (IS_ERR(peer)) {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "cannot create new peer object for peer %u (sockaddr=%pIScp): %ld",
+					       id, ss, PTR_ERR(peer));
+			return PTR_ERR(peer);
+		}
+	}
+
+	if (new_peer && NL_REQ_ATTR_CHECK(info->extack,
+					  info->attrs[OVPN_A_PEER], attrs,
+					  OVPN_A_PEER_SOCKET)) {
+		ret = -EINVAL;
+		goto peer_release;
+	}
+
+	if (new_peer && ovpn->mode == OVPN_MODE_MP &&
+	    !attrs[OVPN_A_PEER_VPN_IPV4] && !attrs[OVPN_A_PEER_VPN_IPV6]) {
+		NL_SET_ERR_MSG_MOD(info->extack,
+				   "a VPN IP is required when adding a peer in MP mode");
+		ret = -EINVAL;
+		goto peer_release;
+	}
+
+	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);
+			ret = -ENOTSOCK;
+			goto peer_release;
+		}
+
+		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;
+			ret = -ENOTSOCK;
+			goto peer_release;
+		}
+	}
+
+	/* 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 (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP &&
+	    attrs[OVPN_A_PEER_SOCKADDR_REMOTE]) {
+		ss = nla_data(attrs[OVPN_A_PEER_SOCKADDR_REMOTE]);
+		sa_len = nla_len(attrs[OVPN_A_PEER_SOCKADDR_REMOTE]);
+		switch (sa_len) {
+		case sizeof(struct sockaddr_in):
+			if (ss->ss_family == AF_INET)
+				/* valid sockaddr */
+				break;
+
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "remote sockaddr_in has invalid family");
+			ret = -EINVAL;
+			goto peer_release;
+		case sizeof(struct sockaddr_in6):
+			if (ss->ss_family == AF_INET6)
+				/* valid sockaddr */
+				break;
+
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "remote sockaddr_in6 has invalid family");
+			ret = -EINVAL;
+			goto peer_release;
+		default:
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "invalid size for sockaddr: %zd",
+					       sa_len);
+			ret = -EINVAL;
+			goto peer_release;
+		}
+
+		/* if this is a v6-mapped-v4, convert the sockaddr
+		 * object from AF_INET6 to AF_INET before continue
+		 * processing
+		 */
+		if (ss->ss_family == AF_INET6) {
+			in6 = (struct sockaddr_in6 *)ss;
+
+			if (ipv6_addr_v4mapped(&in6->sin6_addr)) {
+				mapped.sin_family = AF_INET;
+				mapped.sin_addr.s_addr =
+					in6->sin6_addr.s6_addr32[3];
+				mapped.sin_port = in6->sin6_port;
+				ss = (struct sockaddr_storage *)&mapped;
+			}
+		}
+
+		if (attrs[OVPN_A_PEER_LOCAL_IP]) {
+			local_ip = ovpn_nl_attr_local_ip(info, ovpn,
+							 attrs,
+							 ss->ss_family);
+			if (IS_ERR(local_ip)) {
+				ret = PTR_ERR(local_ip);
+				NL_SET_ERR_MSG_FMT_MOD(info->extack,
+						       "cannot retrieve local IP: %d",
+						       ret);
+				goto peer_release;
+			}
+		}
+
+		/* 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);
+			goto peer_release;
+		}
+	}
+
+	/* VPN IPs cannot be updated, because they are hashed */
+	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
+		peer->vpn_addrs.ipv4.s_addr =
+			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
+
+	/* VPN IPs cannot be updated, because they are hashed */
+	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
+		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]) {
+		keepalive_set = true;
+		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
+		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
+	}
+
+	if (keepalive_set)
+		ovpn_peer_keepalive_set(peer, interv, timeout);
+
+	netdev_dbg(ovpn->dev,
+		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
+		   __func__, (new_peer ? "adding" : "modifying"), ss,
+		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
+		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
+
+	if (new_peer) {
+		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;
+		}
+	} else {
+		ovpn_peer_put(peer);
+	}
+
+	return 0;
+
+peer_release:
+	if (new_peer) {
+		/* release right away because peer is not really used in any
+		 * context
+		 */
+		ovpn_peer_release(peer);
+		kfree(peer);
+	} else {
+		ovpn_peer_put(peer);
+	}
+
+	return ret;
+}
+
+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_SET_PEER);
+	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->sa.in4.sin_family == AF_INET) {
+			if (nla_put(skb, OVPN_A_PEER_SOCKADDR_REMOTE,
+				    sizeof(bind->sa.in4), &bind->sa.in4) ||
+			    nla_put(skb, OVPN_A_PEER_LOCAL_IP,
+				    sizeof(bind->local.ipv4),
+				    &bind->local.ipv4))
+				goto err_unlock;
+		} else if (bind->sa.in4.sin_family == AF_INET6) {
+			if (nla_put(skb, OVPN_A_PEER_SOCKADDR_REMOTE,
+				    sizeof(bind->sa.in6), &bind->sa.in6) ||
+			    nla_put(skb, OVPN_A_PEER_LOCAL_IP,
+				    sizeof(bind->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_get_peer_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)
+		return -ENOMEM;
+
+	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_get_peer_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
-	return -EOPNOTSUPP;
+	const struct genl_info *info = genl_info_dump(cb);
+	struct ovpn_struct *ovpn;
+	struct ovpn_peer *peer;
+	struct net_device *dev;
+	int bkt, last_idx = cb->args[1], dumped = 0;
+
+	dev = ovpn_get_dev_from_attrs(sock_net(cb->skb->sk), info);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	ovpn = netdev_priv(dev);
+
+	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(dev, NULL);
+
+	/* 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_del_peer_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_set_key_doit(struct sk_buff *skb, struct genl_info *info)
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index c07d148c52b4..2105bcc981fa 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -134,9 +134,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)
 {
 	struct ovpn_bind *bind;
 	size_t ip_len;
@@ -251,7 +251,7 @@  static void ovpn_peer_timer_delete_all(struct ovpn_peer *peer)
  * ovpn_peer_release - release peer private members
  * @peer: the peer to release
  */
-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);
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 691cf20bd870..8d24a8fdd03e 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -127,6 +127,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);
 
 /**
@@ -193,5 +194,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_ */