Skip to content

Commit cd23e77

Browse files
author
Paolo Abeni
committed
Merge branch 'net_sched-make-qlen_notify-idempotent'
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>
2 parents 69ae947 + ce94507 commit cd23e77

File tree

8 files changed

+179
-19
lines changed

8 files changed

+179
-19
lines changed

‎net/sched/sch_codel.c

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,7 @@ static struct sk_buff *codel_qdisc_dequeue(struct Qdisc *sch)
6565
&q->stats, qdisc_pkt_len, codel_get_enqueue_time,
6666
drop_func, dequeue_func);
6767

68-
/* We cant call qdisc_tree_reduce_backlog() if our qlen is 0,
69-
* or HTB crashes. Defer it for next round.
70-
*/
71-
if (q->stats.drop_count && sch->q.qlen) {
68+
if (q->stats.drop_count) {
7269
qdisc_tree_reduce_backlog(sch, q->stats.drop_count, q->stats.drop_len);
7370
q->stats.drop_count = 0;
7471
q->stats.drop_len = 0;

‎net/sched/sch_drr.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
105105
return -ENOBUFS;
106106

107107
gnet_stats_basic_sync_init(&cl->bstats);
108+
INIT_LIST_HEAD(&cl->alist);
108109
cl->common.classid = classid;
109110
cl->quantum = quantum;
110111
cl->qdisc = qdisc_create_dflt(sch->dev_queue,
@@ -229,7 +230,7 @@ static void drr_qlen_notify(struct Qdisc *csh, unsigned long arg)
229230
{
230231
struct drr_class *cl = (struct drr_class *)arg;
231232

232-
list_del(&cl->alist);
233+
list_del_init(&cl->alist);
233234
}
234235

235236
static int drr_dump_class(struct Qdisc *sch, unsigned long arg,
@@ -390,7 +391,7 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
390391
if (unlikely(skb == NULL))
391392
goto out;
392393
if (cl->qdisc->q.qlen == 0)
393-
list_del(&cl->alist);
394+
list_del_init(&cl->alist);
394395

395396
bstats_update(&cl->bstats, skb);
396397
qdisc_bstats_update(sch, skb);
@@ -431,7 +432,7 @@ static void drr_reset_qdisc(struct Qdisc *sch)
431432
for (i = 0; i < q->clhash.hashsize; i++) {
432433
hlist_for_each_entry(cl, &q->clhash.hash[i], common.hnode) {
433434
if (cl->qdisc->q.qlen)
434-
list_del(&cl->alist);
435+
list_del_init(&cl->alist);
435436
qdisc_reset(cl->qdisc);
436437
}
437438
}

‎net/sched/sch_ets.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
293293
* to remove them.
294294
*/
295295
if (!ets_class_is_strict(q, cl) && sch->q.qlen)
296-
list_del(&cl->alist);
296+
list_del_init(&cl->alist);
297297
}
298298

299299
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)
488488
if (unlikely(!skb))
489489
goto out;
490490
if (cl->qdisc->q.qlen == 0)
491-
list_del(&cl->alist);
491+
list_del_init(&cl->alist);
492492
return ets_qdisc_dequeue_skb(sch, skb);
493493
}
494494

@@ -657,7 +657,7 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
657657
}
658658
for (i = q->nbands; i < oldbands; i++) {
659659
if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
660-
list_del(&q->classes[i].alist);
660+
list_del_init(&q->classes[i].alist);
661661
qdisc_tree_flush_backlog(q->classes[i].qdisc);
662662
}
663663
WRITE_ONCE(q->nstrict, nstrict);
@@ -713,7 +713,7 @@ static void ets_qdisc_reset(struct Qdisc *sch)
713713

714714
for (band = q->nstrict; band < q->nbands; band++) {
715715
if (q->classes[band].qdisc->q.qlen)
716-
list_del(&q->classes[band].alist);
716+
list_del_init(&q->classes[band].alist);
717717
}
718718
for (band = 0; band < q->nbands; band++)
719719
qdisc_reset(q->classes[band].qdisc);

