[Patch net 00/12] net_sched: make ->qlen_notify() idempotent

[Patch net 00/12] net_sched: make ->qlen_notify() idempotent

[Cong Wang]

Gerrard reported a vulnerability exists in fq_codel where manipulating
the MTU can cause codel_dequeue() to drop all packets. The parent qdisc's
sch->q.qlen is only updated via ->qlen_notify() if the fq_codel queue
remains non-empty after the drops. This discrepancy in qlen between
fq_codel and its parent can lead to a use-after-free condition.

Let's fix this by making all existing ->qlen_notify() idempotent so that
the sch->q.qlen check is no longer necessary.

Patch 1~5 make all existing ->qlen_notify() idempotent to prepare for
patch 6 which removes the sch->q.qlen check. They are followed by 6
selftests for each type of Qdisc's we touch here.

All existing and new Qdisc selftests pass after this patchset.

Fixes: 4b549a2ef4be ("fq_codel: Fair Queue Codel AQM")
Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")

---
Cong Wang (12):
  sch_htb: make htb_qlen_notify() idempotent
  sch_drr: make drr_qlen_notify() idempotent
  sch_hfsc: make hfsc_qlen_notify() idempotent
  sch_qfq: make qfq_qlen_notify() idempotent
  sch_ets: make est_qlen_notify() idempotent
  codel: remove sch->q.qlen check before qdisc_tree_reduce_backlog()
  selftests/tc-testing: Add a test case for FQ_CODEL with HTB parent
  selftests/tc-testing: Add a test case for CODEL with HTB parent
  selftests/tc-testing: Add a test case for FQ_CODEL with QFQ parent
  selftests/tc-testing: Add a test case for FQ_CODEL with HFSC parent
  selftests/tc-testing: Add a test case for FQ_CODEL with DRR parent
  selftests/tc-testing: Add a test case for FQ_CODEL with ETS parent

 net/sched/sch_codel.c                         |   5 +-
 net/sched/sch_drr.c                           |   7 +-
 net/sched/sch_ets.c                           |   8 +-
 net/sched/sch_fq_codel.c                      |   6 +-
 net/sched/sch_hfsc.c                          |   8 +-
 net/sched/sch_htb.c                           |   2 +
 net/sched/sch_qfq.c                           |   7 +-
 .../tc-testing/tc-tests/infra/qdiscs.json     | 188 +++++++++++++++++-
 8 files changed, 211 insertions(+), 20 deletions(-)

-- 
2.34.1

[Patch net 01/12] sch_htb: make htb_qlen_notify() idempotent

[Cong Wang]

htb_qlen_notify() always deactivates the HTB class and in fact could
trigger a warning if it is already deactivated. Therefore, it is not
idempotent and not friendly to its callers, like fq_codel_dequeue().

Let's make it idempotent to ease qdisc_tree_reduce_backlog() callers'
life.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_htb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index c31bc5489bdd..4b9a639b642e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1485,6 +1485,8 @@ static void htb_qlen_notify(struct Qdisc *sch, unsigned long arg)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
 
+	if (!cl->prio_activity)
+		return;
 	htb_deactivate(qdisc_priv(sch), cl);
 }
 
-- 
2.34.1

[Patch net 02/12] sch_drr: make drr_qlen_notify() idempotent

[Cong Wang]

drr_qlen_notify() always deletes the DRR class from its active list
with list_del(), therefore, it is not idempotent and not friendly
to its callers, like fq_codel_dequeue().

Let's make it idempotent to ease qdisc_tree_reduce_backlog() callers'
life. Also change other list_del()'s to list_del_init() just to be
extra safe.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_drr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index c69b999fae17..e0a81d313aa7 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -105,6 +105,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		return -ENOBUFS;
 
 	gnet_stats_basic_sync_init(&cl->bstats);
