Skip to content

Commit

Permalink
Merge branch 'net_sched-make-qlen_notify-idempotent'
Browse files Browse the repository at this point in the history 
Cong Wang says:

====================
net_sched: make ->qlen_notify() idempotent

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 will be 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 5
selftests for each type of Qdisc's we touch here.

All existing and new Qdisc selftests pass after this patchset.

Fixes: 4b549a2 ("fq_codel: Fair Queue Codel AQM")
Fixes: 76e3cc1 ("codel: Controlled Delay AQM")
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
====================

Link: https://patch.msgid.link/20250403211033.166059-1-xiyou.wangcong@gmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Paolo Abeni committed Apr 8, 2025
2 parents 69ae947 + ce94507 commit cd23e77
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 19 deletions.
5 changes: 1 addition & 4 deletions net/sched/sch_codel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 4 additions & 3 deletions net/sched/sch_drr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
Expand Down
8 changes: 4 additions & 4 deletions net/sched/sch_ets.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 2 additions & 4 deletions net/sched/sch_fq_codel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 6 additions & 2 deletions net/sched/sch_hfsc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 2 additions & 0 deletions net/sched/sch_htb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
7 changes: 5 additions & 2 deletions net/sched/sch_qfq.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down
155 changes: 155 additions & 0 deletions tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,160 @@
"$TC qdisc del dev $DUMMY handle 1: root",
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
]
},
{
"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"
]
},
{
"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"
]
},
{
"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"
]
},
{
"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"
]
},
{
"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"
]
}
]

0 comments on commit cd23e77

Please sign in to comment.