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

[PATCH iproute2 v3 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.

v3:
 - Merge rewritten to the shape Stephen suggested.
v2: https://lore.kernel.org/netdev/20260908181225.31712-1-green@qrator.net/
 - Comments and commit messages trimmed, the merge reworked around the
   two border attributes that can actually be halves of one nest.
v1: https://lore.kernel.org/netdev/20260830181949.1096-1-green@qrator.net/

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 | 145 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 137 insertions(+), 8 deletions(-)

-- 
2.55.0

[PATCH iproute2 v3 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. 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.

Fixes: 54d82b0699a0 ("ip: Add a new family of commands, "stats"")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Alexander Zubkov <green@qrator.net>
---

v3:
 - Merge rewritten along the lines Stephen suggested: no attribute
   counting, no classifier, no refusal. Only the top-level nest and,
   for bridge xstats, the one inside it can be reopened.
 - The invariants moved from the cover letter into the comment above
   ipstats_merge_attrs().
 - Mask rta_type with NLA_TYPE_MASK, drop the dead addattr_nest() NULL
   checks, malloc instead of calloc, no err out-parameter and no
   separate ipstats_msg_merge().
 - One deviation from suggested version: the depth == 2 test is moved
   above the two scans. At that depth there might be no nested
   attributes.
v2: https://lore.kernel.org/netdev/20260908181225.31712-1-green@qrator.net/
 - 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.
v1: https://lore.kernel.org/netdev/20260830181949.1096-1-green@qrator.net/

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 | 140 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 133 insertions(+), 7 deletions(-)

diff --git a/ip/ipstats.c b/ip/ipstats.c
index 8a2ac85e..1d44fe47 100644
--- a/ip/ipstats.c
+++ b/ip/ipstats.c
@@ -850,12 +850,68 @@ ipstats_show_one(int ifindex, struct ipstats_stat_enabled *enabled)
 	return err;
 }
 
-static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
+/* 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;
+
+	if (depth == 2)
+		goto copy;
+
+	/* 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 (!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;
+}
+
+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 +919,78 @@ static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
 	return 0;
 }
 
+#define IFSM_PAYLOAD(n) NLMSG_PAYLOAD(n, sizeof(struct if_stats_msg))
+
+static int ipstats_dump_merge(struct ipstats_dump_ctx *ctx, struct nlmsghdr *n)
+{
+	int hdrlen = NLMSG_LENGTH(sizeof(struct if_stats_msg));
+	int maxlen = ctx->pending->nlmsg_len + n->nlmsg_len;
+	struct nlmsghdr *merged;
+	int rc;
+
+	merged = malloc(maxlen);
+	if (merged == NULL) {
+		fprintf(stderr, "Error merging netlink answer: %s\n",
+			strerror(errno));
+		return -errno;
+	}
+
+	memcpy(merged, ctx->pending, hdrlen);
+	merged->nlmsg_len = hdrlen;
+
+	rc = ipstats_merge_attrs(merged, maxlen,
+				 IFLA_STATS_RTA(NLMSG_DATA(ctx->pending)),
+				 IFSM_PAYLOAD(ctx->pending),
+				 IFLA_STATS_RTA(NLMSG_DATA(n)),
+				 IFSM_PAYLOAD(n), 0);
+	if (rc) {
+		free(merged);
+		return rc;
+	}
+
+	free(ctx->pending);
+	ctx->pending = merged;
+	return 0;
+}
+
+static int ipstats_dump_one(struct nlmsghdr *n, void *arg)
+{
+	struct ipstats_dump_ctx *ctx = arg;
+	int rc;
+
+	if (IFSM_PAYLOAD(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)
+			return ipstats_dump_merge(ctx, n);
+
+		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 +999,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 v3 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>
---
v3:
 - No change.
v2: https://lore.kernel.org/netdev/20260908181225.31712-1-green@qrator.net/
 - Trimmed the commit message.
v1: https://lore.kernel.org/netdev/20260830181949.1096-1-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 1d44fe47..59cecfe4 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