+	INIT_LIST_HEAD(&cl->alist);
 	cl->common.classid = classid;
 	cl->quantum	   = quantum;
 	cl->qdisc	   = qdisc_create_dflt(sch->dev_queue,
@@ -229,7 +230,7 @@ static void drr_qlen_notify(struct Qdisc *csh, unsigned long arg)
 {
 	struct drr_class *cl = (struct drr_class *)arg;
 
-	list_del(&cl->alist);
+	list_del_init(&cl->alist);
 }
 
 static int drr_dump_class(struct Qdisc *sch, unsigned long arg,
@@ -390,7 +391,7 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
 			if (unlikely(skb == NULL))
 				goto out;
 			if (cl->qdisc->q.qlen == 0)
-				list_del(&cl->alist);
+				list_del_init(&cl->alist);
 
 			bstats_update(&cl->bstats, skb);
 			qdisc_bstats_update(sch, skb);
@@ -431,7 +432,7 @@ static void drr_reset_qdisc(struct Qdisc *sch)
 	for (i = 0; i < q->clhash.hashsize; i++) {
 		hlist_for_each_entry(cl, &q->clhash.hash[i], common.hnode) {
 			if (cl->qdisc->q.qlen)
-				list_del(&cl->alist);
+				list_del_init(&cl->alist);
 			qdisc_reset(cl->qdisc);
 		}
 	}
-- 
2.34.1

[Patch net 03/12] sch_hfsc: make hfsc_qlen_notify() idempotent

[Cong Wang]

hfsc_qlen_notify() is not idempotent either and not friendly
to its callers, like fq_codel_dequeue(). Let's make it idempotent
to ease qdisc_tree_reduce_backlog() callers' life:

1. update_vf() decreases cl->cl_nactive, so we can check whether it is
non-zero before calling it.

2. eltree_remove() always removes RB node cl->el_node, but we can use
   RB_EMPTY_NODE() + RB_CLEAR_NODE() to make it safe.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_hfsc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index c287bf8423b4..ce5045eea065 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -203,7 +203,10 @@ eltree_insert(struct hfsc_class *cl)
 static inline void
 eltree_remove(struct hfsc_class *cl)
 {
-	rb_erase(&cl->el_node, &cl->sched->eligible);
+	if (!RB_EMPTY_NODE(&cl->el_node)) {
+		rb_erase(&cl->el_node, &cl->sched->eligible);
+		RB_CLEAR_NODE(&cl->el_node);
+	}
 }
 
 static inline void
@@ -1220,7 +1223,8 @@ hfsc_qlen_notify(struct Qdisc *sch, unsigned long arg)
 	/* vttree is now handled in update_vf() so that update_vf(cl, 0, 0)
 	 * needs to be called explicitly to remove a class from vttree.
 	 */
-	update_vf(cl, 0, 0);
+	if (cl->cl_nactive)
+		update_vf(cl, 0, 0);
 	if (cl->cl_flags & HFSC_RSC)
 		eltree_remove(cl);
 }
-- 
2.34.1

[Patch net 04/12] sch_qfq: make qfq_qlen_notify() idempotent

[Cong Wang]

qfq_qlen_notify() always deletes its class from its active list
with list_del_init() _and_ calls qfq_deactivate_agg() when the whole list
becomes empty.

To make it idempotent, just skip everything when it is not in the active
list.

Also change other list_del()'s to list_del_init() just to be extra safe.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_qfq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 6a07cdbdb9e1..9f8e1ada058c 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -347,7 +347,7 @@ static void qfq_deactivate_class(struct qfq_sched *q, struct qfq_class *cl)
 	struct qfq_aggregate *agg = cl->agg;
 
 
-	list_del(&cl->alist); /* remove from RR queue of the aggregate */
+	list_del_init(&cl->alist); /* remove from RR queue of the aggregate */
 	if (list_empty(&agg->active)) /* agg is now inactive */
 		qfq_deactivate_agg(q, agg);
 }
@@ -474,6 +474,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	gnet_stats_basic_sync_init(&cl->bstats);
 	cl->common.classid = classid;
 	cl->deficit = lmax;
+	INIT_LIST_HEAD(&cl->alist);
 
 	cl->qdisc = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
 				      classid, NULL);
