[Patch net] sch_sfb: fix a crash in sfb_destroy()

[Patch net] sch_sfb: fix a crash in sfb_destroy()

[Cong Wang]

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);
 }
 
-- 
2.21.0

Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

[Eric Dumazet]

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 ?

Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

[Cong Wang]

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.

Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

[Linus Torvalds]

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

Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

[David Miller]

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.

Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

[Cong Wang]

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.