Re: [Patch net-next] net_sched: initialize timer earlier in red_init()

[Petr Machata <petrm@mellanox.com>]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> When red_init() fails, red_destroy() is called to clean up.
> If the timer is not initialized yet, del_timer_sync() will
> complain. So we have to move timer_setup() before any failure.
>
> Reported-and-tested-by: syzbot+6e95a4fabf88dc217145@syzkaller.appspotmail.com
> Fixes: aee9caa03fc3 ("net: sched: sch_red: Add qevents "early_drop" and "mark"")
> Cc: Petr Machata <petrm@mellanox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Ah, correct, this used to be done in red_init() not only before calling
red_change(), but actually before doing anything that can fail. Thanks
for fixing this.

Reviewed-by: Petr Machata <petrm@mellanox.com>

> ---
>  net/sched/sch_red.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> index 4cc0ad0b1189..deac82f3ad7b 100644
> --- a/net/sched/sch_red.c
> +++ b/net/sched/sch_red.c
> @@ -333,6 +333,10 @@ static int red_init(struct Qdisc *sch, struct nlattr *opt,
>  	struct nlattr *tb[TCA_RED_MAX + 1];
>  	int err;
>
> +	q->qdisc = &noop_qdisc;
> +	q->sch = sch;
> +	timer_setup(&q->adapt_timer, red_adaptative_timer, 0);
> +
>  	if (!opt)
>  		return -EINVAL;
>
> @@ -341,10 +345,6 @@ static int red_init(struct Qdisc *sch, struct nlattr *opt,
>  	if (err < 0)
>  		return err;
>
> -	q->qdisc = &noop_qdisc;
> -	q->sch = sch;
> -	timer_setup(&q->adapt_timer, red_adaptative_timer, 0);
> -
>  	err = __red_change(sch, tb, extack);
>  	if (err)
>  		return err;