[PATCH iproute2 0/2] ip: ipstats: Fix statistics split across netlink messages

[PATCH iproute2 0/2] ip: ipstats: Fix statistics split across netlink messages

[Alexander Zubkov]

When dumping statistics, the kernel may split the statistics of a single
interface across several netlink messages: if an attribute does not fit
into the current skb, rtnl_fill_statsinfo() keeps the partial message and
the dump is resumed at the same ifindex. "ip stats" formats every message
on its own, so such an interface is reported twice and part of its
statistics is lost. With "group offload subgroup l3_stats", which requests
two attributes that the kernel emits one by one, this is easy to hit on a
box with many netdevices:

    109: vlan859: group offload subgroup l3_stats on used on

    109: vlan859: group offload subgroup l3_stats

The first record holds IFLA_OFFLOAD_XSTATS_HW_S_INFO, the second one
holds IFLA_OFFLOAD_XSTATS_L3_STATS, and the counters are never shown.

Patch 1 reassembles such messages before formatting them. Patch 2 is an
unrelated hardening cleanup and can be dropped.

The merge itself was also exercised out of tree against the two message
shapes the kernel can split, offload xstats cut between HW_S_INFO and
L3_STATS, and bridge per-VLAN xstats cut between two BRIDGE_XSTATS_VLAN
entries, plus a layout that must be refused rather than merged.

I faced the issue on kernel 7.1.5 / iproute2-7.0.0, mlxsw switch, with
~120 netdevices. The split was visible in an strace as two RTM_NEWSTATS
messages with the same ifindex, carrying IFLA_OFFLOAD_XSTATS_HW_S_INFO
and IFLA_OFFLOAD_XSTATS_L3_STATS respectively. The further research,
patches and supporting texts was prepared with the help of an AI
assistant. I reviewed and tested the patches against iproute2-7.1.0, with
the proposed patches the interface is correctly reported once, with its
counters. Although the patches makes sense to me, I have little
experience with netlink handling.


Alexander Zubkov (2):
  ip: ipstats: Merge statistics split across several netlink messages
  ip: ipstats: Do not hide HW statistics when hw_stats_info is missing

 ip/ipstats.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 280 insertions(+), 5 deletions(-)

-- 
2.55.0

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

[Alexander Zubkov]

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
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>
---
 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)
+/* A netlink dump can split the statistics of one interface across several
+ * messages: 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.
+ */
+
+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.
+ */
+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;
+
+	if (nrta != 1 || nother != 1)
+		return IPSTATS_MERGE_REFUSE;
+
+	if (depth == 0) {
+		unsigned int type = rta->rta_type;
+
+		if (type >= ARRAY_SIZE(ipstats_stat_ifla_max) ||
+		    ipstats_stat_ifla_max[type] == 0)
+			return IPSTATS_MERGE_REFUSE;
+	}
+
+	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;
 }
 
-- 
2.55.0

[PATCH 2/2] ip: ipstats: Do not hide HW statistics when hw_stats_info is missing

[Alexander Zubkov]

ipstats_show_hw_stats() returns as soon as the
IFLA_OFFLOAD_XSTATS_HW_S_INFO attribute is absent, silently dropping the
counters that the message does carry. __ipstats_show_hw_stats() already
copes with a NULL info attribute, so let it, and skip the record only
when neither attribute is present.

There is no known way to reach this through "ip stats": the l3_stats
descriptor always requests HW_S_INFO next to the counters, and since the
previous patch the two are reassembled even when the kernel splits them
across messages. This is a hardening measure so that a message carrying
just the counters is displayed rather than silently discarded.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Alexander Zubkov <green@qrator.net>
---
 ip/ipstats.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ip/ipstats.c b/ip/ipstats.c
index c531885f..54b8267d 100644
--- a/ip/ipstats.c
+++ b/ip/ipstats.c
@@ -472,13 +472,16 @@ static int ipstats_show_hw_stats(struct ipstats_stat_show_attrs *attrs,
 	int err = 0;
 
 	at_hwsi = ipstats_stat_show_get_attr(attrs, group, hw_s_info, &err);
-	if (at_hwsi == NULL)
+	if (at_hwsi == NULL && err != 0)
 		return err;
 
 	at_stats = ipstats_stat_show_get_attr(attrs, group, hw_stats, &err);
 	if (at_stats == NULL && err != 0)
 		return err;
 
+	if (at_hwsi == NULL && at_stats == NULL)
+		return 0;
+
 	return __ipstats_show_hw_stats(at_hwsi, at_stats, idx);
 }
 
-- 
2.55.0

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

[Stephen Hemminger]

On Sun, 30 Aug 2026 20:19:48 +0200
Alexander Zubkov <green@qrator.net> wrote:

> 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
> 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>
> ---
>  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)
> +/* A netlink dump can split the statistics of one interface across several
> + * messages: 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.
> + */
> +

The patch makes sense but please stop with the AI text spew in comments and
commit message. This is garbage. If you insist on using AI to write stuff,
then tell it to be terse and succinct. We don't need three paragraphs here.
One sentence will do fine.