@@ -982,7 +983,7 @@ static struct sk_buff *agg_dequeue(struct qfq_aggregate *agg,
 	cl->deficit -= (int) len;
 
 	if (cl->qdisc->q.qlen == 0) /* no more packets, remove from list */
-		list_del(&cl->alist);
+		list_del_init(&cl->alist);
 	else if (cl->deficit < qdisc_pkt_len(cl->qdisc->ops->peek(cl->qdisc))) {
 		cl->deficit += agg->lmax;
 		list_move_tail(&cl->alist, &agg->active);
@@ -1415,6 +1416,8 @@ static void qfq_qlen_notify(struct Qdisc *sch, unsigned long arg)
 	struct qfq_sched *q = qdisc_priv(sch);
 	struct qfq_class *cl = (struct qfq_class *)arg;
 
+	if (list_empty(&cl->alist))
+		return;
 	qfq_deactivate_class(q, cl);
 }
 
-- 
2.34.1

[Patch net 05/12] sch_ets: make est_qlen_notify() idempotent

[Cong Wang]

est_qlen_notify() deletes its class from its active list with
list_del() when qlen is 0, therefore, it is not idempotent and
not friendly to its callers, like fq_codel_dequeue().

Let's make it idempotent to ease qdisc_tree_reduce_backlog() callers'
life. Also change other list_del()'s to list_del_init() just to be
extra safe.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_ets.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index 516038a44163..c3bdeb14185b 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -293,7 +293,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
 	 * to remove them.
 	 */
 	if (!ets_class_is_strict(q, cl) && sch->q.qlen)
-		list_del(&cl->alist);
+		list_del_init(&cl->alist);
 }
 
 static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
@@ -488,7 +488,7 @@ static struct sk_buff *ets_qdisc_dequeue(struct Qdisc *sch)
 			if (unlikely(!skb))
 				goto out;
 			if (cl->qdisc->q.qlen == 0)
-				list_del(&cl->alist);
+				list_del_init(&cl->alist);
 			return ets_qdisc_dequeue_skb(sch, skb);
 		}
 
@@ -657,7 +657,7 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 	}
 	for (i = q->nbands; i < oldbands; i++) {
 		if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
-			list_del(&q->classes[i].alist);
+			list_del_init(&q->classes[i].alist);
 		qdisc_tree_flush_backlog(q->classes[i].qdisc);
 	}
 	WRITE_ONCE(q->nstrict, nstrict);
@@ -713,7 +713,7 @@ static void ets_qdisc_reset(struct Qdisc *sch)
 
 	for (band = q->nstrict; band < q->nbands; band++) {
 		if (q->classes[band].qdisc->q.qlen)
-			list_del(&q->classes[band].alist);
+			list_del_init(&q->classes[band].alist);
 	}
 	for (band = 0; band < q->nbands; band++)
 		qdisc_reset(q->classes[band].qdisc);
-- 
2.34.1

[Patch net 06/12] codel: remove sch->q.qlen check before qdisc_tree_reduce_backlog()

[Cong Wang]

After making all ->qlen_notify() callbacks idempotent, now it is safe to
remove the check of qlen!=0 from both fq_codel_dequeue() and
codel_qdisc_dequeue().

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_codel.c    | 5 +----
 net/sched/sch_fq_codel.c | 6 ++----
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
index 81189d02fee7..12dd71139da3 100644
--- a/net/sched/sch_codel.c
+++ b/net/sched/sch_codel.c
@@ -65,10 +65,7 @@ static struct sk_buff *codel_qdisc_dequeue(struct Qdisc *sch)
 			    &q->stats, qdisc_pkt_len, codel_get_enqueue_time,
 			    drop_func, dequeue_func);
 
