[PATCH net v3] net/sched: sch_qfq: revalidate aggregate after peek

[PATCH net v3] net/sched: sch_qfq: revalidate aggregate after peek

[Jérémy Jean]

A QFQ child callback can re-enter qfq_enqueue() during a peek or
dequeue. qfq_enqueue() can then move the class and free its
aggregate. This can cause a UAF or corrupt the active lists and
service accounting.

Restart qfq_dequeue() after a selection change. Defer recursive root
enqueues through agg_dequeue() and replay them after service
accounting. Keep replay drops on the same root. Correct ancestor
counts after failures. Recheck activity and use the current aggregate
in qfq_enqueue(). Keep declarations before the retry label.

Fixes: 462dbc9101ac ("pkt_sched: QFQ Plus: fair-queueing service at DRR cost")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
Changes in v3 after review from Sashiko:
- Keep native child peeks. Defer recursive enqueues until agg_dequeue() returns.
- Keep replay drops on the stable root. Repair ancestor accounting on failure.
- Keep retry declarations before the label.

Changes in v2 after review from Sashiko:
- Cover the post-dequeue peek.
- Clarify the trigger.

v1: https://lore.kernel.org/all/20261106200236.716641-2-Jeremy.Jean@oss.cyber.gouv.fr/
v2: https://lore.kernel.org/all/20261110222946.2147440-2-Jeremy.Jean@oss.cyber.gouv.fr/

 net/sched/sch_qfq.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index e900890..d867d3b 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -182,6 +182,7 @@ struct qfq_sched {
 
 	u64			oldV, V;	/* Precise virtual times. */
 	struct qfq_aggregate	*in_serv_agg;   /* Aggregate being served. */
+	struct sk_buff_head	*deferred_enqueues;
 	u32			wsum;		/* weight sum */
 	u32			iwsum;		/* inverse weight sum */
 
@@ -1103,17 +1104,25 @@ static void qfq_schedule_agg(struct qfq_sched *q, struct qfq_aggregate *agg);
 static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
 {
 	struct qfq_sched *q = qdisc_priv(sch);
-	struct qfq_aggregate *in_serv_agg = q->in_serv_agg;
+	struct qfq_aggregate *in_serv_agg;
 	struct qfq_class *cl;
-	struct sk_buff *skb = NULL;
+	struct sk_buff_head deferred_enqueues;
+	struct sk_buff *skb, *next;
 	/* next-packet len, 0 means no more active classes in in-service agg */
-	unsigned int len = 0;
+	unsigned int len;
+
+restart:
+	in_serv_agg = q->in_serv_agg;
+	len = 0;
 
 	if (in_serv_agg == NULL)
 		return NULL;
 
-	if (!list_empty(&in_serv_agg->active))
+	if (!list_empty(&in_serv_agg->active)) {
 		skb = qfq_peek_skb(in_serv_agg, &cl, &len);
+		if (unlikely(in_serv_agg != q->in_serv_agg || cl->agg != in_serv_agg))
+			goto restart;
+	}
 
 	/*
 	 * If there are no active classes in the in-service aggregate,
@@ -1151,17 +1160,23 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
 		 */
 		in_serv_agg = q->in_serv_agg = qfq_choose_next_agg(q);
 		skb = qfq_peek_skb(in_serv_agg, &cl, &len);
+		if (unlikely(in_serv_agg != q->in_serv_agg || cl->agg != in_serv_agg))
+			goto restart;
 	}
 	if (!skb)
 		return NULL;
 
 	qdisc_qlen_dec(sch);
 
+	qdisc_root_sleeping(sch)->flags |= TCQ_F_DEQUEUE_DROPS;
+	__skb_queue_head_init(&deferred_enqueues);
+	q->deferred_enqueues = &deferred_enqueues;
 	skb = agg_dequeue(in_serv_agg, cl, len);
+	q->deferred_enqueues = NULL;
 
 	if (!skb) {
 		qdisc_qlen_inc(sch);
-		return NULL;
+		goto replay_deferred;
 	}
 
 	qdisc_qstats_backlog_dec(sch, skb);
@@ -1181,6 +1196,11 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
 		 len, (unsigned long long) in_serv_agg->F,
 		 (unsigned long long) q->V);
 
+replay_deferred:
+	while ((next = __skb_dequeue(&deferred_enqueues)) != NULL)
+		if (qdisc_enqueue(next, sch, &qdisc_root_sleeping(sch)->to_free))
+			qdisc_tree_reduce_backlog(sch, 1, qdisc_pkt_len(next));
+
 	return skb;
 }
 
@@ -1236,6 +1256,11 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct qfq_aggregate *agg;
 	int err = 0;
 
+	if (unlikely(q->deferred_enqueues)) {
+		__skb_queue_tail(q->deferred_enqueues, skb);
+		return NET_XMIT_SUCCESS;
+	}
+
 	cl = qfq_classify(skb, sch, &err);
 	if (cl == NULL) {
 		if (err & __NET_XMIT_BYPASS)
@@ -1274,9 +1299,10 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	/* if the class is active, then done here */
 	if (cl_is_active(cl)) {
 		if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
-		    list_first_entry(&agg->active, struct qfq_class, alist)
+		    cl_is_active(cl) &&
+		    list_first_entry(&cl->agg->active, struct qfq_class, alist)
 		    == cl && cl->deficit < len)
-			list_move_tail(&cl->alist, &agg->active);
+			list_move_tail(&cl->alist, &cl->agg->active);
 
 		return err;
 	}
-- 
2.47.3

Re: [PATCH net v3] net/sched: sch_qfq: revalidate aggregate after peek

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.