Try again

Re: [PATCH iproute2 0/2] ip: ipstats: Fix statistics split across netlink messages

[Stephen Hemminger]

On Sun, 30 Aug 2026 20:19:47 +0200
Alexander Zubkov <green@qrator.net> wrote:

> When dumping statistics, the kernel may split the statistics of a single
> interface across several netlink messages: if an attribute does not fit
> into the current skb, rtnl_fill_statsinfo() keeps the partial message and
> the dump is resumed at the same ifindex. "ip stats" formats every message
> on its own, so such an interface is reported twice and part of its
> statistics is lost. With "group offload subgroup l3_stats", which requests
> two attributes that the kernel emits one by one, this is easy to hit on a
> box with many netdevices:
> 
>     109: vlan859: group offload subgroup l3_stats on used on
> 
>     109: vlan859: group offload subgroup l3_stats
> 
> The first record holds IFLA_OFFLOAD_XSTATS_HW_S_INFO, the second one
> holds IFLA_OFFLOAD_XSTATS_L3_STATS, and the counters are never shown.
> 
> Patch 1 reassembles such messages before formatting them. Patch 2 is an
> unrelated hardening cleanup and can be dropped.
> 
> The merge itself was also exercised out of tree against the two message
> shapes the kernel can split, offload xstats cut between HW_S_INFO and
> L3_STATS, and bridge per-VLAN xstats cut between two BRIDGE_XSTATS_VLAN
> entries, plus a layout that must be refused rather than merged.
> 
> I faced the issue on kernel 7.1.5 / iproute2-7.0.0, mlxsw switch, with
> ~120 netdevices. The split was visible in an strace as two RTM_NEWSTATS
> messages with the same ifindex, carrying IFLA_OFFLOAD_XSTATS_HW_S_INFO
> and IFLA_OFFLOAD_XSTATS_L3_STATS respectively. The further research,
> patches and supporting texts was prepared with the help of an AI
> assistant. I reviewed and tested the patches against iproute2-7.1.0, with
> the proposed patches the interface is correctly reported once, with its
> counters. Although the patches makes sense to me, I have little
> experience with netlink handling.
> 
> 
> Alexander Zubkov (2):
>   ip: ipstats: Merge statistics split across several netlink messages
>   ip: ipstats: Do not hide HW statistics when hw_stats_info is missing
> 
>  ip/ipstats.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 280 insertions(+), 5 deletions(-)
> 


Please write patches for humans not AI.
Tell Claude to be concise and terse and follow patterns of other code
in the same project.

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

[Petr Machata]

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

Re: [PATCH 2/2] ip: ipstats: Do not hide HW statistics when hw_stats_info is missing

[Petr Machata]

Alexander Zubkov <green@qrator.net> writes:

> ipstats_show_hw_stats() returns as soon as the
> IFLA_OFFLOAD_XSTATS_HW_S_INFO attribute is absent, silently dropping the
> counters that the message does carry. __ipstats_show_hw_stats() already
> copes with a NULL info attribute, so let it, and skip the record only
> when neither attribute is present.
>
> There is no known way to reach this through "ip stats": the l3_stats
> descriptor always requests HW_S_INFO next to the counters, and since the
> previous patch the two are reassembled even when the kernel splits them
> across messages. This is a hardening measure so that a message carrying
> just the counters is displayed rather than silently discarded.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Alexander Zubkov <green@qrator.net>
> ---
>  ip/ipstats.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ip/ipstats.c b/ip/ipstats.c
> index c531885f..54b8267d 100644
> --- a/ip/ipstats.c
> +++ b/ip/ipstats.c
> @@ -472,13 +472,16 @@ static int ipstats_show_hw_stats(struct ipstats_stat_show_attrs *attrs,
>  	int err = 0;
>  
>  	at_hwsi = ipstats_stat_show_get_attr(attrs, group, hw_s_info, &err);
> -	if (at_hwsi == NULL)
> +	if (at_hwsi == NULL && err != 0)
>  		return err;

I am pretty sure this is deliberate. The logic is, if we do not even get
hw_s_info, then surely we will not get hw_stats either, because the
kernel is supposed to provide information about the statistics together
with the statistics themselves.

Now this did not take into account that messages might be split. But
with your patch, this is will not be an issue anymore either.

On second though, it is not the task of iproute2 to policy the kernel,
and when life gives you hw_stats but no hw_s_info, you make hw_stats'ade
or whatever. So I actually think the patch is OK.

So,
Reviewed-by: Petr Machata <me@pmachata.org>

>  
>  	at_stats = ipstats_stat_show_get_attr(attrs, group, hw_stats, &err);
>  	if (at_stats == NULL && err != 0)
>  		return err;
>  
> +	if (at_hwsi == NULL && at_stats == NULL)
> +		return 0;
> +
>  	return __ipstats_show_hw_stats(at_hwsi, at_stats, idx);
>  }

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

[Alexander Zubkov]

Hi guys,

Thanks for the feedback. I wasn't sure how much explanation is ok
here, that is why I passed it as is. I agree that AI agents are way
too verbose. I'll try to get/make a more concise version. I'll need to
post v2 patches here, right?

