lkml.org 
[lkml]   [2017]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2017-09-13 00:06    [W:0.135 / U:3.348 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site