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

On Tue, Sep 8, 2026 at 8:31 PM Stephen Hemminger
[off-list ref] wrote:

On Tue,  8 Sep 2026 20:12:23 +0200
Alexander Zubkov [off-list ref] wrote:

quoted

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.