On Tue, Sep 1, 2026 at 5:12 PM Petr Machata <petrm@nvidia.com> wrote:
>
> 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 :)

Probably yes. Because I do not have a lot of interfaces, 119 to be
precise, and first 90 of them do not have l3_stats enabled. And all of
them fit into 2 netlink reads, problem interface is number 109. So I
do not think much of them is required to hit the issue, but some bad
luck so that one of them gets fragmented.

>
> > 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.

I can only tell now, that it first was merging attributes the other
way, than after some of my questions it found out that it would not
work for bridge vlan attributes, because there are some other sort of
merging is needed and produced this construction. But I do not
understood it much. I would need to invest some time to understand
those attributes and things. Probably it would be better anyway, I'll
try to figure it out.

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

[PATCH iproute2 v2 0/2] ip: ipstats: Fix statistics split across netlink messages

[Alexander Zubkov]

The kernel can split the statistics of one interface across several
netlink messages, and "ip stats" formats each of them on its own, so the
interface is shown twice and part of its statistics is lost. Easy to hit
with "group offload subgroup l3_stats" on a box with many netdevices:

    109: vlan859: group offload subgroup l3_stats on used on

    109: vlan859: group offload subgroup l3_stats

Patch 1 merges such messages before formatting them, patch 2 is an
unrelated cleanup.

What the merge rests on, for whoever touches it next:

 - Only the last attribute of one message and the first of the next can
   be two halves of one nest.

 - A leaf is emitted in one piece, so a leaf attribute seen in both
   messages is a layout the kernel does not produce, and the merge is
   refused.

 - An array is only ever split between two of its elements. Were an
   element itself split, the result would not be distinguishable from a
   longer array of shorter elements, and no reader could reassemble it.

 - At the outer level ipstats_stat_ifla_max[] tells which attributes are
   nests. One level in there is no such table, and none is needed: the
   children of the two halves are whole either way, whether they are an
   array, split only between elements, or attributes indexed by type, of
   which there is at most one of each. Appending them is then what the
   kernel would have sent unsplit.

 - Where the merge does not apply, the two messages are formatted
   separately, which is the behaviour this patch set replaces rather
   than a new failure mode.

v2:
 - Comments and commit messages trimmed, the merge reworked around the
   two border attributes that can actually be halves of one nest.

Alexander Zubkov (2):
  ip: ipstats: Merge statistics split across several netlink messages
  ip: ipstats: Do not hide HW statistics when hw_stats_info is missing

 ip/ipstats.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 258 insertions(+), 8 deletions(-)

-- 
2.55.0

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

[Alexander Zubkov]

The kernel can split the statistics of one interface across several
netlink messages: when an attribute does not fit into the current skb,
rtnl_fill_statsinfo() keeps the partial message and the dump is resumed
at the same ifindex. ipstats_dump_one() formats every message on its
own, so the interface is shown twice and the attributes of the second
message are dropped:

    109: vlan859: group offload subgroup l3_stats on used on

    109: vlan859: group offload subgroup l3_stats

Buffer the message instead, merge into it the following messages with
the same ifindex, and format it once another ifindex shows up or the
dump ends.

Whether two attributes are a reopened nest or merely share a type
cannot be told from the message, so it is decided from what ipstats.c
knows: at the outer level ipstats_stat_ifla_max[] says which types are
nests, and below it the children of the two halves are appended.

Fixes: 54d82b0699a0 ("ip: Add a new family of commands, "stats"")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Alexander Zubkov <green@qrator.net>
---
v2:
 - Trimmed the comments and the commit message.
 - Dropped IPSTATS_MERGE_MAX_DEPTH.
 - Only the boundary attributes are merged.
 - All of the merging decision is in ipstats_merge_classify().
 - Do not index ipstats_stat_ifla_max[] with attribute type 0 (padding).
 - Use addraw_l() to copy attributes.

Notes:
 - Reproduced on Linux 7.1.5 / iproute2-7.0.0, Mellanox MSN4600C with 121
   netdevices, 31 with l3_stats; the split is visible in strace sa two
   RTM_NEWSTATS messages with the same ifindex.
 - The merge was also exercised by Claude out of tree against offload
   xstats cut between HW_S_INFO and L3_STATS, bridge per-VLAN xstats cut
   between two BRIDGE_XSTATS_VLAN entries, and a leaf attribute in both
   messages.

 ip/ipstats.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 254 insertions(+), 7 deletions(-)

diff --git a/ip/ipstats.c b/ip/ipstats.c
index 8a2ac85e..f7a5619f 100644
--- a/ip/ipstats.c
+++ b/ip/ipstats.c
@@ -850,12 +850,208 @@ ipstats_show_one(int ifindex, struct ipstats_stat_enabled *enabled)
 	return err;
 }
 
