| Message ID | 20190911183445.32547-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] sch_sfb: fix a crash in sfb_destroy() | expand |
On 9/11/19 8:34 PM, Cong Wang wrote: > When tcf_block_get() fails in sfb_init(), q->qdisc is still a NULL > pointer which leads to a crash in sfb_destroy(). > > Linus suggested three solutions for this problem, the simplest fix > is just moving the noop_qdisc assignment before tcf_block_get() > so that qdisc_put() would become a nop. > > Fixes: 6529eaba33f0 ("net: sched: introduce tcf block infractructure") > Reported-by: syzbot+d5870a903591faaca4ae@syzkaller.appspotmail.com > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > 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_sfb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c > index 1dff8506a715..db1c8eb521a2 100644 > --- a/net/sched/sch_sfb.c > +++ b/net/sched/sch_sfb.c > @@ -552,11 +552,11 @@ static int sfb_init(struct Qdisc *sch, struct nlattr *opt, > struct sfb_sched_data *q = qdisc_priv(sch); > int err; > > + q->qdisc = &noop_qdisc; > + > err = tcf_block_get(&q->block, &q->filter_list, sch, extack); > if (err) > return err; > - > - q->qdisc = &noop_qdisc; > return sfb_change(sch, opt, extack); > } > > It seems a similar fix would be needed in net/sched/sch_dsmark.c ?
On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > It seems a similar fix would be needed in net/sched/sch_dsmark.c ? > Yeah, or just add a NULL check in dsmark_destroy(). Anyway, I will send a separate patch for it. Thanks.
On Thu, Sep 12, 2019 at 2:10 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > It seems a similar fix would be needed in net/sched/sch_dsmark.c ? > > > > Yeah, or just add a NULL check in dsmark_destroy(). Well, this was why one of my suggestions was to just make "qdisc_put()" be happy with a NULL pointer (or even an ERR_PTR()). That would have fixed not just sfb, but also dsmark with a single patch. We tend to have that kind of pattern in a lot of places, where we can free unallocated structures (end ERR_PTR() pointers) withour errors, so that destroy_fn(alloc_fn()); is defined to always work, even when alloc_fn() returns NULL or an error. That, and allowing the "it was never allocated at all" case (as long as it's initialized to NULL, of course) tends to make various error cases simpler. The obvious one is kfree(kmalloc()), of course, but we've done it in other places too. So you find things like void i2c_unregister_device(struct i2c_client *client) { if (IS_ERR_OR_NULL(client)) return; in various subsystems and drivers. So one of my suggestions was to just do that to qdisc_put(). It depends on what you want to do, of course. Do you want to make sure each user is being very careful? Or do you want to make the interfaces easy to use without _having_ to be careful? There are arguments both ways, but we've tended to move more towards a "easy to use" model than the "be careful" one. Linus
From: Linus Torvalds <torvalds@linux-foundation.org> Date: Thu, 12 Sep 2019 11:31:06 +0100 > It depends on what you want to do, of course. Do you want to make sure > each user is being very careful? Or do you want to make the interfaces > easy to use without _having_ to be careful? There are arguments both > ways, but we've tended to move more towards a "easy to use" model than > the "be careful" one. Yes, I think allowing NULL or error pointers on free/destroy makes sense.
On Thu, Sep 12, 2019 at 3:31 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, Sep 12, 2019 at 2:10 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > It seems a similar fix would be needed in net/sched/sch_dsmark.c ? > > > > > > > Yeah, or just add a NULL check in dsmark_destroy(). > > Well, this was why one of my suggestions was to just make > "qdisc_put()" be happy with a NULL pointer (or even an ERR_PTR()). > > That would have fixed not just sfb, but also dsmark with a single patch. Sure, I don't have any preference here, just want to find a minimum fix for -stable. I will send v2. Thanks.
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c index 1dff8506a715..db1c8eb521a2 100644 --- a/net/sched/sch_sfb.c +++ b/net/sched/sch_sfb.c @@ -552,11 +552,11 @@ static int sfb_init(struct Qdisc *sch, struct nlattr *opt, struct sfb_sched_data *q = qdisc_priv(sch); int err; + q->qdisc = &noop_qdisc; + err = tcf_block_get(&q->block, &q->filter_list, sch, extack); if (err) return err; - - q->qdisc = &noop_qdisc; return sfb_change(sch, opt, extack); }
When tcf_block_get() fails in sfb_init(), q->qdisc is still a NULL pointer which leads to a crash in sfb_destroy(). Linus suggested three solutions for this problem, the simplest fix is just moving the noop_qdisc assignment before tcf_block_get() so that qdisc_put() would become a nop. Fixes: 6529eaba33f0 ("net: sched: introduce tcf block infractructure") Reported-by: syzbot+d5870a903591faaca4ae@syzkaller.appspotmail.com Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> 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_sfb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)