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

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

[Cong Wang]

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>
---
 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;
-- 
2.27.0

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

[Petr Machata]

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;

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

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 25 Jul 2020 13:17:07 -0700

> 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>

Applied, thank you.