Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> > On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> > >> @@ -871,13 +882,27 @@ int klp_module_coming(struct module *mod)
> > >>  			pr_notice("applying patch '%s' to loading module '%s'\n",
> > >>  				  patch->mod->name, obj->mod->name);
> > >>  
> > >> +			ret = klp_pre_patch_callback(obj);
> > >> +			if (ret) {
> > >> +				pr_warn("pre-patch callback failed for object '%s'\n",
> > >> +					obj->name);
> > >> +				goto err;
> > >> +			}
> > > 
> > > There is a problem here. We cycle through all enabled patches (or 
> > > klp_transition_patch) and call klp_pre_patch_callback() everytime an 
> > > enabled patch contains a patch for a coming module. Now, it can easily 
> > > happen that klp_pre_patch_callback() fails. And not the first one from the 
> > > first relevant patch, but the next one. In that case we need to call 
> > > klp_post_unpatch_callback() for all already processed relevant patches in 
> > > the error path.
> > 
> > Good test case, if I understand you correctly:
> > 
> >  - Load target modules mod1 and mod2
> >  - Load a livepatch that targets mod1 and mod2
> >    - pre-patch succeeds for mod1
> >    - pre-patch fails for mod2
> > 
> > and then we should:
> > 
> >  - NOT run post-patch or pre/post-unpatch handlers for mod2
> >  - NOT run post-patch or pre-unpatch handlers for mod1
> >  - do run post-unpatch handler for mod1
> >  - Refuse to load the livepatch
> > 
> > Does that sound right?
> 
> Erm, probably not...
> 
> > > Unfortunately, we need to do the same for klp_patch_object() below, 
> > > because there is the same problem and we missed it.
> > > 
> > >> +
> > >>  			ret = klp_patch_object(obj);
> > >>  			if (ret) {
> > >>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > >>  					patch->mod->name, obj->mod->name, ret);
> > >> +
> > >> +				if (patch != klp_transition_patch)
> > >> +					klp_post_unpatch_callback(obj);
> > >> +
> > >>  				goto err;
> > > 
> > > Here.
> > > 
> > > Could you do it as a part of the patch set (or send it separately), 
> > > please?
> 
> I've re-read this a few times, and I think I might have been originally
> off-base with what I thought you were concerned about.  But I think I
> grok it now: the problem you pointed out arises because
> klp_module_coming() iterates like so:
> 
>   for each klp_patch
>     for each kobj in klp_patch
> 
> which means that we may have made pre-patch callbacks and patched a
> given kobj for an earlier klp_patch that now fails for a later
> klp_patch.
> 
> What should be the defined behavior in this case?  I would expect that
> we need to unpatch all similar kobjs across klp_patches which have
> already been successfully patched.  In turn, their post-unpatch
> callbacks should be invoked.
> 
> If that's true, maybe this would make a better follow-on patch.

The rabbit hole seems to be getting deeper, is it really worth it?  I'd
rather we just make the pre-patch handler return void and be done with
it, as Joe originally proposed.

So far, allowing the pre-patch handler to halt patching is a purely
theoretical feature, nobody even knows if we need it yet, and whether
it's worth the pain.  So I'd vote to just simplify this mess and let
whoever wants the feature try to implement it :-)

-- 
Josh