Messages in this thread | | | From | Linus Torvalds <> | Date | Sat, 30 Jun 2018 12:45:21 -0700 | 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@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.
Linus
| |