-	/* We cant call qdisc_tree_reduce_backlog() if our qlen is 0,
-	 * or HTB crashes. Defer it for next round.
-	 */
-	if (q->stats.drop_count && sch->q.qlen) {
+	if (q->stats.drop_count) {
 		qdisc_tree_reduce_backlog(sch, q->stats.drop_count, q->stats.drop_len);
 		q->stats.drop_count = 0;
 		q->stats.drop_len = 0;
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 799f5397ad4c..6c9029f71e88 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -315,10 +315,8 @@ static struct sk_buff *fq_codel_dequeue(struct Qdisc *sch)
 	}
 	qdisc_bstats_update(sch, skb);
 	flow->deficit -= qdisc_pkt_len(skb);
-	/* We cant call qdisc_tree_reduce_backlog() if our qlen is 0,
-	 * or HTB crashes. Defer it for next round.
-	 */
-	if (q->cstats.drop_count && sch->q.qlen) {
+
+	if (q->cstats.drop_count) {
 		qdisc_tree_reduce_backlog(sch, q->cstats.drop_count,
 					  q->cstats.drop_len);
 		q->cstats.drop_count = 0;
-- 
2.34.1

[Patch net 07/12] selftests/tc-testing: Add a test case for FQ_CODEL with HTB parent

[Cong Wang]

Add a test case for FQ_CODEL with HTB parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

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 9044ac054167..06cb2c3c577e 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -126,5 +126,36 @@
             "$TC qdisc del dev $DUMMY root handle 1: drr",
             "$IP addr del 10.10.10.10/24 dev $DUMMY"
         ]
-   }
+   },
+    {
+        "id": "a4bb",
+        "name": "Test FQ_CODEL with HTB parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "fq_codel",
+            "htb"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root htb default 10",
+            "$TC class add dev $DUMMY parent 1: classid 1:10 htb rate 1kbit",
+            "$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
+            "ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.1"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
+    }
 ]
-- 
2.34.1

[Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Cong Wang]

Add a test case for CODEL with HTB parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 31 +++++++++++++++++++
 1 file changed, 31 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 06cb2c3c577e..3ee3197ec7d9 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -157,5 +157,36 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4bd",
+        "name": "Test CODEL with HTB parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "codel",
+            "htb"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root htb default 10",
+            "$TC class add dev $DUMMY parent 1: classid 1:10 htb rate 1kbit ceil 1kbit burst 1 prio 1",
+            "$TC qdisc add dev $DUMMY parent 1:10 handle 10: codel limit 1 target 0.01ms interval 1ms noecn",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
+            "ping -c 2 -i 0 -s 1400 -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.5"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

[Patch net 09/12] selftests/tc-testing: Add a test case for FQ_CODEL with QFQ parent

[Cong Wang]

Add a test case for FQ_CODEL with QFQ parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 31 +++++++++++++++++++
 1 file changed, 31 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 3ee3197ec7d9..d69d2fde1c1c 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -188,5 +188,36 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4be",
+        "name": "Test FQ_CODEL with QFQ parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "fq_codel",
+            "qfq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root qfq",
+            "$TC class add dev $DUMMY parent 1: classid 1:10 qfq weight 1 maxpkt 1000",
+            "$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
+            "ping -c 10 -s 1000 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.1"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

[Patch net 10/12] selftests/tc-testing: Add a test case for FQ_CODEL with HFSC parent

[Cong Wang]

Add a test case for FQ_CODEL with HFSC parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 31 +++++++++++++++++++
 1 file changed, 31 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 d69d2fde1c1c..4fda4007e520 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -219,5 +219,36 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4bf",
+        "name": "Test FQ_CODEL with HFSC parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "fq_codel",
+            "hfsc"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root hfsc default 10",
+            "$TC class add dev $DUMMY parent 1: classid 1:10 hfsc sc rate 1kbit ul rate 1kbit",
+            "$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
+            "ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.1"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

[Patch net 11/12] selftests/tc-testing: Add a test case for FQ_CODEL with DRR parent

[Cong Wang]

Add a test case for FQ_CODEL with DRR parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 31 +++++++++++++++++++
 1 file changed, 31 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 4fda4007e520..a6d6ca245909 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -250,5 +250,36 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4c0",
+        "name": "Test FQ_CODEL with DRR parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "fq_codel",
+            "drr"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root drr",
+            "$TC class add dev $DUMMY parent 1: classid 1:10 drr quantum 1500",
+            "$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
+            "ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.1"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