-static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
+/* A netlink dump can split the statistics of one interface across several
+ * messages, which need to be merged back before they are shown.
+ */
+
+static unsigned int ipstats_rta_count(struct rtattr *rtas, int len,
+				      unsigned short type)
+{
+	unsigned int count = 0;
+	struct rtattr *rta;
+
+	for (rta = rtas; RTA_OK(rta, len); rta = RTA_NEXT(rta, len))
+		if (rta->rta_type == type)
+			count++;
+
+	return count;
+}
+
+enum ipstats_merge_action {
+	IPSTATS_MERGE_COPY,
+	IPSTATS_MERGE_NEST,
+	IPSTATS_MERGE_APPEND,
+	IPSTATS_MERGE_REFUSE,
+};
+
+static enum ipstats_merge_action
+ipstats_merge_classify(const struct rtattr *last_a,
+		       const struct rtattr *first_b,
+		       unsigned int na, unsigned int nb, bool outer)
+{
+	if (last_a == NULL || first_b == NULL ||
+	    last_a->rta_type != first_b->rta_type)
+		return IPSTATS_MERGE_COPY;
+
+	/* Array attributes should only be split on attribute boundaries.
+	 * Otherwise indistinguishable by any reader.
+	 */
+	if (na > 1 || nb > 1)
+		return IPSTATS_MERGE_COPY;
+
+	/* Deeper nesting is useless without knowing the structure.
+	 * But no attributes requires it today, and append is safe.
+	 */
+	if (!outer)
+		return IPSTATS_MERGE_APPEND;
+
+	/* An attribute that ipstats_stat_ifla_max does not describe is a leaf.
+	 * But a leaf should not be split.
+	 */
+	if (last_a->rta_type >= ARRAY_SIZE(ipstats_stat_ifla_max) ||
+	    ipstats_stat_ifla_max[last_a->rta_type] == 0)
+		return IPSTATS_MERGE_REFUSE;
+
+	return IPSTATS_MERGE_NEST;
+}
+
+static int ipstats_merge_rtas(struct nlmsghdr *n, int maxlen,
+			      struct rtattr *a, int alen,
+			      struct rtattr *b, int blen, bool outer)
+{
+	struct rtattr *first_b = NULL;
+	struct rtattr *last_a = NULL;
+	struct rtattr *rest_b = NULL;
+	unsigned int na = 0, nb = 0;
+	struct rtattr *nest;
+	struct rtattr *rta;
+	int rest_blen = 0;
+	int rem;
+	int err;
+
+	/* Type 0 is used in ipstats_stat_ifla_max to represent the top level.
+	 * But it is also a valid type used for padding. So safe to ignore.
+	 */
+	for (rta = a, rem = alen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem))
+		if (rta->rta_type != 0)
+			last_a = rta;
+
+	for (rta = b, rem = blen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem)) {
+		if (rta->rta_type == 0)
+			continue;
+
+		first_b = rta;
+		rest_b = RTA_NEXT(rta, rem);
+		rest_blen = rem;
+		break;
+	}
+
+	if (last_a != NULL)
+		na = ipstats_rta_count(a, alen, last_a->rta_type);
+	if (first_b != NULL)
+		nb = ipstats_rta_count(b, blen, first_b->rta_type);
+
+	if (last_a != NULL) {
+		if (addraw_l(n, maxlen, a, (char *)last_a - (char *)a))
+			return -EMSGSIZE;
+	}
+
+	switch (ipstats_merge_classify(last_a, first_b, na, nb, outer)) {
+	case IPSTATS_MERGE_REFUSE:
+		return -EOPNOTSUPP;
+
+	case IPSTATS_MERGE_COPY:
+		if (last_a != NULL) {
+			if (addattr_l(n, maxlen, last_a->rta_type,
+				      RTA_DATA(last_a), RTA_PAYLOAD(last_a)))
+				return -EMSGSIZE;
+		}
+
+		if (first_b != NULL) {
+			if (addattr_l(n, maxlen, first_b->rta_type,
+				      RTA_DATA(first_b), RTA_PAYLOAD(first_b)))
+				return -EMSGSIZE;
+		}
+		break;
+
+	case IPSTATS_MERGE_NEST:
+		nest = addattr_nest(n, maxlen, last_a->rta_type);
+		if (nest == NULL)
+			return -EMSGSIZE;
+
+		err = ipstats_merge_rtas(n, maxlen,
+					 RTA_DATA(last_a), RTA_PAYLOAD(last_a),
+					 RTA_DATA(first_b),
+					 RTA_PAYLOAD(first_b), false);
+		if (err)
+			return err;
+
+		addattr_nest_end(n, nest);
+		break;
+
+	case IPSTATS_MERGE_APPEND:
+		nest = addattr_nest(n, maxlen, last_a->rta_type);
+		if (nest == NULL)
+			return -EMSGSIZE;
+		if (addraw_l(n, maxlen, RTA_DATA(last_a),
+			     RTA_PAYLOAD(last_a)))
+			return -EMSGSIZE;
+		if (addraw_l(n, maxlen, RTA_DATA(first_b),
+			     RTA_PAYLOAD(first_b)))
+			return -EMSGSIZE;
+		addattr_nest_end(n, nest);
+		break;
+	}
+
+	if (rest_b != NULL) {
+		if (addraw_l(n, maxlen, rest_b, rest_blen))
+			return -EMSGSIZE;
+	}
+
+	return 0;
+}
+
+static int ipstats_attrs_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_attrs_len(a),
+				  IFLA_STATS_RTA(NLMSG_DATA(b)),
+				  ipstats_attrs_len(b), true);
+	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;
 	int rc;
 
