[Patch net 0/2] net_sched: Fix HFSC qlen/backlog accounting bug and add selftest

[Patch net 0/2] net_sched: Fix HFSC qlen/backlog accounting bug and add selftest

[Cong Wang]

This series addresses a long-standing bug in the HFSC qdisc where queue length
and backlog accounting could become inconsistent if a packet is dropped during
a peek-induced dequeue operation, and adds a corresponding selftest to tc-testing.

---
Cong Wang (2):
  sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()
  selftests/tc-testing: Add an HFSC qlen accounting test

 net/sched/sch_hfsc.c                          |  6 ++---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 27 +++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

-- 
2.34.1

[Patch net 1/2] sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()

[Cong Wang]

When enqueuing the first packet to an HFSC class, hfsc_enqueue() calls the
child qdisc's peek() operation before incrementing sch->q.qlen and
sch->qstats.backlog. If the child qdisc uses qdisc_peek_dequeued(), this may
trigger an immediate dequeue and potential packet drop. In such cases,
qdisc_tree_reduce_backlog() is called, but the HFSC qdisc's qlen and backlog
have not yet been updated, leading to inconsistent queue accounting. This
can leave an empty HFSC class in the active list, causing further
consequences like use-after-free.

This patch fixes the bug by moving the increment of sch->q.qlen and
sch->qstats.backlog before the call to the child qdisc's peek() operation.
This ensures that queue length and backlog are always accurate when packet
drops or dequeues are triggered during the peek.

Fixes: 12d0ad3be9c3 ("net/sched/sch_hfsc.c: handle corner cases where head may change invalidating calculated deadline")
Reported-by: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_hfsc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index cb8c525ea20e..7986145a527c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1569,6 +1569,9 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 		return err;
 	}
 
+	sch->qstats.backlog += len;
+	sch->q.qlen++;
+
 	if (first && !cl->cl_nactive) {
 		if (cl->cl_flags & HFSC_RSC)
 			init_ed(cl, len);
@@ -1584,9 +1587,6 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 
 	}
 
-	sch->qstats.backlog += len;
-	sch->q.qlen++;
-
 	return NET_XMIT_SUCCESS;
 }
 
-- 
2.34.1

Re: [Patch net 1/2] sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()

[Simon Horman]

On Sun, May 18, 2025 at 03:20:37PM -0700, Cong Wang wrote:
> When enqueuing the first packet to an HFSC class, hfsc_enqueue() calls the
> child qdisc's peek() operation before incrementing sch->q.qlen and
> sch->qstats.backlog. If the child qdisc uses qdisc_peek_dequeued(), this may
> trigger an immediate dequeue and potential packet drop. In such cases,
> qdisc_tree_reduce_backlog() is called, but the HFSC qdisc's qlen and backlog
> have not yet been updated, leading to inconsistent queue accounting. This
> can leave an empty HFSC class in the active list, causing further
> consequences like use-after-free.
> 
> This patch fixes the bug by moving the increment of sch->q.qlen and
> sch->qstats.backlog before the call to the child qdisc's peek() operation.
> This ensures that queue length and backlog are always accurate when packet
> drops or dequeues are triggered during the peek.
> 
> Fixes: 12d0ad3be9c3 ("net/sched/sch_hfsc.c: handle corner cases where head may change invalidating calculated deadline")
> Reported-by: Mingi Cho <mincho@theori.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[Patch net 2/2] selftests/tc-testing: Add an HFSC qlen accounting test

[Cong Wang]

This test reproduces a scenario where HFSC queue length and backlog accounting
can become inconsistent when a peek operation triggers a dequeue and possible
drop before the parent qdisc updates its counters. The test sets up a DRR root
qdisc with an HFSC class, netem, and blackhole children, and uses Scapy to
inject a packet. It helps to verify that HFSC correctly tracks qlen and backlog
even when packets are dropped during peek-induced dequeue.

Cc: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
index a951c0d33cd2..ddc97ecd8b39 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -573,5 +573,32 @@
         "teardown": [
             "$TC qdisc del dev $DEV1 handle 1: root"
         ]