[Patch net 12/12] selftests/tc-testing: Add a test case for FQ_CODEL with ETS parent

[Cong Wang]

Add a test case for FQ_CODEL with ETS parent to verify packet drop
behavior when the queue becomes empty. This helps ensure proper
notification mechanisms between qdiscs.

Note this is best-effort, it is hard to play with those parameters
perfectly to always trigger ->qlen_notify().

Cc: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 31 +++++++++++++++++++
 1 file changed, 31 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 a6d6ca245909..3e38f81f830c 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -281,5 +281,36 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4c1",
+        "name": "Test FQ_CODEL with ETS parent - force packet drop with empty queue",
+        "category": [
+            "qdisc",
+            "fq_codel",
+            "ets"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1: root ets bands 2 strict 1",
+            "$TC class change dev $DUMMY parent 1: classid 1:1 ets",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
+            "$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1",
+            "ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
+            "sleep 0.1"
+        ],
+        "cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
+        "matchPattern": "dropped [1-9][0-9]*",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Victor Nogueira]

On 20/03/2025 20:25, Cong Wang wrote:
> Add a test case for CODEL with HTB parent to verify packet drop
> behavior when the queue becomes empty. This helps ensure proper
> notification mechanisms between qdiscs.
> 
> Note this is best-effort, it is hard to play with those parameters
> perfectly to always trigger ->qlen_notify().
> 
> Cc: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong, can you double check this test?
I ran all of your other tests and they all succeeded, however
this one specifically is always failing:

Test a4bd: Test CODEL with HTB parent - force packet drop with empty queue

All test results:

1..1
not ok 1 a4bd - Test CODEL with HTB parent - force packet drop with 
empty queue
         Could not match regex pattern. Verify command output:
qdisc codel 10: parent 1:10 limit 1p target 9us interval 999us
  Sent 2884 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
  backlog 0b 0p requeues 0
   count 0 lastcount 0 ldelay 11.5s drop_next 0us
   maxpacket 1442 ecn_mark 0 drop_overlimit 0

cheers,
Victor

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Cong Wang]

On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
> On 20/03/2025 20:25, Cong Wang wrote:
> > Add a test case for CODEL with HTB parent to verify packet drop
> > behavior when the queue becomes empty. This helps ensure proper
> > notification mechanisms between qdiscs.
> > 
> > Note this is best-effort, it is hard to play with those parameters
> > perfectly to always trigger ->qlen_notify().
> > 
> > Cc: Pedro Tammela <pctammela@mojatatu.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> 
> Cong, can you double check this test?
> I ran all of your other tests and they all succeeded, however
> this one specifically is always failing:

Interesting, I thought I completely fixed this before posting after several
rounds of playing with the parameters. I will double check it, maybe it
just becomes less reproducible.

codel is indeed the hardest one, since fq_codel has a mem limit we can
leverage. (In the worst case, we can drop the codel test case.)

Thanks!

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Victor Nogueira]

On 28/03/2025 17:35, Cong Wang wrote:
> On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
>> On 20/03/2025 20:25, Cong Wang wrote:
>>> Add a test case for CODEL with HTB parent to verify packet drop
>>> behavior when the queue becomes empty. This helps ensure proper
>>> notification mechanisms between qdiscs.
>>>
>>> Note this is best-effort, it is hard to play with those parameters
>>> perfectly to always trigger ->qlen_notify().
>>>
>>> Cc: Pedro Tammela <pctammela@mojatatu.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>
>> Cong, can you double check this test?
>> I ran all of your other tests and they all succeeded, however
>> this one specifically is always failing:
> 
> Interesting, I thought I completely fixed this before posting after several
> rounds of playing with the parameters. I will double check it, maybe it
> just becomes less reproducible.

I see.
I experimented with it a bit today and found out that changing the ping
command to:

ping -c 2 -i 0 -s 1500 -I $DUMMY 10.10.10.1 > /dev/null || true

makes the test pass consistently (at least in my environment).
So essentially just changing the "-s" option to 1500.

If you could, please try it out as well.
Maybe I just got lucky.

cheers,
Victor

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Cong Wang]