-	rc = ipstats_process_ifsm(stdout, n, enabled);
+	if (ctx->pending == NULL)
+		return 0;
+
+	rc = ipstats_process_ifsm(stdout, ctx->pending, ctx->enabled);
+	free(ctx->pending);
+	ctx->pending = NULL;
 	if (rc)
 		return rc;
 
@@ -863,9 +1059,59 @@ 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;
+	int rc;
+
+	if (ipstats_attrs_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) {
+			struct nlmsghdr *merged;
+
+			merged = ipstats_msg_merge(ctx->pending, n, &rc);
+			if (merged != NULL) {
+				free(ctx->pending);
+				ctx->pending = merged;
+				return 0;
+			}
+
+			if (rc != -EOPNOTSUPP)
+				return rc;
+
+			fprintf(stderr,
+				"Cannot merge statistics of ifindex %d split across messages\n",
+				ifsm->ifindex);
+		}
+
+		rc = ipstats_dump_pending(ctx);
+		if (rc)
+			return rc;
+	}
+
+	ctx->pending = malloc(n->nlmsg_len);
+	if (ctx->pending == NULL) {
+		fprintf(stderr, "Error saving netlink answer: %s\n",
+			strerror(errno));
+		return -ENOMEM;
+	}
+
+	memcpy(ctx->pending, n, n->nlmsg_len);
+	return 0;
+}
+
 static int ipstats_dump(struct ipstats_stat_enabled *enabled)
 {
-	int rc = 0;
+	struct ipstats_dump_ctx ctx = {
+		.enabled = enabled,
+	};
 
 	if (rtnl_statsdump_req_filter(&rth, PF_UNSPEC, 0,
 				      ipstats_req_add_filters,
@@ -874,12 +1120,13 @@ 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;
+		free(ctx.pending);
+		return -2;
 	}
 
-	return rc;
+	return ipstats_dump_pending(&ctx);
 }
 
 static int
-- 
2.55.0

[PATCH iproute2 v2 2/2] ip: ipstats: Do not hide HW statistics when hw_stats_info is missing

[Alexander Zubkov]

ipstats_show_hw_stats() returns as soon as IFLA_OFFLOAD_XSTATS_HW_S_INFO
is absent, dropping the counters that the message does carry.
__ipstats_show_hw_stats() already copes with a NULL info attribute, so
let it, and skip the record only when neither attribute is present.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Alexander Zubkov <green@qrator.net>
---
v2:
 - Trimmed the commit message.

 ip/ipstats.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ip/ipstats.c b/ip/ipstats.c
index f7a5619f..4edfcebe 100644
--- a/ip/ipstats.c
+++ b/ip/ipstats.c
@@ -472,13 +472,16 @@ static int ipstats_show_hw_stats(struct ipstats_stat_show_attrs *attrs,
 	int err = 0;
 
 	at_hwsi = ipstats_stat_show_get_attr(attrs, group, hw_s_info, &err);
-	if (at_hwsi == NULL)
+	if (at_hwsi == NULL && err != 0)
 		return err;
 
 	at_stats = ipstats_stat_show_get_attr(attrs, group, hw_stats, &err);
 	if (at_stats == NULL && err != 0)
 		return err;
 
+	if (at_hwsi == NULL && at_stats == NULL)
+		return 0;
+
 	return __ipstats_show_hw_stats(at_hwsi, at_stats, idx);
 }
 
-- 
2.55.0

Re: [PATCH iproute2 v2 0/2] ip: ipstats: Fix statistics split across netlink messages

[Stephen Hemminger]

On Tue,  8 Sep 2026 20:12:23 +0200
Alexander Zubkov <green@qrator.net> wrote:

> The kernel can split the statistics of one interface across several
> netlink messages, and "ip stats" formats each of them on its own, so the
> interface is shown twice and part of its statistics is lost. Easy to hit
> with "group offload subgroup l3_stats" on a box with many netdevices:
> 
>     109: vlan859: group offload subgroup l3_stats on used on
> 
>     109: vlan859: group offload subgroup l3_stats
> 
> Patch 1 merges such messages before formatting them, patch 2 is an
> unrelated cleanup.
> 
> What the merge rests on, for whoever touches it next:
> 
>  - Only the last attribute of one message and the first of the next can
>    be two halves of one nest.
> 
>  - A leaf is emitted in one piece, so a leaf attribute seen in both
>    messages is a layout the kernel does not produce, and the merge is
>    refused.
> 
>  - An array is only ever split between two of its elements. Were an
>    element itself split, the result would not be distinguishable from a
>    longer array of shorter elements, and no reader could reassemble it.
> 
>  - At the outer level ipstats_stat_ifla_max[] tells which attributes are
>    nests. One level in there is no such table, and none is needed: the
>    children of the two halves are whole either way, whether they are an
>    array, split only between elements, or attributes indexed by type, of
>    which there is at most one of each. Appending them is then what the
>    kernel would have sent unsplit.
> 
>  - Where the merge does not apply, the two messages are formatted
>    separately, which is the behaviour this patch set replaces rather
>    than a new failure mode.
> 
> v2:
>  - Comments and commit messages trimmed, the merge reworked around the
>    two border attributes that can actually be halves of one nest.
> 
> Alexander Zubkov (2):
>   ip: ipstats: Merge statistics split across several netlink messages
>   ip: ipstats: Do not hide HW statistics when hw_stats_info is missing
> 
>  ip/ipstats.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 258 insertions(+), 8 deletions(-)
> 

This is a real problem (not AI hallucination) but the code generated is
far larger than really needed. I asked Fable to come up with something
more concise.


The fix is right in principle. Per-ifindex buffering is unavoidable
here: each enabled group prints its own header and JSON object, and
l3_stats needs HW_S_INFO and L3_STATS together, so nothing can go out
until the next ifindex shows up. Merging the raw messages is also the
right layer, it leaves the show code alone. But the merge is about
three times bigger than the problem.

What the kernel actually does (rtnl_fill_statsinfo, rtnl_stats_dump,
br_fill_linkxstats): a message is kept partial only when prividx
advanced. The resume reopens the top-level nest in idxattr and, inside
it, at most one more nest (LINK_XSTATS_TYPE_BRIDGE, resumed at a VLAN
index). Everything below that is whole. Nothing at those two levels
repeats within one message; the only repeated type anywhere is
BRIDGE_XSTATS_VLAN, a leaf two levels down. So:

- ipstats_rta_count() and the na/nb test guard a layout that does not
  exist. Drop them.

- IPSTATS_MERGE_REFUSE guards a leaf on both sides, which idxattr
  gating rules out. If it ever happened, copying both and letting
  parse_rtattr() keep the first is no worse than a warning plus
  formatting the halves separately. Drop it, and the -EOPNOTSUPP
  fallback in ipstats_dump_one() with it.

- With those gone the enum and ipstats_merge_classify() collapse into
  one condition and the switch into an if.

Something like this. Checked against synthetic offload, bridge VLAN
and empty-nest splits, same bytes as your version:

/* The kernel resumes a split dump by reopening the top-level nest it
 * stopped in and, for bridge xstats, the nest inside that. Everything
 * below is whole and nothing at those two levels repeats, so join the
 * two halves at the boundary and copy the rest.
 */
static int ipstats_merge_attrs(struct nlmsghdr *n, int maxlen,
			       struct rtattr *a, int alen,
			       struct rtattr *b, int blen, int depth)
{
	struct rtattr *last = NULL, *rta, *nest;
	unsigned short type;
	int rem;

	/* type 0 is 64-bit padding, possibly on either side of the split */
	for (rta = a, rem = alen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem))
		if (rta->rta_type)
			last = rta;
	while (RTA_OK(b, blen) && !b->rta_type)
		b = RTA_NEXT(b, blen);

	if (depth == 2 || !last || !RTA_OK(b, blen))
		goto copy;
	type = last->rta_type & NLA_TYPE_MASK;
	if (type != (b->rta_type & NLA_TYPE_MASK) ||
	    (!depth && (type > IFLA_STATS_MAX || !ipstats_stat_ifla_max[type])))
		goto copy;

	if (addraw_l(n, maxlen, a, (char *)last - (char *)a))
		return -EMSGSIZE;
	nest = addattr_nest(n, maxlen, last->rta_type);
	if (ipstats_merge_attrs(n, maxlen, RTA_DATA(last), RTA_PAYLOAD(last),
				RTA_DATA(b), RTA_PAYLOAD(b), depth + 1))
		return -EMSGSIZE;
	addattr_nest_end(n, nest);
	b = RTA_NEXT(b, blen);
	return addraw_l(n, maxlen, b, blen) ? -EMSGSIZE : 0;

copy:
	if (addraw_l(n, maxlen, a, alen) || addraw_l(n, maxlen, b, blen))
		return -EMSGSIZE;
	return 0;
}

