On Tue, 12 Sep 2017, Josh Poimboeuf wrote:
> 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.
Yes, that's the scenario.
> > 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.
Yes, you'd need to loop back, unpatch everything and call post-unpatch
callbacks too. Probably too much for this patch set, so we can deal with
the problem later.
> 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 :-)
Unfortunately, the problem is there even without Joe's callbacks. If it
was only a problem of callbacks, I'd go along with you. I see two options.
1. we'll fix this for klp_patch_object(). Then callbacks' problem would be
simple to solve, because the infrastructure would be already there.
2. we'll remove any error handling from klp_coming_module and we'll allow
target modules to load even with a patching failure. This doesn't seem to
be the right approach...
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
