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

[Josh Poimboeuf]

On Thu, May 05, 2016 at 04:25:48PM +0200, Miroslav Benes wrote:
> 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.

I would disagree with the statement that the dynamic kobject doesn't
scale.  We would just need a helper function to get from a kobject to
its klp_patch.

In fact, to me it seems like the right way to do it.  It doesn't make
sense for the code which creates the kobject to be different from the
code which initializes it.  It's slightly out of context, but
kobject.txt does say:

  "Code which creates a kobject must, of course, initialize that object."

I view the completion as a hack to compensate for the fact that we're
abusing the kobject interface.  And so it makes sense to me that
CONFIG_DEBUG_KOBJECT_RELEASE would cause problems, because we're using
kobjects in the wrong way.

So in my view, the two options are:

1. Convert the kobject to dynamic as I described.

2. Change the klp_register() interface so that klp_patch gets allocated
   in livepatch code.

I'd be curious to hear what others think.

-- 
Josh