The dump side is then: flush pending if the ifindex changed, then
either copy the message or merge it into pending. No need for the
err out-parameter or a separate ipstats_msg_merge().

Nits:

- addattr_nest() never returns NULL, those checks are dead.

- Mask rta_type with NLA_TYPE_MASK before comparing or indexing.
  HW_S_INFO already carries NLA_F_NESTED; your >= ARRAY_SIZE test
  would refuse the merge if a top-level nest ever grew the flag.

- calloc -> malloc, addraw_l() zeroes the padding.

- The cover letter bullets are the comment that belongs above the
  merge function, in three lines.

Patch 2 is fine on its own.

Unrelated, for Petr/Ido: in rtnl_fill_statsinfo() when
nla_nest_start_noflag() itself fails, the message is cancelled
(prividx did not move) but idxattr stays set, so the retry for that
netdev skips IFLA_STATS_LINK_64.

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

[Alexander Zubkov]

Hello again,

I've finally completed the rework, and just sent the updated patches.
The cover letter is a bit verbose, but there are several assumptions
the patch is built on, so I believe they are worth mentioning. We've
remade the algorithm of the patch and I tried my best to make it clear
and concise.

Another question I had during my investigation of the problem. The
current merger approach is not aware of the structure of the
attributes. But the parsing functions in the printing part know what
they expect. And it is technically possible not to merge the messages
in the first place, but keep them together and send them further
togeter, where we can try to do the merge on the fly, having the
information from the parser. And we even do some prototype for that,
that doesn't look too intrusive. But on the other hand it seems that
the kernel doesn't do much splitting on the deeper levels and the
current merge approach might work well enough. And actually the kernel
should not do it, otherwise there can be some indistinguishable cases.
So my question is - is it worth further research and should I prepare
the patches for that, or it is not justified enough to do that?