‎net/sched/sch_fq_codel.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -315,10 +315,8 @@ static struct sk_buff *fq_codel_dequeue(struct Qdisc *sch)
315315
}
316316
qdisc_bstats_update(sch, skb);
317317
flow->deficit -= qdisc_pkt_len(skb);
318-
/* We cant call qdisc_tree_reduce_backlog() if our qlen is 0,
319-
* or HTB crashes. Defer it for next round.
320-
*/
321-
if (q->cstats.drop_count && sch->q.qlen) {
318+
319+
if (q->cstats.drop_count) {
322320
qdisc_tree_reduce_backlog(sch, q->cstats.drop_count,
323321
q->cstats.drop_len);
324322
q->cstats.drop_count = 0;

‎net/sched/sch_hfsc.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,10 @@ eltree_insert(struct hfsc_class *cl)
203203
static inline void
204204
eltree_remove(struct hfsc_class *cl)
205205
{
206-
rb_erase(&cl->el_node, &cl->sched->eligible);
206+
if (!RB_EMPTY_NODE(&cl->el_node)) {
207+
rb_erase(&cl->el_node, &cl->sched->eligible);
208+
RB_CLEAR_NODE(&cl->el_node);
209+
}
207210
}
208211

209212
static inline void
@@ -1220,7 +1223,8 @@ hfsc_qlen_notify(struct Qdisc *sch, unsigned long arg)
12201223
/* vttree is now handled in update_vf() so that update_vf(cl, 0, 0)
12211224
* needs to be called explicitly to remove a class from vttree.
12221225
*/
1223-
update_vf(cl, 0, 0);
1226+
if (cl->cl_nactive)
1227+
update_vf(cl, 0, 0);
12241228
if (cl->cl_flags & HFSC_RSC)
12251229
eltree_remove(cl);
12261230
}

‎net/sched/sch_htb.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1485,6 +1485,8 @@ static void htb_qlen_notify(struct Qdisc *sch, unsigned long arg)
14851485
{
14861486
struct htb_class *cl = (struct htb_class *)arg;
14871487

1488+
if (!cl->prio_activity)
1489+
return;
14881490
htb_deactivate(qdisc_priv(sch), cl);
14891491
}
14901492

‎net/sched/sch_qfq.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ static void qfq_deactivate_class(struct qfq_sched *q, struct qfq_class *cl)
347347
struct qfq_aggregate *agg = cl->agg;
348348

349349

350-
list_del(&cl->alist); /* remove from RR queue of the aggregate */
350+
list_del_init(&cl->alist); /* remove from RR queue of the aggregate */
351351
if (list_empty(&agg->active)) /* agg is now inactive */
352352
qfq_deactivate_agg(q, agg);
353353
}
@@ -474,6 +474,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
474474
gnet_stats_basic_sync_init(&cl->bstats);
475475
cl->common.classid = classid;
476476
cl->deficit = lmax;
477+
INIT_LIST_HEAD(&cl->alist);
477478

