[PATCH net 0/2] net/sched: flag inconsistent qdisc dumps

[PATCH net 0/2] net/sched: flag inconsistent qdisc dumps

[Reshma Sreekumar]

tc_dump_qdisc() walks every netdev in the netns and spans many netlink
batches on any busy host.  RTNL is only held while a single batch is filled,
so the device list can change in between and the reply can miss entries --
but nothing under net/sched sets cb->seq, so NLM_F_DUMP_INTR is never raised
and userspace cannot tell a partial listing from a complete one.
rtnl_dump_ifinfo() already pairs for_each_netdev_dump() with
cb->seq = dev_base_seq and nl_dump_check_consistent(); patch 1 does the same
for the qdisc dump.

Patch 2 adds a selftest.  Netlink dumps are driven by the reader, so it
unregisters a device at a known point between two batches instead of racing
against one, and a second case requires the flag to stay clear on an
undisturbed dump.  It fails on net/main and passes with patch 1.

Build coverage: allmodconfig builds clean with W=1 -- vmlinux and all
modules link, no new warnings, none in net/sched.  allyesconfig compiles
clean with W=1, including net/sched/sch_api.o with no diagnostics in
net/sched, but the final LD of vmlinux.o exceeds the memory of the machine
used here, so that link was not completed.

Reshma Sreekumar (2):
  net/sched: flag inconsistent qdisc dumps
  selftests: net: check that a disturbed qdisc dump is flagged

 net/sched/sch_api.c                           |  16 ++-
 tools/testing/selftests/net/Makefile          |   1 +
 .../testing/selftests/net/qdisc_dump_intr.py  | 123 ++++++++++++++++++
 3 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/qdisc_dump_intr.py


base-commit: 51b093a7ba27476e1f639455f005e8d2e75390e4
-- 
2.43.0

[PATCH net 1/2] net/sched: flag inconsistent qdisc dumps

[Reshma Sreekumar]

tc_dump_qdisc() walks every netdev in the netns and can span many netlink
batches.  RTNL is only held while a single batch is filled, so devices can
be registered or unregistered in between.  Since 748bbef5fc6a ("net/sched:
switch tc_dump_qdisc() to for_each_netdev_dump()") the walk resumes on an
ifindex rather than a list position, but ifindexes are reused: a device
created between two batches can be given an ifindex the dump has already
walked past, and is then missed entirely.  Before that commit the resume
used a position in the device list, so any unregister shifted every later
device and silently dropped a run of qdiscs from the reply.

Unlike rtnl_dump_ifinfo(), which pairs for_each_netdev_dump() with
cb->seq = dev_base_seq and nl_dump_check_consistent(), nothing under
net/sched sets cb->seq at all -- and never has.  NLM_F_DUMP_INTR is
therefore never raised for a qdisc dump, an incomplete reply is
indistinguishable from a complete one, and userspace has no way to tell it
is reconciling against a partial listing.  Callers already handle the flag
by redoing the dump (iproute2 warns, netlink libraries retry), so raising
it is enough to make this self-correcting.

Use the same idiom as the link dump.  Both return paths are covered, as
the -EMSGSIZE path is the normal one for every batch but the last.

Tested by dumping the qdiscs of 400 dummy devices (mq + 16 fq + clsact
each) while unregistering devices ahead of the dump: NLM_F_DUMP_INTR is
raised on 482 of 972 dumps with this patch and on 0 of 1089 without it.

Signed-off-by: Reshma Sreekumar <reshmaisat@gmail.com>
---
 net/sched/sch_api.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 668bcd60d..ab31a75fa 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1949,12 +1949,26 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb)
 		netdev_unlock_ops(dev);
 		s_q_idx = 0;
 	}
-	return skb->len;
+	err = skb->len;
+	goto out;
 
 error_unlock:
 	netdev_unlock_ops(dev);
 	ctx->q_idx = q_idx;
 
+out:
+	/* RTNL is dropped between dump batches, so devices may have been
+	 * registered or unregistered while this dump was in flight.  Because
+	 * ifindexes get reused, a device created in that window can be given an
+	 * ifindex this dump has already walked past and be missed entirely.
+	 * Advertise that, as rtnl_dump_ifinfo() does for RTM_GETLINK, so that
+	 * userspace redoes the dump instead of acting on a listing that silently
+	 * omits entries.
+	 */
+	cb->seq = READ_ONCE(net->dev_base_seq);
+	if (skb->len)
+		nl_dump_check_consistent(cb, nlmsg_hdr(skb));
+
 	return err;
 }
 
-- 
2.43.0

[PATCH net 2/2] selftests: net: check that a disturbed qdisc dump is flagged

[Reshma Sreekumar]

Add a test for NLM_F_DUMP_INTR on RTM_GETQDISC.  A qdisc dump spans several
netlink batches once a few devices carry a full set of qdiscs, and RTNL is
only held while a single batch is filled, so the device list can change in
between and entries can be missed.

