| Message ID | 20200827174041.13300-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net_sched: fix error path in red_init() | expand |
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!
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.
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,
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(-)