478479
cl->qdisc = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
479480
classid, NULL);
@@ -982,7 +983,7 @@ static struct sk_buff *agg_dequeue(struct qfq_aggregate *agg,
982983
cl->deficit -= (int) len;
983984

984985
if (cl->qdisc->q.qlen == 0) /* no more packets, remove from list */
985-
list_del(&cl->alist);
986+
list_del_init(&cl->alist);
986987
else if (cl->deficit < qdisc_pkt_len(cl->qdisc->ops->peek(cl->qdisc))) {
987988
cl->deficit += agg->lmax;
988989
list_move_tail(&cl->alist, &agg->active);
@@ -1415,6 +1416,8 @@ static void qfq_qlen_notify(struct Qdisc *sch, unsigned long arg)
14151416
struct qfq_sched *q = qdisc_priv(sch);
14161417
struct qfq_class *cl = (struct qfq_class *)arg;
14171418

1419+
if (list_empty(&cl->alist))
1420+
return;
14181421
qfq_deactivate_class(q, cl);
14191422
}
14201423

‎tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,5 +158,160 @@
158158
"$TC qdisc del dev $DUMMY handle 1: root",
159159
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
160160
]
161+
},
162+
{
163+
"id": "a4bb",
164+
"name": "Test FQ_CODEL with HTB parent - force packet drop with empty queue",
165+
"category": [
166+
"qdisc",
167+
"fq_codel",
168+
"htb"
169+
],
170+
"plugins": {
171+
"requires": "nsPlugin"
172+
},
173+
"setup": [
174+
"$IP link set dev $DUMMY up || true",
175+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
176+
"$TC qdisc add dev $DUMMY handle 1: root htb default 10",
177+
"$TC class add dev $DUMMY parent 1: classid 1:10 htb rate 1kbit",
178+
"$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
179+
"$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
180+
"ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
181+
"sleep 0.1"
182+
],
183+
"cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
184+
"expExitCode": "0",
185+
"verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
186+
"matchPattern": "dropped [1-9][0-9]*",
187+
"matchCount": "1",
188+
"teardown": [
189+
"$TC qdisc del dev $DUMMY handle 1: root",
190+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
191+
]
192+
},
193+
{
194+
"id": "a4be",
195+
"name": "Test FQ_CODEL with QFQ parent - force packet drop with empty queue",
196+
"category": [
197+
"qdisc",
198+
"fq_codel",
199+
"qfq"
200+
],
201+
"plugins": {
202+
"requires": "nsPlugin"
203+
},
204+
"setup": [
205+
"$IP link set dev $DUMMY up || true",
206+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
207+
"$TC qdisc add dev $DUMMY handle 1: root qfq",
208+
"$TC class add dev $DUMMY parent 1: classid 1:10 qfq weight 1 maxpkt 1000",
209+
"$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
210+
"$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
211+
"ping -c 10 -s 1000 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
212+
"sleep 0.1"
213+
],
214+
"cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
215+
"expExitCode": "0",
216+
"verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
217+
"matchPattern": "dropped [1-9][0-9]*",
218+
"matchCount": "1",
219+
"teardown": [
220+
"$TC qdisc del dev $DUMMY handle 1: root",
221+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
222+
]
223+
},
224+
{
225+
"id": "a4bf",
226+
"name": "Test FQ_CODEL with HFSC parent - force packet drop with empty queue",
227+
"category": [
228+
"qdisc",
229+
"fq_codel",
230+
"hfsc"
231+
],
232+
"plugins": {
233+
"requires": "nsPlugin"
234+
},
235+
"setup": [
236+
"$IP link set dev $DUMMY up || true",
237+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
238+
"$TC qdisc add dev $DUMMY handle 1: root hfsc default 10",
239+
"$TC class add dev $DUMMY parent 1: classid 1:10 hfsc sc rate 1kbit ul rate 1kbit",
240+
"$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
241+
"$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
242+
"ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
243+
"sleep 0.1"
244+
],
245+
"cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
246+
"expExitCode": "0",
247+
"verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
248+
"matchPattern": "dropped [1-9][0-9]*",
249+
"matchCount": "1",
250+
"teardown": [
251+
"$TC qdisc del dev $DUMMY handle 1: root",
252+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
253+
]
254+
},
255+
{
256+
"id": "a4c0",
257+
"name": "Test FQ_CODEL with DRR parent - force packet drop with empty queue",
258+
"category": [
259+
"qdisc",
260+
"fq_codel",
261+
"drr"
262+
],
263+
"plugins": {
264+
"requires": "nsPlugin"
265+
},
266+
"setup": [
267+
"$IP link set dev $DUMMY up || true",
268+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
269+
"$TC qdisc add dev $DUMMY handle 1: root drr",
270+
"$TC class add dev $DUMMY parent 1: classid 1:10 drr quantum 1500",
271+
"$TC qdisc add dev $DUMMY parent 1:10 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
272+
"$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:10",
273+
"ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
274+
"sleep 0.1"
275+
],
276+
"cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
277+
"expExitCode": "0",
278+
"verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
279+
"matchPattern": "dropped [1-9][0-9]*",
280+
"matchCount": "1",
281+
"teardown": [
282+
"$TC qdisc del dev $DUMMY handle 1: root",
283+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
284+
]
285+
},
286+
{
287+
"id": "a4c1",
288+
"name": "Test FQ_CODEL with ETS parent - force packet drop with empty queue",
289+
"category": [
290+
"qdisc",
291+
"fq_codel",
292+
"ets"
293+
],
294+
"plugins": {
295+
"requires": "nsPlugin"
296+
},
297+
"setup": [
298+
"$IP link set dev $DUMMY up || true",
299+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
300+
"$TC qdisc add dev $DUMMY handle 1: root ets bands 2 strict 1",
301+
"$TC class change dev $DUMMY parent 1: classid 1:1 ets",
302+
"$TC qdisc add dev $DUMMY parent 1:1 handle 10: fq_codel memory_limit 1 flows 1 target 0.1ms interval 1ms",
303+
"$TC filter add dev $DUMMY parent 1: protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1",
304+
"ping -c 5 -f -I $DUMMY 10.10.10.1 > /dev/null || true",
305+
"sleep 0.1"
306+
],
307+
"cmdUnderTest": "$TC -s qdisc show dev $DUMMY",
308+
"expExitCode": "0",
309+
"verifyCmd": "$TC -s qdisc show dev $DUMMY | grep -A 5 'qdisc fq_codel'",
310+
"matchPattern": "dropped [1-9][0-9]*",
311+
"matchCount": "1",
312+
"teardown": [
313+
"$TC qdisc del dev $DUMMY handle 1: root",
314+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
315+
]
161316
}
162317
]

0 commit comments

Comments
 (0)