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

[netdev-bot+sashiko@kernel.org]

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