netdev - Re: [Patch net] sch_sfb: fix a crash in sfb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=whO37+O-mohvMODnD57ppCsK3Bcv8oHzSBvmwJbsT54cA@mail.gmail.com>
Date:   Thu, 12 Sep 2019 11:31:06 +0100
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        syzbot <syzbot+d5870a903591faaca4ae@...kaller.appspotmail.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net] sch_sfb: fix a crash in sfb_destroy()

On Thu, Sep 12, 2019 at 2:10 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.dumazet@...il.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