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

[Stephen Hemminger <stephen@networkplumber.org>]

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.