Re: [PATCH 1/2] ip: ipstats: Merge statistics split across several netlink messages

[Petr Machata <petrm@nvidia.com>]

Alexander Zubkov <green@qrator.net> writes:

> A netlink dump can split the statistics of a single interface across
> several messages. When an attribute does not fit into the current skb,
> rtnl_fill_statsinfo() keeps the partial message, records how far it got
> in idxattr/prividx, and the dump is resumed at the same ifindex, with
> the remaining attributes emitted in the next message.
>
> "ip stats show group offload subgroup l3_stats" requests both

The reproducer I had to use was

    ip stats show group offload | grep 'subgroup l3_stats' | less

No amount of interfaces (tried up to 4K VLAN netdevices) triggered the
issue when I requested 'group offload subgroup l3_stats'.

I guess I should have enabled some of those L3 counters :)

> IFLA_OFFLOAD_XSTATS_HW_S_INFO and IFLA_OFFLOAD_XSTATS_L3_STATS, and
> rtnl_offload_xstats_fill() emits them one by one, so a dump over a
> sufficient number of netdevices regularly ends up cut in between the
> two. Each message is currently formatted on its own, which yields two
> bogus records instead of one, and, because ipstats_show_hw_stats() bails
> out when the HW_S_INFO attribute is missing, the counters carried by the
> second message are dropped:
>
>     109: vlan859: group offload subgroup l3_stats on used on
>
>     109: vlan859: group offload subgroup l3_stats
>
> Postpone formatting until the whole object has been received: keep the
> message around, merge into it any immediately following message that
> carries the same ifindex, and format the result once a message for
> another ifindex arrives or the dump ends. At most one interface worth of
> statistics is buffered at a time.
>
> Merging the two messages means merging the nests that were open when the
> first one was cut short and that got reopened in the second one. Netlink
> does not mark nests reliably here -- nla_nest_start_noflag() is used --
> so they are recognized by the message layout instead: a reopened nest
> appears exactly once in either message, and at the outer level is known
> to be a nest by ipstats_stat_ifla_max[]. That knowledge only reaches as
> deep as ipstats.c parses the message, so merging stops at the group
> nests and their contents are appended in the order they arrived, which
> is correct both for a deeper nest reopened between two of its children
> and for an array of same-type attributes, as bridge per-VLAN xstats are.
>
> A layout that does not fit the above cannot be told apart from an array
> of same-type attributes, and reassembling it would corrupt the result.
> Such a message pair is refused with -EOPNOTSUPP and the two halves are
> formatted separately, as before this patch. Failures to allocate or to
> build the merged message are not papered over that way and terminate the
> dump.
>
> Fixes: 54d82b0699a0 ("ip: Add a new family of commands, "stats"")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Alexander Zubkov <green@qrator.net>

The patch looks broadly OK to me, including the commit message, which I
see Stephen isn't too fond of. I agree the comments are way too verbose.
Some more commentary below.

> ---
>  ip/ipstats.c | 280 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 276 insertions(+), 4 deletions(-)
>
> diff --git a/ip/ipstats.c b/ip/ipstats.c
> index 8a2ac85e..c531885f 100644
> --- a/ip/ipstats.c
> +++ b/ip/ipstats.c
> @@ -850,12 +850,225 @@ ipstats_show_one(int ifindex, struct ipstats_stat_enabled *enabled)
>  	return err;
>  }
>  
> -static int ipstats_dump_one(struct nlmsghdr *n, void *arg)

So the following comment is super long and nobody will read it. You need
to look into the code to figure out what's what anyway, and the comment
does not really help with that. So I'd keep this:

