Re: [PATCH net 1/2] net/sched: sch_cbs: Call qdisc_reset for child qdisc

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: sch_cbs: Call qdisc_reset for child qdisc
    
This commit introduces a reset callback for the CBS qdisc. The new
cbs_reset() function propagates the reset to the child qdisc to prevent
qlen and backlog accounting issues during resets.
    
> diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
> index 8c9a0400c8622..1efd72581614e 100644
> --- a/net/sched/sch_cbs.c
> +++ b/net/sched/sch_cbs.c
> @@ -243,6 +243,16 @@ static struct sk_buff *cbs_dequeue(struct Qdisc *sch)
>  	return q->dequeue(sch);
>  }
>  
> +static void cbs_reset(struct Qdisc *sch)
> +{
> +	struct cbs_sched_data *q = qdisc_priv(sch);
> +
> +	qdisc_reset(q->qdisc);

Can this cause a null pointer dereference during qdisc teardown?

If a CBS qdisc is created and cbs_init() fails (for example, if opt is null),
it returns -EINVAL leaving q->qdisc and q->watchdog uninitialized.

The cleanup path in qdisc_create_dflt() will call qdisc_put(), which
triggers __qdisc_destroy(). __qdisc_destroy() unconditionally calls
qdisc_reset() before calling ops->destroy().

Because q->qdisc is null here, calling qdisc_reset(q->qdisc) would
dereference a null pointer.

> +	qdisc_watchdog_cancel(&q->watchdog);

Similarly, could this trigger a crash by calling hrtimer_cancel() on an
uninitialized timer?

Does cbs_reset() need an early return if q->qdisc is null, similar to the
safety guard already present in cbs_destroy(), to prevent these issues?

> +	q->credits = 0;
> +	q->last = 0;
> +}
> +
-- 
pw-bot: cr