Re: [RFC PATCH] livepatch: allow removal of a disabled patch

[Miroslav Benes]

On Thu, 5 May 2016, Josh Poimboeuf wrote:

> On Thu, May 05, 2016 at 10:28:12AM +0200, Miroslav Benes wrote:
> > I think it boils down to the following problem.
> > 
> > 1. CONFIG_DEBUG_KOBJECT_RELEASE=y
> > 
> > 2. we have dynamic kobjects, so there is a pointer in klp_patch to struct 
> > kobject
> > 
> > 3. it is allocated during klp_init_patch() and all is fine
> > 
> > 4. now we want to remove the patch module. It is disabled and module_put()
> > is called. User calls rmmod on the module.
> > 
> > 5. klp_unregister_patch() is called in __exit method.
> > 
> > 6. klp_free_patch() is called.
> > 
> > 7. kobject_put(patch->kobj) is called.
> > 
> > ...now it gets interesting...
> > 
> > 8. among others kobject_cleanup() is scheduled as a delayed work (this is 
> > important).
> > 
> > 9. there is no completion, so kobject_put returns and the module goes 
> > away.
> > 
> > 10. someone calls patch enabled_store attribute (for example). They can 
> > because kobject_cleanup() has not been called yet. It is delayed 
> > scheduled.
> > 
> > ...crash...
> 
> But what exactly causes the crash?  In enabled_store() we can see that
> the patch isn't in the list, so we can return -EINVAL.

Ok, bad example. Take enabled_show() instead. It could be fixed in the 
same way, but I am not sure it is the right thing to do. It does not scale 
because the problem is elsewhere.

Anyway, it is (even if theoretically) there in my opinion and we 
have two options.

1. We could forget about CONFIG_DEBUG_KOBJECT_RELEASE and all is ok 
without completion and regardless of dynamic/static kobject allocation.

2. We introduce completion and we are ok even with 
CONFIG_DEBUG_KOBJECT_RELEASE=y and again regardless of dynamic/static 
kobject allocation.

Miroslav