On Sun, Mar 30, 2025 at 06:05:06PM -0300, Victor Nogueira wrote:
> On 28/03/2025 17:35, Cong Wang wrote:
> > On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
> > > On 20/03/2025 20:25, Cong Wang wrote:
> > > > Add a test case for CODEL with HTB parent to verify packet drop
> > > > behavior when the queue becomes empty. This helps ensure proper
> > > > notification mechanisms between qdiscs.
> > > > 
> > > > Note this is best-effort, it is hard to play with those parameters
> > > > perfectly to always trigger ->qlen_notify().
> > > > 
> > > > Cc: Pedro Tammela <pctammela@mojatatu.com>
> > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > 
> > > Cong, can you double check this test?
> > > I ran all of your other tests and they all succeeded, however
> > > this one specifically is always failing:
> > 
> > Interesting, I thought I completely fixed this before posting after several
> > rounds of playing with the parameters. I will double check it, maybe it
> > just becomes less reproducible.
> 
> I see.
> I experimented with it a bit today and found out that changing the ping
> command to:
> 
> ping -c 2 -i 0 -s 1500 -I $DUMMY 10.10.10.1 > /dev/null || true
> 
> makes the test pass consistently (at least in my environment).
> So essentially just changing the "-s" option to 1500.
> 
> If you could, please try it out as well.
> Maybe I just got lucky.

Sure, I will change it to 1500.

Thanks!

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Cong Wang]

On Sun, Mar 30, 2025 at 4:32 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Mar 30, 2025 at 06:05:06PM -0300, Victor Nogueira wrote:
> > On 28/03/2025 17:35, Cong Wang wrote:
> > > On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
> > > > On 20/03/2025 20:25, Cong Wang wrote:
> > > > > Add a test case for CODEL with HTB parent to verify packet drop
> > > > > behavior when the queue becomes empty. This helps ensure proper
> > > > > notification mechanisms between qdiscs.
> > > > >
> > > > > Note this is best-effort, it is hard to play with those parameters
> > > > > perfectly to always trigger ->qlen_notify().
> > > > >
> > > > > Cc: Pedro Tammela <pctammela@mojatatu.com>
> > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > >
> > > > Cong, can you double check this test?
> > > > I ran all of your other tests and they all succeeded, however
> > > > this one specifically is always failing:
> > >
> > > Interesting, I thought I completely fixed this before posting after several
> > > rounds of playing with the parameters. I will double check it, maybe it
> > > just becomes less reproducible.
> >
> > I see.
> > I experimented with it a bit today and found out that changing the ping
> > command to:
> >
> > ping -c 2 -i 0 -s 1500 -I $DUMMY 10.10.10.1 > /dev/null || true
> >
> > makes the test pass consistently (at least in my environment).
> > So essentially just changing the "-s" option to 1500.
> >
> > If you could, please try it out as well.
> > Maybe I just got lucky.
>
> Sure, I will change it to 1500.

Hmm, it failed on my side:

Test a4bd: Test CODEL with HTB parent - force packet drop with empty queue

-----> prepare stage *** Could not execute: "ping -c 2 -i 0 -s 1500 -I
$DUMMY 10.10.10.1 > /dev/null || true"

-----> prepare stage *** Error message: "Command "/sbin/ip netns exec
tcut-3264298917 ping -c 2 -i 0 -s 1500 -I dummy1ida4bd 10.10.10.1 >
/dev/null || true" timed out
"
returncode 255; expected [0]

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Victor Nogueira]

