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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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 fix is incomplete.
- [High] The new `goto restart` in qfq_dequeue() can loop forever.
- [Low] The commit message omits the trigger mechanism and one
  behavioural change.

--

net/sched: sch_qfq: revalidate aggregate after peek

This patch makes qfq_dequeue() reload q->in_serv_agg after each
qfq_peek_skb() call. It restarts if q->in_serv_agg or cl->agg changed,
because a child peek can re-enter QFQ and free the in-service aggregate.
It also makes qfq_enqueue() recheck cl_is_active() and use cl->agg after
the child peek.

> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index e900890e9c544..517e9083b403e 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -1103,17 +1103,24 @@ static void qfq_schedule_agg(struct qfq_sched *q, struct qfq_aggregate *agg);
>  static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
>  {

[ ... ]

> +restart:
> +	in_serv_agg = q->in_serv_agg;
> +	skb = NULL;
> +	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;
> +	}

[Severity: High]
Can this goto restart spin forever when cl is linked on
in_serv_agg->active but cl->agg points to a different aggregate?

That state looks reachable through the child peek in agg_dequeue(), which
this patch leaves unguarded (see below). Take a netem child that feeds a
duplicating netem. The nested rootq->enqueue() lands in qfq_enqueue(). If
cl->agg->lmax < len, qfq_change_agg() moves cl from the in-service
aggregate A to aggregate C. A survives because it has other classes:

qfq_dequeue()
  agg_dequeue(A, cl, len)
    qdisc_peek_len(cl->qdisc)
      netem_dequeue() -> netem_enqueue(dup) -> rootq->enqueue()
        qfq_enqueue()
          qfq_change_agg()
            qfq_deactivate_class()   /* cl removed from A->active */
            qfq_add_to_agg()         /* cl->agg = C, cl on C->active */
    cl->deficit += agg->lmax;
    list_move_tail(&cl->alist, &agg->active);   /* agg is still A */

Now cl is on A->active with cl->agg == C. Once cl reaches the head of A,
qfq_peek_skb(A) returns cl and the cl->agg != in_serv_agg check is true.
The restart then reloads the same in_serv_agg and peeks the same head
class again.

For a qdisc_peek_dequeued() child, the peek just returns the cached
gso_skb, so nothing changes between iterations. Would qfq_dequeue() then
loop forever while holding the root qdisc lock?

C may also stay scheduled with an empty active list. A later
qfq_choose_next_agg() followed by qfq_peek_skb() could then call
list_first_entry() on an empty list.

> @@ -1151,6 +1158,8 @@ 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;

[Severity: High]
Is the fix complete? Right after this, qfq_dequeue() calls
agg_dequeue(in_serv_agg, cl, len). That does another child peek through
qdisc_peek_len() and then uses the cached aggregate without revalidating
it:

net/sched/sch_qfq.c:agg_dequeue() {
    ...
	else if (cl->deficit < qdisc_peek_len(cl->qdisc)) {
		cl->deficit += agg->lmax;
		list_move_tail(&cl->alist, &agg->active);
	}
    ...
}

qdisc_peek_len() calls sch->ops->peek(sch) directly. That is the same
re-entry point this patch guards after qfq_peek_skb().

Take a QFQ class whose child is netem N1, where N1's child is a
duplicating netem N2. The free looks reachable:

qfq_dequeue()
  agg_dequeue()
    qdisc_peek_len(N1)
      qdisc_peek_dequeued() -> netem_dequeue(N1)
        qdisc_enqueue(skb, N2) -> netem_enqueue(N2)
          rootq->enqueue(skb2, rootq, to_free)
            qfq_enqueue()
              qfq_change_agg()       /* cl->agg->lmax < len */
                qfq_rm_from_agg()
                  qfq_destroy_agg()  /* num_classes == 1, kfree(agg) */

The cl->agg->lmax < len condition can hold if lmax is lowered through
qfq_change_class() while packets are queued in netem.

After that, reading agg->lmax reads freed memory, and list_move_tail()
links cl into the freed agg->active. When agg_dequeue() returns,
qfq_dequeue() still updates the same aggregate without a check:

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

This is the budget field from the KASAN report in the commit message.

The commit message says "After each child peek, restart qfq_dequeue() if
either q->in_serv_agg or cl->agg changed". Does the qdisc_peek_len() call
in agg_dequeue() need the same treatment?

A codel or fq_codel child that is emptied during the peek doesn't seem to
hit this. In that case qdisc_peek_len() returns 0, cl->deficit < 0u is
always false, and list_move_tail() is never reached. The netem path above
is enough to trigger it.

> @@ -1274,9 +1283,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);

[Severity: Low]
This isn't a bug, but could the commit message say more about the trigger
and about this behaviour change?

It only says "a child peek can re-enter QFQ and free it". It doesn't
describe the topology: a netem child whose qdisc_peek_dequeued() runs
netem_dequeue(), feeding a duplicating netem that calls rootq->enqueue().
It also doesn't give the free path (qfq_enqueue() -> qfq_change_agg() ->
qfq_rm_from_agg() -> qfq_destroy_agg()) or a reproducer.

The added cl_is_active(cl) recheck also skips the DRR requeue when the
nested path has deactivated the class. The commit message only says "Also
use cl->agg after the child peek in qfq_enqueue()".

Describing the mechanism would make it easier to check that every child
peek or dequeue site that can re-enter QFQ is covered. One example is the
qdisc_peek_len() call in agg_dequeue() mentioned above.

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