Re: [PATCH net-next 5/5] ovpn: send peer object along with PEER_DEL_NTF

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's premise is factually wrong: it states
  that final peer statistics are obtained "today ... by issuing a…
- [Medium] Information disclosure across a privilege boundary:
  PEER_DEL_NTF is multicast to the ovpn "peers" generic-netlink group,…
- [Low] The selftest does not exercise the behaviour the patch adds.

--

ovpn: send peer object along with PEER_DEL_NTF

This factors the peer serialization out of ovpn_nl_send_peer() into a new
ovpn_nl_fill_peer() helper and calls it from ovpn_nl_peer_del_notify(), so
PEER_DEL_NTF now carries the same peer object that PEER_GET returns. The
ovpn selftest notification fixtures are updated, and the jq filter
normalizes the timing-dependent traffic counters.

A few questions below.

> OpenVPN userspace needs the final statistics of a disconnecting peer.
> Today this is done by issuing a PEER_GET request after receiving
> PEER_DEL_NTF. When several peers disconnect at the same time, those
> extra request/reply transactions overlap with notification processing on
> the same netlink socket and can make userspace hit transient netlink
> errors such as NLE_BUSY or NLE_NOMEM.

[Severity: Medium]
Is the described existing workflow actually possible?  In
ovpn_peer_remove() the peer is unlinked from the lookup structures before
the notification is emitted:

drivers/net/ovpn/peer.c:ovpn_peer_remove() {
	...
		hlist_del_init_rcu(&peer->hash_entry_id);
	...
	peer->delete_reason = reason;
	ovpn_nl_peer_del_notify(peer);
}

A PEER_GET issued in response to the notification then goes through
ovpn_peer_get_by_id(), which walks exactly those structures:

drivers/net/ovpn/netlink.c:ovpn_nl_peer_get_doit() {
	...
	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;
	}
}

That looks like it fails with -ENOENT for every deleted peer, including a
single disconnect on an otherwise idle netlink socket, so socket contention
would not be what makes the sequence fail.  Would it be more accurate to
describe this patch as making the final counters available at all, rather
than as removing redundant request/reply traffic?

> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 4dad852941982..aaca0612d6f4c 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c
> @@ -546,27 +546,15 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
>  	return 0;
>  }
>  
> -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)
> +static int ovpn_nl_fill_peer(struct sk_buff *skb, const struct genl_info *info,
> +			     const struct ovpn_peer *peer)
>  {

[ ... ]

> @@ -1193,7 +1206,8 @@ int ovpn_nl_peer_del_notify(struct ovpn_peer *peer)
>  	if (nla_put_u32(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))
>  		goto err_cancel_msg;
>  
> -	if (nla_put_u32(msg, OVPN_A_PEER_ID, peer->id))
> +	ret = ovpn_nl_fill_peer(msg, NULL, peer);
> +	if (ret < 0)
>  		goto err_cancel_msg;
>  
>  	nla_nest_end(msg, attr);

[Severity: Medium]
Does this widen what unprivileged readers can observe?
ovpn_nl_peer_del_notify() ends with:

	genlmsg_multicast_netns(&ovpn_nl_family, sock_net(sock->sk), msg, 0,
				OVPN_NLGRP_PEERS, GFP_ATOMIC);

and the destination group is declared without any capability flag:

drivers/net/ovpn/netlink-gen.c:
	static const struct genl_multicast_group ovpn_nl_mcgrps[] = {
		[OVPN_NLGRP_PEERS] = { "peers", },
	};

genl_bind() only rejects the subscription when a flag is set:

net/netlink/genetlink.c:genl_bind() {
	...
		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
			ret = -EPERM;
}

So any process in the netns can join "peers".  Before this change the
notification carried ifindex, peer id and delete reason; with
ovpn_nl_fill_peer(msg, NULL, peer) it now also carries
OVPN_A_PEER_VPN_IPV4/IPV6, OVPN_A_PEER_LOCAL_IPV4/IPV6,
OVPN_A_PEER_LOCAL_PORT, the keepalive parameters and all eight VPN/link
byte and packet counters.  Every OVPN_CMD_PEER_* op, including both
PEER_GET do and dump, carries GENL_ADMIN_PERM in ovpn_nl_ops, so the same
object is gated on CAP_NET_ADMIN when fetched directly.

Since the notification layout becomes uAPI once released, would it make
sense to mark the group GENL_MCAST_CAP_NET_ADMIN while the family is still
new, or to emit only the subset userspace needs?

> diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh
> index de96d333ee2b3..611d528c7de88 100644
> --- a/tools/testing/selftests/net/ovpn/common.sh
> +++ b/tools/testing/selftests/net/ovpn/common.sh
> @@ -19,9 +19,42 @@ OVPN_VERBOSE=${OVPN_VERBOSE:-0}
>  
>  export OVPN_ID_OFFSET=$(( 9 * (OVPN_SYMMETRIC_ID == 0) ))
>  
> -OVPN_JQ_FILTER='map(if type == "array" then .[] else . end) |
> -	map(select(.msg.peer | has("remote-ipv6") | not)) |
> -	map(del(.msg.ifindex)) | sort_by(.msg.peer.id)[]'
> +# Peer delete notifications include traffic counters whose values depend on
> +# timing. zero_attr() sets a counter to zero only when that counter is present,
> +# so missing stats still fail the comparison. zero_peer_stats is just the list
> +# of counters to normalize. normalize_peer_del_ntf applies that to peer-del-ntf
> +# messages and drops transport endpoint details, while leaving other
> +# notifications unchanged.
> +OVPN_JQ_FILTER='
> +	def zero_attr(key):
> +		if has(key) then .[key] = 0 else . end;
> +
> +	def zero_peer_stats:
> +		zero_attr("vpn-rx-bytes") |
> +		zero_attr("vpn-rx-packets") |
> +		zero_attr("vpn-tx-bytes") |
> +		zero_attr("vpn-tx-packets") |
> +		zero_attr("link-rx-bytes") |
> +		zero_attr("link-rx-packets") |
> +		zero_attr("link-tx-bytes") |
> +		zero_attr("link-tx-packets");

[Severity: Low]
This isn't a bug, but does the updated selftest still assert anything about
the counter values the patch adds?

zero_peer_stats rewrites all eight counters to 0, and ovpn_compare_ntfs()
applies OVPN_JQ_FILTER to both the fixture and the captured stream, while
the fixtures hard-code 0 for peers that exchanged ping and iperf traffic
earlier in test.sh:

tools/testing/selftests/net/ovpn/json/peer0.json:
	..."vpn-rx-bytes": 0, "vpn-rx-packets": 0, ..., "link-tx-packets": 0...

With the `if has(key)` guard the only property checked is that the
attributes are present, so a kernel emitting constant or bogus values from
ovpn_nl_fill_peer() would still pass.  Could a weaker value-level check be
added instead, for example requiring nonzero counters for the peers that
carried traffic, or comparing against a PEER_GET snapshot taken before the
deletion?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922060852.2266148-2-antonio%40openvpn.net