Messages in this thread | | | Date | Tue, 12 Sep 2017 18:05:44 -0400 | From | Joe Lawrence <> | Subject | Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks |
| |
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.
-- Joe
| |