Netlink dumps are driven by the reader, so rather than racing against a
dump the test unregisters a device at a known point: it receives the first
batch, unregisters a dummy, then drains the rest.  This is deterministic --
one flagged message, every run.

A second case dumps without disturbing anything and requires that the flag
stays clear, so the test cannot be satisfied by a kernel that raises it
unconditionally.

Four dummy devices with mq + 16 fq + clsact each, read with a 4KiB buffer,
give 12 batches and run in about a second.  The test skips if dummy, mq, fq
or clsact are unavailable.

Signed-off-by: Reshma Sreekumar <reshmaisat@gmail.com>
---
 tools/testing/selftests/net/Makefile          |   1 +
 .../testing/selftests/net/qdisc_dump_intr.py  | 123 ++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100755 tools/testing/selftests/net/qdisc_dump_intr.py

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 708d960ae..769ee26ae 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -72,6 +72,7 @@ TEST_PROGS := \
 	pmtu.sh \
 	protodown.sh \
 	psock_snd.sh \
+	qdisc_dump_intr.py \
 	reuseaddr_ports_exhausted.sh \
 	reuseport_addr_any.sh \
 	route_hint.sh \
diff --git a/tools/testing/selftests/net/qdisc_dump_intr.py b/tools/testing/selftests/net/qdisc_dump_intr.py
new file mode 100755
index 000000000..9623847f4
--- /dev/null
+++ b/tools/testing/selftests/net/qdisc_dump_intr.py
@@ -0,0 +1,123 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+Check that a qdisc dump disturbed by a device unregister reports
+NLM_F_DUMP_INTR.
+
+tc_dump_qdisc() walks every netdev in the netns and spans several netlink
+batches once there are more than a handful of qdiscs.  RTNL is only held
+while a single batch is filled, so the device list can change in between and
+the reply can miss entries.  Netlink dumps are driven by the reader, so this
+test unregisters a device at a known point between two batches rather than
+racing against one.
+"""
+
+import socket
+import struct
+
+from lib.py import ksft_run, ksft_exit, ksft_true, ksft_pr, KsftSkipEx
+from lib.py import CmdExitFailure, NetNS, NetNSEnter, ip, tc
+
+NETLINK_ROUTE = 0
+RTM_NEWQDISC = 36
+RTM_GETQDISC = 38
+NLMSG_DONE = 3
+NLM_F_REQUEST = 0x001
+NLM_F_DUMP = 0x300
+NLM_F_DUMP_INTR = 0x010
+
+NLMSGHDR = "=IHHII"
+TCMSG_LEN = 20          # struct tcmsg
+
+# tc parses handles as hex; 64: keeps the mq root clear of the fq children
+MQ_HANDLE = "64"
+CHANNELS = 16           # fq children per device
+NDEVS = 4               # 4 * (mq + 16 fq + clsact) is several batches worth
+BUFSZ = 4096            # small recv buffer keeps the kernel's batches small
+
+
+def _setup():
+    """Devices carrying enough qdiscs that a dump has to paginate."""
+    try:
+        for i in range(NDEVS):
+            ip(f"link add d{i} numtxqueues {CHANNELS} type dummy")
+            ip(f"link set d{i} up")
+            tc(f"qdisc replace dev d{i} root handle {MQ_HANDLE}: mq")
+            for q in range(1, CHANNELS + 1):
+                tc(f"qdisc replace dev d{i} parent {MQ_HANDLE}:{q:x} "
+                   f"handle {q:x}: fq")
+            tc(f"qdisc replace dev d{i} clsact")
+    except CmdExitFailure as exc:
+        raise KsftSkipEx("dummy, mq, fq or clsact unavailable") from exc
+
+
+def _dump(between_batches=None):
+    """
+    Walk an RTM_GETQDISC dump one batch at a time, optionally running
+    `between_batches` after the first batch.  Returns (batches, qdiscs,
+    messages carrying NLM_F_DUMP_INTR).
+    """
+    sk = socket.socket(socket.AF_NETLINK, socket.SOCK_RAW, NETLINK_ROUTE)
+    try:
+        sk.bind((0, 0))
+        hdr = struct.pack(NLMSGHDR, struct.calcsize(NLMSGHDR) + TCMSG_LEN,
+                          RTM_GETQDISC, NLM_F_REQUEST | NLM_F_DUMP, 1, 0)
+        sk.send(hdr + bytes(TCMSG_LEN))
+
+        batches = qdiscs = intr = 0
+        done = False
+        while not done:
+            buf = sk.recv(BUFSZ)
+            batches += 1
+            off = 0
+            while off + 16 <= len(buf):
+                mlen, mtype, flags = struct.unpack_from("=IHH", buf, off)
+                if mlen < 16:
+                    break
+                if flags & NLM_F_DUMP_INTR:
+                    intr += 1
+                if mtype == NLMSG_DONE:
+                    done = True
+                elif mtype == RTM_NEWQDISC:
+                    qdiscs += 1
+                off += (mlen + 3) & ~3
+            if batches == 1 and not done and between_batches:
+                between_batches()
+        return batches, qdiscs, intr
+    finally:
+        sk.close()
+
+
+def dump_intr_on_unregister() -> None:
+    """Unregistering a device mid-dump must raise NLM_F_DUMP_INTR."""
+    ip("link add victim type dummy")
+
+    batches, qdiscs, intr = _dump(lambda: ip("link del victim"))
+
+    ksft_pr(f"{batches} batches, {qdiscs} qdiscs, {intr} flagged messages")
+    ksft_true(batches > 1, "dump did not paginate, nothing to disturb")
+    ksft_true(intr > 0,
+              "NLM_F_DUMP_INTR not set although a device was unregistered "
+              "while the dump was in flight")
+
+
+def dump_intr_absent_when_stable() -> None:
+    """An undisturbed dump must not raise NLM_F_DUMP_INTR."""
+    batches, qdiscs, intr = _dump()
+
+    ksft_pr(f"{batches} batches, {qdiscs} qdiscs, {intr} flagged messages")
+    ksft_true(batches > 1, "dump did not paginate, test is not meaningful")
+    ksft_true(intr == 0, "NLM_F_DUMP_INTR set on an undisturbed dump")
+
+
+def main() -> None:
+    with NetNS() as ns:
+        with NetNSEnter(str(ns)):
+            _setup()
+            ksft_run(globs=globals(), case_pfx={"dump_"})
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.43.0

Re: [PATCH net 1/2] net/sched: flag inconsistent qdisc dumps

[Eric Dumazet]

On Thu, Jul 30, 2026 at 12:00 PM Reshma Sreekumar <reshmaisat@gmail.com> wrote:
>
> tc_dump_qdisc() walks every netdev in the netns and can span many netlink
> batches.  RTNL is only held while a single batch is filled, so devices can
> be registered or unregistered in between.  Since 748bbef5fc6a ("net/sched:
> switch tc_dump_qdisc() to for_each_netdev_dump()") the walk resumes on an
> ifindex rather than a list position, but ifindexes are reused: a device
> created between two batches can be given an ifindex the dump has already
> walked past, and is then missed entirely.  Before that commit the resume
> used a position in the device list, so any unregister shifted every later
> device and silently dropped a run of qdiscs from the reply.

I dislike this patch; it will force dump operations to restart on busy netns.

Most devices are created with a new ifindex (dev_index_reserve() is
called with ifindex == 0)

Forcing a given ifindex is rather unusual.

I suggest detecting this specific use case instead of slowing down everything.

Re: [PATCH net 1/2] net/sched: flag inconsistent qdisc dumps

[Reshma Sreekumar]

On Thu, Jul 30, 2026 at 1:24 PM Eric Dumazet <edumazet@google.com> wrote:
  > I dislike this patch; it will force dump operations to restart on
busy netns.
  >
  > Most devices are created with a new ifindex (dev_index_reserve() is
  > called with ifindex == 0)
  >
  > Forcing a given ifindex is rather unusual.

  Agreed.

  For context on why I went looking: we hit this on production hosts running
  kernels predating 748bbef5fc6a. An agent that reconciles tc configuration
  against a qdisc dump saw qdiscs that did exist as absent, tried to create them
  again (-EEXIST), and tore down configuration it believed had gone stale.
  Because nothing sets cb->seq there, a truncated reply is indistinguishable
  from a complete one, so there was no way to detect it and retry.

  748bbef5fc6a makes that far less likely, and I have not been able to reproduce
  the missing-qdisc case on 7.2-rc5. One case does remain, which this patch
  would not have fixed either: ctx->q_idx is a position within the per-device
  qdisc_hash walk, so a qdisc added or removed on the device at a batch boundary
  misaligns the skip count.

  Is that worth reporting to userspace, and how would you want it detected?


On Thu, Jul 30, 2026 at 12:25 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jul 30, 2026 at 12:00 PM Reshma Sreekumar <reshmaisat@gmail.com> wrote:
> >
> > tc_dump_qdisc() walks every netdev in the netns and can span many netlink
> > batches.  RTNL is only held while a single batch is filled, so devices can
> > be registered or unregistered in between.  Since 748bbef5fc6a ("net/sched:
> > switch tc_dump_qdisc() to for_each_netdev_dump()") the walk resumes on an
> > ifindex rather than a list position, but ifindexes are reused: a device
> > created between two batches can be given an ifindex the dump has already
> > walked past, and is then missed entirely.  Before that commit the resume
> > used a position in the device list, so any unregister shifted every later
> > device and silently dropped a run of qdiscs from the reply.
>
> I dislike this patch; it will force dump operations to restart on busy netns.
>
> Most devices are created with a new ifindex (dev_index_reserve() is
> called with ifindex == 0)
>
> Forcing a given ifindex is rather unusual.
>
> I suggest detecting this specific use case instead of slowing down everything.