+    },
+    {
+        "id": "831d",
+        "name": "Test HFSC qlen accounting with DRR/NETEM/BLACKHOLE chain",
+        "category": ["qdisc", "hfsc", "drr", "netem", "blackhole"],
+        "plugins": { "requires": ["nsPlugin", "scapyPlugin"] },
+        "setup": [
+            "$IP link set dev $DEV1 up || true",
+            "$TC qdisc add dev $DEV1 root handle 1: drr",
+            "$TC filter add dev $DEV1 parent 1: basic classid 1:1",
+            "$TC class add dev $DEV1 parent 1: classid 1:1 drr",
+            "$TC qdisc add dev $DEV1 parent 1:1 handle 2: hfsc def 1",
+            "$TC class add dev $DEV1 parent 2: classid 2:1 hfsc rt m1 8 d 1 m2 0",
+            "$TC qdisc add dev $DEV1 parent 2:1 handle 3: netem",
+            "$TC qdisc add dev $DEV1 parent 3:1 handle 4: blackhole"
+        ],
+        "scapy": {
+            "iface": "$DEV0",
+            "count": 5,
+            "packet": "Ether()/IP(dst='10.10.10.1', src='10.10.10.10')/ICMP()"
+        },
+        "cmdUnderTest": "$TC -s qdisc show dev $DEV1",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DEV1",
+        "matchPattern": "qdisc hfsc",
+        "matchCount": "1",
+        "teardown": ["$TC qdisc del dev $DEV1 root handle 1: drr"]
     }
 ]
-- 
2.34.1

Re: [Patch net 2/2] selftests/tc-testing: Add an HFSC qlen accounting test

[Simon Horman]

On Sun, May 18, 2025 at 03:20:38PM -0700, Cong Wang wrote:
> This test reproduces a scenario where HFSC queue length and backlog accounting
> can become inconsistent when a peek operation triggers a dequeue and possible
> drop before the parent qdisc updates its counters. The test sets up a DRR root
> qdisc with an HFSC class, netem, and blackhole children, and uses Scapy to
> inject a packet. It helps to verify that HFSC correctly tracks qlen and backlog
> even when packets are dropped during peek-induced dequeue.
> 
> Cc: Mingi Cho <mincho@theori.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Patch net 0/2] net_sched: Fix HFSC qlen/backlog accounting bug and add selftest

[Jamal Hadi Salim]

On Sun, May 18, 2025 at 6:21 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> This series addresses a long-standing bug in the HFSC qdisc where queue length
> and backlog accounting could become inconsistent if a packet is dropped during
> a peek-induced dequeue operation, and adds a corresponding selftest to tc-testing.
>

For the series:
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
> ---
> Cong Wang (2):
>   sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()
>   selftests/tc-testing: Add an HFSC qlen accounting test
>
>  net/sched/sch_hfsc.c                          |  6 ++---
>  .../tc-testing/tc-tests/infra/qdiscs.json     | 27 +++++++++++++++++++
>  2 files changed, 30 insertions(+), 3 deletions(-)
>
> --
> 2.34.1
>

Re: [Patch net 0/2] net_sched: Fix HFSC qlen/backlog accounting bug and add selftest

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Sun, 18 May 2025 15:20:36 -0700 you wrote:
> This series addresses a long-standing bug in the HFSC qdisc where queue length
> and backlog accounting could become inconsistent if a packet is dropped during
> a peek-induced dequeue operation, and adds a corresponding selftest to tc-testing.
> 
> ---
> Cong Wang (2):
>   sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()
>   selftests/tc-testing: Add an HFSC qlen accounting test
> 
> [...]

Here is the summary with links:
  - [net,1/2] sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()
    https://git.kernel.org/netdev/net/c/3f981138109f
  - [net,2/2] selftests/tc-testing: Add an HFSC qlen accounting test
    https://git.kernel.org/netdev/net/c/c3572acffb75

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html