[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

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

[Stephen Hemminger]

On Thu, 10 Sep 2026 11:20:56 +0200
Alexander Zubkov <green@qrator.net> wrote:

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

AI review found somethings here.


On Thu, 10 Sep 2026 11:20:56 +0200, Alexander Zubkov wrote:
> +#define IFSM_PAYLOAD(n) NLMSG_PAYLOAD(n, sizeof(struct if_stats_msg))
> +
>  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;
> +	}

This check never fires. NLMSG_PAYLOAD() expands to

	((nlh)->nlmsg_len - NLMSG_SPACE((len)))

and nlmsg_len is __u32, so the subtraction is done in unsigned arithmetic
and the result is promoted to unsigned long, not int. A truncated message
wraps to a huge positive value instead of going negative, and the compare
against 0 is always false. Compiled and confirmed: with nlmsg_len = 4,
NLMSG_PAYLOAD() yields 0xffffffffffffffe8, and "IFSM_PAYLOAD(n) < 0"
evaluates to false.

To be clear about severity: I do not think this is reachable from a
correct kernel. RTM_NEWSTATS messages are built with a fixed-size struct
if_stats_msg, so a short one should not occur in practice, and I am not
reporting a live crash. But the check was clearly written to catch
something, and as written it cannot.

It is worth fixing rather than deleting, because nothing else on this path
does the job. NLMSG_OK() only validates against sizeof(struct nlmsghdr),
not against the 28-byte if_stats_msg header, so libnetlink hands the
message to the filter without checking the family payload.
ipstats_process_ifsm() does have an equivalent check that works, because
it computes into a signed int first:

	show_attrs.len = (answer->nlmsg_len -
			  NLMSG_LENGTH(sizeof(*show_attrs.ifsm)));
	if (show_attrs.len < 0) {

but that only guards the print path. The new merge path runs before any
printing and passes the payload lengths straight down:

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

ipstats_merge_attrs() takes these as "int alen"/"int blen", so on a
malformed message the wrapped value truncates back to a negative int and
feeds RTA_OK() and addraw_l(). So the guard in ipstats_dump_one() is the
only thing covering that, and it should work.

Simplest fix is to compare the length rather than the payload:

	if (n->nlmsg_len < NLMSG_LENGTH(sizeof(struct if_stats_msg))) {
		fprintf(stderr, "BUG: wrong nlmsg len %d\n", n->nlmsg_len);
		return -EINVAL;
	}

or assign to an int first, matching what ipstats_process_ifsm() already
does.

> +	merged = malloc(maxlen);
> +	if (merged == NULL) {
> +		fprintf(stderr, "Error merging netlink answer: %s\n",
> +			strerror(errno));
> +		return -errno;
> +	}

Minor: fprintf() may clobber errno before it is read back for the return
value, so this can return an unrelated error (or 0). The sibling failure
path in ipstats_dump_one() gets this right by returning -ENOMEM
explicitly; please do the same here.

The rest of the series looks correct to me. In particular the merge itself
holds up: maxlen cannot be overflowed since the merged content is at most
the sum of both messages minus a header per merge level, addattr_nest()
is passed the raw rta_type so NLA_F_NESTED survives, padding attributes
are inert on both sides of the split, and the allocation paths are
balanced -- free(ctx.pending) on dump abort, free(merged) on merge
failure, and the iterative scheme handles three or more messages for one
ifindex.

[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

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

[Stephen Hemminger]

On Thu, 10 Sep 2026 11:20:57 +0200
Alexander Zubkov <green@qrator.net> wrote:

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

Ok to check for bugs, but this is not that much of a problem.

After patch 1 this is unreachable with a sane kernel.
rtnl_offload_xstats_fill() emits HW_S_INFO unconditionally whenever
its filter bit is set, and ipstats packs that bit together with
L3_STATS. The only way to get L3_STATS without HW_S_INFO is the
resumed message with prividx at L3_STATS, which patch 1 now merges.
GET is not NLM_F_MULTI so it cannot split, the monitor notification
carries HW_S_INFO only, and both attributes landed in the same kernel
series, so there is no version skew either.

The commit message still describes the v1 symptom. It is not dropping
any counters once patch 1 is in. If you want to keep it, reword it as
a consistency cleanup: __ipstats_show_hw_stats() already tolerates a
NULL at_hwsi, the wrapper does not. Otherwise drop it. Not going to
add a validator higher up for a message the kernel never sends.