Regards,
Alexander Zubkov

On Wed, Sep 2, 2026 at 9:20 PM Alexander Zubkov <green@qrator.net> wrote:
>
> Hi guys,
>
> Thanks for the feedback. I wasn't sure how much explanation is ok
> here, that is why I passed it as is. I agree that AI agents are way
> too verbose. I'll try to get/make a more concise version. I'll need to
> post v2 patches here, right?
>
> On Tue, Sep 1, 2026 at 5:12 PM Petr Machata <petrm@nvidia.com> wrote:
> >
> > 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 :)
>
> Probably yes. Because I do not have a lot of interfaces, 119 to be
> precise, and first 90 of them do not have l3_stats enabled. And all of
> them fit into 2 netlink reads, problem interface is number 109. So I
> do not think much of them is required to hit the issue, but some bad
> luck so that one of them gets fragmented.
>
> >
> > > 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.
>
> I can only tell now, that it first was merging attributes the other
> way, than after some of my questions it found out that it would not
> work for bridge vlan attributes, because there are some other sort of
> merging is needed and produced this construction. But I do not
> understood it much. I would need to invest some time to understand
> those attributes and things. Probably it would be better anyway, I'll
> try to figure it out.
>
> >
> > > +             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;
> > >  }

Re: [PATCH iproute2 v2 0/2] ip: ipstats: Fix statistics split across netlink messages

[Alexander Zubkov]

On Tue, Sep 8, 2026 at 8:31 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue,  8 Sep 2026 20:12:23 +0200
> Alexander Zubkov <green@qrator.net> wrote:
>
> > The kernel can split the statistics of one interface across several
> > netlink messages, and "ip stats" formats each of them on its own, so the
> > interface is shown twice and part of its statistics is lost. Easy to hit
> > with "group offload subgroup l3_stats" on a box with many netdevices:
> >
> >     109: vlan859: group offload subgroup l3_stats on used on
> >
> >     109: vlan859: group offload subgroup l3_stats
> >
> > Patch 1 merges such messages before formatting them, patch 2 is an
> > unrelated cleanup.
> >
> > What the merge rests on, for whoever touches it next:
> >
> >  - Only the last attribute of one message and the first of the next can
> >    be two halves of one nest.
> >
> >  - A leaf is emitted in one piece, so a leaf attribute seen in both
> >    messages is a layout the kernel does not produce, and the merge is
> >    refused.
> >
> >  - An array is only ever split between two of its elements. Were an
> >    element itself split, the result would not be distinguishable from a
> >    longer array of shorter elements, and no reader could reassemble it.
> >
> >  - At the outer level ipstats_stat_ifla_max[] tells which attributes are
> >    nests. One level in there is no such table, and none is needed: the
> >    children of the two halves are whole either way, whether they are an
> >    array, split only between elements, or attributes indexed by type, of
> >    which there is at most one of each. Appending them is then what the
> >    kernel would have sent unsplit.
> >
> >  - Where the merge does not apply, the two messages are formatted
> >    separately, which is the behaviour this patch set replaces rather
> >    than a new failure mode.
> >
> > v2:
> >  - Comments and commit messages trimmed, the merge reworked around the
> >    two border attributes that can actually be halves of one nest.
> >
> > Alexander Zubkov (2):
> >   ip: ipstats: Merge statistics split across several netlink messages
> >   ip: ipstats: Do not hide HW statistics when hw_stats_info is missing
> >
> >  ip/ipstats.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 258 insertions(+), 8 deletions(-)
> >
>
> This is a real problem (not AI hallucination) but the code generated is
> far larger than really needed. I asked Fable to come up with something
> more concise.
>

For sure, I faced it on our device and observed the split in strace
before turning to AI.