On 02/04/2025 21:40, Cong Wang wrote:
> On Sun, Mar 30, 2025 at 4:32 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> On Sun, Mar 30, 2025 at 06:05:06PM -0300, Victor Nogueira wrote:
>>> On 28/03/2025 17:35, Cong Wang wrote:
>>>> On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
>>>>> On 20/03/2025 20:25, Cong Wang wrote:
>>>>>> Add a test case for CODEL with HTB parent to verify packet drop
>>>>>> behavior when the queue becomes empty. This helps ensure proper
>>>>>> notification mechanisms between qdiscs.
>>>>>>
>>>>>> Note this is best-effort, it is hard to play with those parameters
>>>>>> perfectly to always trigger ->qlen_notify().
>>>>>>
>>>>>> Cc: Pedro Tammela <pctammela@mojatatu.com>
>>>>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>>>
>>>>> Cong, can you double check this test?
>>>>> I ran all of your other tests and they all succeeded, however
>>>>> this one specifically is always failing:
>>>>
>>>> Interesting, I thought I completely fixed this before posting after several
>>>> rounds of playing with the parameters. I will double check it, maybe it
>>>> just becomes less reproducible.
>>>
>>> I see.
>>> I experimented with it a bit today and found out that changing the ping
>>> command to:
>>>
>>> ping -c 2 -i 0 -s 1500 -I $DUMMY 10.10.10.1 > /dev/null || true
>>>
>>> makes the test pass consistently (at least in my environment).
>>> So essentially just changing the "-s" option to 1500.
>>>
>>> If you could, please try it out as well.
>>> Maybe I just got lucky.
>>
>> Sure, I will change it to 1500.
> 
> Hmm, it failed on my side:
> 
> Test a4bd: Test CODEL with HTB parent - force packet drop with empty queue
> [...]

I see, this test seems to be very environment sensitive.
I think it will be better if we do what you suggested earlier.
It's not fair to stall this set because of this single test.
Can you resend your patches excluding this test?
I can try and figure this one out later so that we can
unblock.

cheers,
Victor

Re: [Patch net 08/12] selftests/tc-testing: Add a test case for CODEL with HTB parent

[Cong Wang]

On Wed, Apr 02, 2025 at 10:23:22PM -0300, Victor Nogueira wrote:
> On 02/04/2025 21:40, Cong Wang wrote:
> > On Sun, Mar 30, 2025 at 4:32 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > 
> > > On Sun, Mar 30, 2025 at 06:05:06PM -0300, Victor Nogueira wrote:
> > > > On 28/03/2025 17:35, Cong Wang wrote:
> > > > > On Sun, Mar 23, 2025 at 07:48:39PM -0300, Victor Nogueira wrote:
> > > > > > On 20/03/2025 20:25, Cong Wang wrote:
> > > > > > > Add a test case for CODEL with HTB parent to verify packet drop
> > > > > > > behavior when the queue becomes empty. This helps ensure proper
> > > > > > > notification mechanisms between qdiscs.
> > > > > > > 
> > > > > > > Note this is best-effort, it is hard to play with those parameters
> > > > > > > perfectly to always trigger ->qlen_notify().
> > > > > > > 
> > > > > > > Cc: Pedro Tammela <pctammela@mojatatu.com>
> > > > > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > > > > 
> > > > > > Cong, can you double check this test?
> > > > > > I ran all of your other tests and they all succeeded, however
> > > > > > this one specifically is always failing:
> > > > > 
> > > > > Interesting, I thought I completely fixed this before posting after several
> > > > > rounds of playing with the parameters. I will double check it, maybe it
> > > > > just becomes less reproducible.
> > > > 
> > > > I see.
> > > > I experimented with it a bit today and found out that changing the ping
> > > > command to:
> > > > 
> > > > ping -c 2 -i 0 -s 1500 -I $DUMMY 10.10.10.1 > /dev/null || true
> > > > 
> > > > makes the test pass consistently (at least in my environment).
> > > > So essentially just changing the "-s" option to 1500.
> > > > 
> > > > If you could, please try it out as well.
> > > > Maybe I just got lucky.
> > > 
> > > Sure, I will change it to 1500.
> > 
> > Hmm, it failed on my side:
> > 
> > Test a4bd: Test CODEL with HTB parent - force packet drop with empty queue
> > [...]
> 
> I see, this test seems to be very environment sensitive.
> I think it will be better if we do what you suggested earlier.
> It's not fair to stall this set because of this single test.
> Can you resend your patches excluding this test?

Sure, will do. It sounds like a good plan.

> I can try and figure this one out later so that we can
> unblock.

I really appreciate this.

Thanks!