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

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

[Jérémy Jean]

A netem child can re-enter qfq_enqueue() through a duplicating netem.
If lmax was lowered with packets queued, qfq_change_agg() can move the
class and free its old aggregate. This can cause a UAF or put the
class on the wrong active list.

Restart qfq_dequeue() if a selection peek changes the aggregate. Charge
the serving aggregate before the post-dequeue peek. After this peek,
requeue only an active class using cl->agg. Recheck activity and use
cl->agg after the peek in qfq_enqueue().

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 v2 after review from Sashiko:
- Cover the post-dequeue peek without moving service accounting.
- Clarify the trigger.

v1: https://lore.kernel.org/all/20261106200236.716641-2-Jeremy.Jean@oss.cyber.gouv.fr/
Sashiko: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106200236.716641-2-Jeremy.Jean%40oss.cyber.gouv.fr

 net/sched/sch_qfq.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index e900890..82903f9 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -992,8 +992,7 @@ static void qfq_update_eligible(struct qfq_sched *q)
 }
 
 /* Dequeue head packet of the head class in the DRR queue of the aggregate. */
-static struct sk_buff *agg_dequeue(struct qfq_aggregate *agg,
-				   struct qfq_class *cl, unsigned int len)
+static struct sk_buff *agg_dequeue(struct qfq_class *cl, unsigned int len)
 {
 	struct sk_buff *skb = qdisc_dequeue_peeked(cl->qdisc);
 
@@ -1002,13 +1001,6 @@ 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_init(&cl->alist);
-	else if (cl->deficit < qdisc_peek_len(cl->qdisc)) {
-		cl->deficit += agg->lmax;
-		list_move_tail(&cl->alist, &agg->active);
-	}
-
 	return skb;
 }
 
@@ -1103,6 +1095,7 @@ 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);
+restart:;
 	struct qfq_aggregate *in_serv_agg = q->in_serv_agg;
 	struct qfq_class *cl;
 	struct sk_buff *skb = NULL;
@@ -1112,8 +1105,11 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
 	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,13 +1147,15 @@ 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);
 
-	skb = agg_dequeue(in_serv_agg, cl, len);
+	skb = agg_dequeue(cl, len);
 
 	if (!skb) {
 		qdisc_qlen_inc(sch);
@@ -1181,6 +1179,13 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
 		 len, (unsigned long long) in_serv_agg->F,
 		 (unsigned long long) q->V);
 
+	if (cl->qdisc->q.qlen == 0) /* no more packets, remove from list */
+		list_del_init(&cl->alist);
+	else if (cl->deficit < qdisc_peek_len(cl->qdisc) && cl_is_active(cl)) {
+		cl->deficit += cl->agg->lmax;
+		list_move_tail(&cl->alist, &cl->agg->active);
+	}
+
 	return skb;
 }
 
@@ -1274,9 +1279,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 v2] 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 code inspection). If it was triggered please include
   the symptoms, like the stack trace or error 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.

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch does not fully fix the use-after-free it targets.
- [Low] qfq_dequeue() puts a `restart:;` label (a label on a null
  statement) before its local declarations with initialisers (`struct…

--

net/sched: sch_qfq: revalidate aggregate after peek

This patch makes qfq_dequeue() restart when a child peek re-enters
qfq_enqueue() and changes the in-service aggregate or the class's
aggregate. It moves the DRR requeue logic out of agg_dequeue() and into
qfq_dequeue(), after the service charge, using cl->agg. It also rechecks
class activity after the peek in qfq_enqueue().

> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index e900890e9c544..82903f9e58504 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c

[ ... ]

> @@ -1103,6 +1095,7 @@ 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);
> +restart:;
>  	struct qfq_aggregate *in_serv_agg = q->in_serv_agg;
>  	struct qfq_class *cl;
>  	struct sk_buff *skb = NULL;

[Severity: Low]
This isn't a bug, but the restart label sits in front of the local
declarations, so the backward gotos run their initializers again:

restart:;
	struct qfq_aggregate *in_serv_agg = q->in_serv_agg;
	...
	struct sk_buff *skb = NULL;
	...
	unsigned int len = 0;

This relies on declarations following a statement. Mainline no longer
passes -Wdeclaration-after-statement. This patch has Cc: stable and a
Fixes: tag going back to 2012, though.

Would older stable trees that still use that flag (5.10, 5.15 or 6.1, for
example) warn here, or fail to build with CONFIG_WERROR?

Would a loop around the selection step, or assigning these variables
after the label, be a simpler way to write the retry?

[ ... ]

> @@ -1151,13 +1147,15 @@ 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);
>  
> -	skb = agg_dequeue(in_serv_agg, cl, len);
> +	skb = agg_dequeue(cl, len);

[Severity: High]
Can the child dequeue here still re-enter qfq_enqueue() and free
in_serv_agg?

The new restart checks only come after the two qfq_peek_skb() calls.
agg_dequeue() calls qdisc_dequeue_peeked(cl->qdisc). That helper only
takes the skb from gso_skb if the child's ->peek() cached one there.
Otherwise it calls the child's ->dequeue() again.

sch_prio is one such child. prio_peek() caches nothing at the prio level,
and prio_dequeue() calls qdisc_dequeue_peeked() on each band, starting
from band 0.

Here is an example setup. The QFQ class child is prio. Band 0 is a netem
A0 with a delay, whose child is a netem B with duplicate. Band 1 is a
pfifo.

At peek time, A0's head is not due yet. The peek returns band 1's skb
without re-entering QFQ, so both restart checks pass. Suppose A0's head
becomes due before agg_dequeue() runs, and lmax was lowered while packets
were queued:

qfq_dequeue()
  agg_dequeue(cl, len)
    qdisc_dequeue_peeked(prio)
      prio_dequeue()
        qdisc_dequeue_peeked(A0)
          netem_dequeue(A0)
            qdisc_enqueue(skb, B)
              netem_enqueue(B)
                rootq->enqueue(skb2, rootq, to_free)
                  qfq_enqueue()
                    qfq_change_agg(cl)
                      qfq_deactivate_class()
                        qfq_deactivate_agg()
                      qfq_rm_from_agg()
                        qfq_destroy_agg()
                          kfree(agg)

qfq_deactivate_agg() and qfq_destroy_agg() update q->in_serv_agg. They
cannot update the local copy in qfq_dequeue(), and that copy is then
used after agg_dequeue() returns:

	if (unlikely(in_serv_agg->budget < len))
		in_serv_agg->budget = 0;
	else
		in_serv_agg->budget -= len;

The pr_debug() that follows also reads in_serv_agg->F.

Is this a write to the freed aggregate? And if the aggregate is not
freed, does the service still get charged to an aggregate that is no
longer in service?

Could q->in_serv_agg and cl->agg be revalidated after agg_dequeue() too?
Alternatively, the code could be restructured so that no aggregate
pointer is used after any child dequeue or peek.

>  
>  	if (!skb) {
>  		qdisc_qlen_inc(sch);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110222946.2147440-2-Jeremy.Jean%40oss.cyber.gouv.fr