>
> The fix is right in principle. Per-ifindex buffering is unavoidable
> here: each enabled group prints its own header and JSON object, and
> l3_stats needs HW_S_INFO and L3_STATS together, so nothing can go out
> until the next ifindex shows up. Merging the raw messages is also the
> right layer, it leaves the show code alone. But the merge is about
> three times bigger than the problem.
>
> What the kernel actually does (rtnl_fill_statsinfo, rtnl_stats_dump,
> br_fill_linkxstats): a message is kept partial only when prividx
> advanced. The resume reopens the top-level nest in idxattr and, inside
> it, at most one more nest (LINK_XSTATS_TYPE_BRIDGE, resumed at a VLAN
> index). Everything below that is whole. Nothing at those two levels
> repeats within one message; the only repeated type anywhere is
> BRIDGE_XSTATS_VLAN, a leaf two levels down. So:

Thanks for the insights on the kernel internals, I lack this
knowledge, so yes, the original work was too generic.

>
> - ipstats_rta_count() and the na/nb test guard a layout that does not
>   exist. Drop them.
>
> - IPSTATS_MERGE_REFUSE guards a leaf on both sides, which idxattr
>   gating rules out. If it ever happened, copying both and letting
>   parse_rtattr() keep the first is no worse than a warning plus
>   formatting the halves separately. Drop it, and the -EOPNOTSUPP
>   fallback in ipstats_dump_one() with it.
>
> - With those gone the enum and ipstats_merge_classify() collapse into
>   one condition and the switch into an if.
>
> Something like this. Checked against synthetic offload, bridge VLAN
> and empty-nest splits, same bytes as your version:
>
> /* The kernel resumes a split dump by reopening the top-level nest it
>  * stopped in and, for bridge xstats, the nest inside that. Everything
>  * below is whole and nothing at those two levels repeats, so join the
>  * two halves at the boundary and copy the rest.
>  */
> static int ipstats_merge_attrs(struct nlmsghdr *n, int maxlen,
>                                struct rtattr *a, int alen,
>                                struct rtattr *b, int blen, int depth)
> {
>         struct rtattr *last = NULL, *rta, *nest;
>         unsigned short type;
>         int rem;
>
>         /* type 0 is 64-bit padding, possibly on either side of the split */
>         for (rta = a, rem = alen; RTA_OK(rta, rem); rta = RTA_NEXT(rta, rem))
>                 if (rta->rta_type)
>                         last = rta;
>         while (RTA_OK(b, blen) && !b->rta_type)
>                 b = RTA_NEXT(b, blen);
>
>         if (depth == 2 || !last || !RTA_OK(b, blen))
>                 goto copy;
>         type = last->rta_type & NLA_TYPE_MASK;
>         if (type != (b->rta_type & NLA_TYPE_MASK) ||
>             (!depth && (type > IFLA_STATS_MAX || !ipstats_stat_ifla_max[type])))
>                 goto copy;
>
>         if (addraw_l(n, maxlen, a, (char *)last - (char *)a))
>                 return -EMSGSIZE;
>         nest = addattr_nest(n, maxlen, last->rta_type);
>         if (ipstats_merge_attrs(n, maxlen, RTA_DATA(last), RTA_PAYLOAD(last),
>                                 RTA_DATA(b), RTA_PAYLOAD(b), depth + 1))
>                 return -EMSGSIZE;
>         addattr_nest_end(n, nest);
>         b = RTA_NEXT(b, blen);
>         return addraw_l(n, maxlen, b, blen) ? -EMSGSIZE : 0;
>
> copy:
>         if (addraw_l(n, maxlen, a, alen) || addraw_l(n, maxlen, b, blen))
>                 return -EMSGSIZE;
>         return 0;
> }
>
> The dump side is then: flush pending if the ifindex changed, then
> either copy the message or merge it into pending. No need for the
> err out-parameter or a separate ipstats_msg_merge().
>
> Nits:
>
> - addattr_nest() never returns NULL, those checks are dead.
>
> - Mask rta_type with NLA_TYPE_MASK before comparing or indexing.
>   HW_S_INFO already carries NLA_F_NESTED; your >= ARRAY_SIZE test
>   would refuse the merge if a top-level nest ever grew the flag.

This is actually related to an additional question I wanted to raise
later. The type is used to compare types, which should not be
different, and also to index ipstats_stat_ifla_max[], and the
top-level type is unflagged AFAIK. But I agree, that it better to mask
the type right now.

And my related question is the following. While preparing the patch we
noticed that parse_rtattr() pass 0 flag to parse_rtattr_flags(), so
the type is not masked there. And it seems to me that it is better to
apply the mask there too, if there is no strong reasons not to do it.
Because it would allow the kernel to add the nested flag the top level
attribute eventually, while now it cannot do that. Claude refered to
commit 0da4cfaa where Petr added masking to rta_parse_nested() only
and suggested that he might know the reason why the type is not masked
in other places.

>
> - calloc -> malloc, addraw_l() zeroes the padding.
>
> - The cover letter bullets are the comment that belongs above the
>   merge function, in three lines.
>
> Patch 2 is fine on its own.
>
> Unrelated, for Petr/Ido: in rtnl_fill_statsinfo() when
> nla_nest_start_noflag() itself fails, the message is cancelled
> (prividx did not move) but idxattr stays set, so the retry for that
> netdev skips IFLA_STATS_LINK_64.