[Patch net] net_sched: fix error path in red_init()

[Patch net] net_sched: fix error path in red_init()

[Cong Wang]

When ->init() fails, ->destroy() is called to clean up.
So it is unnecessary to clean up in red_init(), and it
would cause some refcount underflow.

Fixes: aee9caa03fc3 ("net: sched: sch_red: Add qevents "early_drop" and "mark"")
Reported-and-tested-by: syzbot+b33c1cb0a30ebdc8a5f9@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+e5ea5f8a3ecfd4427a1c@syzkaller.appspotmail.com
Cc: Petr Machata <petrm@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_red.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index deac82f3ad7b..e89fab6ccb34 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -353,23 +353,11 @@ static int red_init(struct Qdisc *sch, struct nlattr *opt,
 			      FLOW_BLOCK_BINDER_TYPE_RED_EARLY_DROP,
 			      tb[TCA_RED_EARLY_DROP_BLOCK], extack);
 	if (err)
-		goto err_early_drop_init;
-
-	err = tcf_qevent_init(&q->qe_mark, sch,
-			      FLOW_BLOCK_BINDER_TYPE_RED_MARK,
-			      tb[TCA_RED_MARK_BLOCK], extack);
-	if (err)
-		goto err_mark_init;
-
-	return 0;
+		return err;
 
-err_mark_init:
-	tcf_qevent_destroy(&q->qe_early_drop, sch);
-err_early_drop_init:
-	del_timer_sync(&q->adapt_timer);
-	red_offload(sch, false);
-	qdisc_put(q->qdisc);
-	return err;
+	return tcf_qevent_init(&q->qe_mark, sch,
+			       FLOW_BLOCK_BINDER_TYPE_RED_MARK,
+			       tb[TCA_RED_MARK_BLOCK], extack);
 }
 
 static int red_change(struct Qdisc *sch, struct nlattr *opt,
-- 
2.28.0

Re: [Patch net] net_sched: fix error path in red_init()

[Petr Machata]

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

> When ->init() fails, ->destroy() is called to clean up.
> So it is unnecessary to clean up in red_init(), and it
> would cause some refcount underflow.

Hmm, yeah, qdisc_put() would get called twice. A surprising API, the
init needs to make sure to always bring the qdisc into destroyable
state. But qevents are like that after kzalloc, so the fix looks
correct.

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

Thanks!

Re: [Patch net] net_sched: fix error path in red_init()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 27 Aug 2020 10:40:41 -0700

> When ->init() fails, ->destroy() is called to clean up.
> So it is unnecessary to clean up in red_init(), and it
> would cause some refcount underflow.
> 
> Fixes: aee9caa03fc3 ("net: sched: sch_red: Add qevents "early_drop" and "mark"")
> Reported-and-tested-by: syzbot+b33c1cb0a30ebdc8a5f9@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+e5ea5f8a3ecfd4427a1c@syzkaller.appspotmail.com
> Cc: Petr Machata <petrm@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thank you.