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

[Stephen Hemminger <stephen@networkplumber.org>]

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.