linux-kernel - Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue

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: <CA+55aFwg+j60ZFi7t7kVXbobCT4XHoiZP_w6+Cbm+AeU1ytfNQ@mail.gmail.com>
Date:   Sat, 30 Jun 2018 12:45:21 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Joel Stanley <joel@....id.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue_dir

On Thu, Jun 28, 2018 at 7:21 PM Benjamin Herrenschmidt
<benh@...nel.crashing.org> wrote:
>
> Under some circumstances (such as when using kobject debugging)
> a gluedir whose kref is 0 might remain in the class kset for
> a long time. The reason is that we don't actively remove glue
> dirs when they become empty, but instead rely on the implicit
> removal done by kobject_release(), which can happen some amount
> of time after the last kobject_put().
>
> Using such a dead object is a bad idea and will lead to warnings
> and crashes.

So with the other patch in mind, here's my comments on this one. Pick
one of two scenarios:

 (a) it's obviously correct.

     We obviously can *not* take an object with a zero refcount,
because it is already been scheduled for kobject_cleanup(), and
incrementing the refcount is simply fundamentally wrong, because
incrementing the refcount won't unschedule the deletion of the object.

 (b) the patch is wrong, and our "kobject_get()" should cancel the
kobject_cleanup() instead.

There are problems with both of the above cases.

The "patch is obviously correct" case leads to another issue: why
would kobject_get() _ever_ succeed on an object wioth a zero refcount?
IOW, why do we have kobject_get() vs kobject_get_unless_zero() in the
first place? It is *never* ok to get an kobject with a zero refcount
because of the above "it's already scheduled for deletion" issue.

The (b) case sounds nice, and would actually fix the problem that
patch 2/2 was tryihng to address, and would make
CONFIG_DEBUG_KOBJECT_RELEASE work.

HOWEVER. It's completely untenable in reality - it's a nightmare from
a locking standpoint, because kref_put() literally depends not on
locking, but on the exclusive "went to zero".

So I think (b) is practically not acceptable. Which means that (a) is
the right reaction, and "kobject_get()" on an object with a zero
refcount is _always_ wrong.

But that says that "yes, the patch is obviously correct", but it also
says "the patch should be pointless, because kobject_get() should just
_always_ have the semantics of "kobject_get_unless_zero()", and the
latter shouldn't even exist.

Greg? When would it possibly be valid to do "kobject_get()" on a zero
refcount object? I don't see it. But this is all very much your code.

               Linus