> +/* A netlink dump can split the statistics of one interface across several
> + * messages:

and drop all this:

>               when an attribute does not fit into the current skb, the kernel
> + * ends the message and resumes the dump at the same ifindex, emitting the
> + * remaining attributes in the next message. See rtnl_fill_statsinfo() and
> + * rtnl_offload_xstats_fill() in the kernel.
> + *
> + * Reassembling the two messages means merging the nests that were open when
> + * the first message was cut short and got reopened in the second one. Netlink
> + * does not mark nests reliably -- nla_nest_start_noflag() is used here -- so
> + * they are recognized by the message layout instead: a nest that was reopened
> + * appears exactly once in either message, and, at the outer level, is known to
> + * be a nest by ipstats_stat_ifla_max[].
> + *
> + * That knowledge only reaches as deep as ipstats.c parses the message, so
> + * merging stops at the group nests, and their contents are appended in the
> + * order they arrived. Appending is the right thing to do whether a deeper nest
> + * was reopened between two of its children or the children are just an array
> + * of same-type attributes, as bridge per-VLAN xstats are. It does the wrong
> + * thing only if a kernel splits a nest that ipstats.c does not know how to
> + * show either, in which case the merge is not the first thing that needs
> + * updating.
> + */

Writing is kinda Claude's thing, so it does it a lot, it comments
everything like crazy, inlines whole functions instead of reusing, etc.
I find myself having to moderate its code a lot because of this.

> +
> +enum {
> +	/* Merge the outer level and the group nests, append below that. */
> +	IPSTATS_MERGE_MAX_DEPTH = 1,
> +};
> +
> +static struct rtattr *ipstats_rta_find(struct rtattr *rtas, int len,
> +				       unsigned short type, unsigned int *count)
> +{
> +	struct rtattr *found = NULL;
> +	struct rtattr *rta;
> +
> +	*count = 0;
> +	for (rta = rtas; RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> +		if (rta->rta_type != type)
> +			continue;
> +		if (found == NULL)
> +			found = rta;
> +		(*count)++;
> +	}
> +
> +	return found;
> +}
> +
> +/* Whether the two messages can be reassembled at this attribute, and how.
> + * MERGE means RTA and OTHER are the two halves of one nest that the first
> + * message left open. APPEND means the attribute belongs to one message only
> + * and is copied over as it is. REFUSE means the type is present in both
> + * messages in a layout that cannot be told apart from an array of same-type
> + * attributes, so reassembling it would corrupt the result.
> + */

Drop the comment. It's clear from the code and the names what it does.

