lkml.org 
[lkml]   [2018]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] drivers: core: Don't try to use a dead glue_dir
On Sat, Jun 30, 2018 at 12:45:21PM -0700, Linus Torvalds wrote:
> On Thu, Jun 28, 2018 at 7:21 PM Benjamin Herrenschmidt
> <benh@kernel.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.

No, kobject_get() should never happen on a 0 refcount object. That
being said, the code does allow it, so if things are messed up, it will
happen. I think that change happened when the switch to refcount_t
occured, before then we would WARN_ON() if that ever happened. I should
go fix that up, and restore that old behavior, so that syzbot starts
complaining loudly when stuff like that hits.

So I hate using kobject_get_unless_zero(), and resisted ever adding it
to the tree as it shows a bad locking/tree situation as you point out
here. But for some reason, the block developers seemed to insist they
needed it, and so it is in the tree for them. I don't want it to spread
if at all possible, which makes me want to reject this patch as this
should be "a case that can never be hit".

thanks,

greg k-h

\
 
 \ /
  Last update: 2018-07-07 18:49    [W:0.098 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site