> +enum ipstats_merge_action {
> +	IPSTATS_MERGE_APPEND,
> +	IPSTATS_MERGE_MERGE,
> +	IPSTATS_MERGE_REFUSE,
> +};
> +
> +static enum ipstats_merge_action
> +ipstats_merge_action(const struct rtattr *rta, unsigned int nrta,
> +		     const struct rtattr *other, unsigned int nother, int depth)
> +{
> +	if (rta == NULL || other == NULL)
> +		return IPSTATS_MERGE_APPEND;

This is one useful tidbit from the wall of text above, and it should be
here, not there:

	/* If we have >1 hit for an attribute, it's an array of
         * same-type attributes. Bail out. */
> +	if (nrta != 1 || nother != 1)
> +		return IPSTATS_MERGE_REFUSE;
> +
> +	if (depth == 0) {

Hmm, I suspect this depth == 0 and the above IPSTATS_MERGE_MAX_DEPTH == 1
are correlated? We can't validate attribute type in ipstats_merge_action
at depth > 0, so it's unnecessary to recurse into ipstats_merge_rtas()
beyond and and we can optimize this case by just copying all the
attributes right away through ipstats_copy_rtas.

If this just returned an IPSTATS_MERGE_APPEND in an else branch, then we
wouldn't need ipstats_copy_rtas(), IPSTATS_MERGE_MAX_DEPTH, and the
related special casing.

> +		unsigned int type = rta->rta_type;
> +
> +		if (type >= ARRAY_SIZE(ipstats_stat_ifla_max) ||
> +		    ipstats_stat_ifla_max[type] == 0)
> +			return IPSTATS_MERGE_REFUSE;
> +	}
(	  else {
		return IPSTATS_MERGE_APPEND;
	})

Maybe the branch needs to be elsewhere for actual array calls. I took a
code modified like this for a spin, and got the same output, but I
wasn't trying to cover all counter types, so I might be missing
something.

Plus the optimized copy saves the find calls, which is not nothing.

But in any case I don't like how we end up with this
IPSTATS_MERGE_MAX_DEPTH pretending to be all configurable when in fact
the only value that makes sense is 1.

> +
> +	return IPSTATS_MERGE_MERGE;
> +}
> +
> +static int ipstats_copy_rtas(struct nlmsghdr *n, int maxlen,
> +			     struct rtattr *rtas, int len)
> +{
> +	struct rtattr *rta;
> +
> +	for (rta = rtas; RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> +		if (addattr_l(n, maxlen, rta->rta_type,
> +			      RTA_DATA(rta), RTA_PAYLOAD(rta)))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ipstats_merge_rtas(struct nlmsghdr *n, int maxlen,
> +			      struct rtattr *a, int alen,
> +			      struct rtattr *b, int blen, int depth)
> +{
> +	struct rtattr *rta;
> +	int rem;
> +	int err;
> +
> +	for (rta = a, rem = alen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem)) {
> +		unsigned int nrta, nother;
> +		struct rtattr *other;
> +		struct rtattr *nest;
> +
> +		other = ipstats_rta_find(b, blen, rta->rta_type, &nother);
> +		ipstats_rta_find(a, alen, rta->rta_type, &nrta);
> +
> +		switch (ipstats_merge_action(rta, nrta, other, nother, depth)) {
> +		case IPSTATS_MERGE_REFUSE:
> +			return -EOPNOTSUPP;
> +		case IPSTATS_MERGE_APPEND:
> +			if (addattr_l(n, maxlen, rta->rta_type,
> +				      RTA_DATA(rta), RTA_PAYLOAD(rta)))
> +				return -EMSGSIZE;
> +			continue;
> +		case IPSTATS_MERGE_MERGE:
> +			break;
> +		}
> +
> +		nest = addattr_nest(n, maxlen, rta->rta_type);
> +		if (nest == NULL)
> +			return -EMSGSIZE;
> +
> +		if (depth < IPSTATS_MERGE_MAX_DEPTH) {
> +			err = ipstats_merge_rtas(n, maxlen,
> +						 RTA_DATA(rta), RTA_PAYLOAD(rta),
> +						 RTA_DATA(other),
> +						 RTA_PAYLOAD(other), depth + 1);
> +		} else {
> +			err = ipstats_copy_rtas(n, maxlen, RTA_DATA(rta),
> +						RTA_PAYLOAD(rta));
> +			if (!err)
> +				err = ipstats_copy_rtas(n, maxlen,
> +							RTA_DATA(other),
> +							RTA_PAYLOAD(other));
> +		}
> +		if (err)
> +			return err;
> +
> +		addattr_nest_end(n, nest);
> +	}
> +
> +	for (rta = b, rem = blen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem)) {
> +		unsigned int nrta, nother;
> +		struct rtattr *other;
> +
> +		other = ipstats_rta_find(a, alen, rta->rta_type, &nother);
> +		ipstats_rta_find(b, blen, rta->rta_type, &nrta);
> +
> +		/* Whatever is present in both messages has been dealt with
> +		 * in the loop above.
> +		 */
> +		if (ipstats_merge_action(rta, nrta, other, nother,
> +					 depth) != IPSTATS_MERGE_APPEND)
> +			continue;
> +
> +		if (addattr_l(n, maxlen, rta->rta_type,
> +			      RTA_DATA(rta), RTA_PAYLOAD(rta)))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ipstats_ifsm_len(const struct nlmsghdr *n)
> +{
> +	return n->nlmsg_len - NLMSG_LENGTH(sizeof(struct if_stats_msg));
> +}
> +
> +static struct nlmsghdr *ipstats_msg_merge(const struct nlmsghdr *a,
> +					  const struct nlmsghdr *b, int *err)
> +{
> +	int hdrlen = NLMSG_LENGTH(sizeof(struct if_stats_msg));
> +	int maxlen = a->nlmsg_len + b->nlmsg_len;
> +	struct nlmsghdr *n;
> +
> +	n = calloc(1, maxlen);
> +	if (n == NULL) {
> +		fprintf(stderr, "Error merging netlink answer: %s\n",
> +			strerror(errno));
> +		*err = -errno;
> +		return NULL;
> +	}
> +
> +	memcpy(n, a, hdrlen);
> +	n->nlmsg_len = hdrlen;
> +
> +	*err = ipstats_merge_rtas(n, maxlen,
> +				  IFLA_STATS_RTA(NLMSG_DATA(a)),
> +				  ipstats_ifsm_len(a),
> +				  IFLA_STATS_RTA(NLMSG_DATA(b)),
> +				  ipstats_ifsm_len(b), 0);
> +	if (*err) {
> +		free(n);
> +		return NULL;
> +	}
> +
> +	return n;
> +}
> +
> +struct ipstats_dump_ctx {
> +	struct ipstats_stat_enabled *enabled;
> +	struct nlmsghdr *pending;
> +};
> +
> +static int ipstats_dump_pending(struct ipstats_dump_ctx *ctx)
>  {
> -	struct ipstats_stat_enabled *enabled = arg;
> +	struct nlmsghdr *n = ctx->pending;
>  	int rc;
>  
> -	rc = ipstats_process_ifsm(stdout, n, enabled);
> +	if (n == NULL)
> +		return 0;
> +
> +	ctx->pending = NULL;
> +	rc = ipstats_process_ifsm(stdout, n, ctx->enabled);
> +	free(n);
>  	if (rc)
>  		return rc;
>  
> @@ -863,8 +1076,63 @@ static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
>  	return 0;
>  }
>  
> +static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
> +{
> +	struct ipstats_dump_ctx *ctx = arg;
> +	struct nlmsghdr *merged;
> +	struct nlmsghdr *copy;
> +	int rc;
> +
> +	if (ipstats_ifsm_len(n) < 0) {
> +		fprintf(stderr, "BUG: wrong nlmsg len %d\n", n->nlmsg_len);
> +		return -EINVAL;
> +	}
> +
> +	if (ctx->pending != NULL) {
> +		const struct if_stats_msg *pending = NLMSG_DATA(ctx->pending);
> +		const struct if_stats_msg *ifsm = NLMSG_DATA(n);
> +
> +		if (pending->ifindex == ifsm->ifindex) {
> +			merged = ipstats_msg_merge(ctx->pending, n, &rc);
> +			if (merged != NULL) {
> +				free(ctx->pending);
> +				ctx->pending = merged;
> +				return 0;
> +			}
> +			if (rc != -EOPNOTSUPP)
> +				return rc;
> +
> +			/* An attribute layout that cannot be reassembled:
> +			 * show the two halves separately, like older
> +			 * versions did, rather than dropping either.
> +			 */
> +			fprintf(stderr,
> +				"Cannot merge statistics of ifindex %d split across messages\n",
> +				ifsm->ifindex);
> +		}
> +
> +		rc = ipstats_dump_pending(ctx);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	copy = malloc(n->nlmsg_len);
> +	if (copy == NULL) {
> +		fprintf(stderr, "Error saving netlink answer: %s\n",
> +			strerror(errno));
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(copy, n, n->nlmsg_len);
> +	ctx->pending = copy;
> +	return 0;
> +}
> +
>  static int ipstats_dump(struct ipstats_stat_enabled *enabled)
>  {
> +	struct ipstats_dump_ctx ctx = {
> +		.enabled = enabled,
> +	};
>  	int rc = 0;
>  
>  	if (rtnl_statsdump_req_filter(&rth, PF_UNSPEC, 0,
> @@ -874,11 +1142,15 @@ static int ipstats_dump(struct ipstats_stat_enabled *enabled)
>  		return -2;
>  	}
>  
> -	if (rtnl_dump_filter(&rth, ipstats_dump_one, enabled) < 0) {
> +	if (rtnl_dump_filter(&rth, ipstats_dump_one, &ctx) < 0) {
>  		fprintf(stderr, "Dump terminated\n");
>  		rc = -2;
>  	}
>  
> +	if (rc == 0)
> +		rc = ipstats_dump_pending(&ctx);
> +
> +	free(ctx.pending);
>  	return rc;
>  }