* [PATCH] livepatch: Prevent to enable uninitialized patch @ 2015-05-11 2:57 Minfei Huang 2015-05-11 12:02 ` Miroslav Benes 0 siblings, 1 reply; 6+ messages in thread From: Minfei Huang @ 2015-05-11 2:57 UTC (permalink / raw) To: jpoimboe, sjenning, jkosina, vojtech Cc: live-patching, linux-kernel, Minfei Huang From: Minfei Huang <minfei.huang@hotmail.com> The previous patches can be applied, while the corresponding module is loaded. Now the code cannot handle correct behavior to deal with the case that the patch fail to be initialized when the module is being loaded. In general, the patch will do relocation (if necessary) and obtain/verify function address before we start to enable patch. But we can still trigger to enable the patch (disable the patch firstly, then enable it), although the patch fail to be initialized in the function klp_module_notify_coming. To fix it, we can make obj->mod to NULL, if the object fails to be initialized. Signed-off-by: Minfei Huang <minfei.huang@hotmail.com> --- kernel/livepatch/core.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 284e269..4bbcdda 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch) } EXPORT_SYMBOL_GPL(klp_register_patch); -static void klp_module_notify_coming(struct klp_patch *patch, +static int klp_module_notify_coming(struct klp_patch *patch, struct klp_object *obj) { struct module *pmod = patch->mod; struct module *mod = obj->mod; - int ret; + int ret = 0; ret = klp_init_object_loaded(patch, obj); if (ret) - goto err; + goto out; if (patch->state == KLP_DISABLED) - return; + goto out; pr_notice("applying patch '%s' to loading module '%s'\n", pmod->name, mod->name); ret = klp_enable_object(obj); - if (!ret) - return; -err: - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", - pmod->name, mod->name, ret); +out: + if (ret) + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", + pmod->name, mod->name, ret); + return ret; } static void klp_module_notify_going(struct klp_patch *patch, @@ -930,6 +930,7 @@ disabled: static int klp_module_notify(struct notifier_block *nb, unsigned long action, void *data) { + int ret = 0; struct module *mod = data; struct klp_patch *patch; struct klp_object *obj; @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, if (action == MODULE_STATE_COMING) { obj->mod = mod; - klp_module_notify_coming(patch, obj); + ret = klp_module_notify_coming(patch, obj); + if (ret) + obj->mod = NULL; } else /* MODULE_STATE_GOING */ klp_module_notify_going(patch, obj); -- 2.2.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: Prevent to enable uninitialized patch 2015-05-11 2:57 [PATCH] livepatch: Prevent to enable uninitialized patch Minfei Huang @ 2015-05-11 12:02 ` Miroslav Benes 2015-05-11 12:55 ` Minfei Huang 0 siblings, 1 reply; 6+ messages in thread From: Miroslav Benes @ 2015-05-11 12:02 UTC (permalink / raw) To: Minfei Huang Cc: jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel, Minfei Huang On Mon, 11 May 2015, Minfei Huang wrote: > From: Minfei Huang <minfei.huang@hotmail.com> > > The previous patches can be applied, while the corresponding module is > loaded. Now the code cannot handle correct behavior to deal with the > case that the patch fail to be initialized when the module is being > loaded. > > In general, the patch will do relocation (if necessary) and > obtain/verify function address before we start to enable patch. But we > can still trigger to enable the patch (disable the patch firstly, then > enable it), although the patch fail to be initialized in the function > klp_module_notify_coming. > > To fix it, we can make obj->mod to NULL, if the object fails to be > initialized. > > Signed-off-by: Minfei Huang <minfei.huang@hotmail.com> Hi, just to be sure, is the following what makes you worried? The module comes and our notifier is called. We verify that it needs to be patched and we call klp_module_notify_coming where the object (for this module) is enabled. But that could fail somewhere and we print warning to the log (pr_warn). Now, you can disable and enable patch, during which the object for this very module is enabled again. And it could fail again. Is this correct? Do you want to prevent printing of the warning again and again to the log? It could happen that the first enablement could fail because of something which would not be true for the second try. In such case the module would not be patched with your fix (it would be skipped in __klp_enable_patch loop). It is possible that I do not understand the changelog and the patch correctly, so please shed some light on this if necessary... Thanks, Miroslav > --- > kernel/livepatch/core.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 284e269..4bbcdda 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch) > } > EXPORT_SYMBOL_GPL(klp_register_patch); > > -static void klp_module_notify_coming(struct klp_patch *patch, > +static int klp_module_notify_coming(struct klp_patch *patch, > struct klp_object *obj) > { > struct module *pmod = patch->mod; > struct module *mod = obj->mod; > - int ret; > + int ret = 0; > > ret = klp_init_object_loaded(patch, obj); > if (ret) > - goto err; > + goto out; > > if (patch->state == KLP_DISABLED) > - return; > + goto out; > > pr_notice("applying patch '%s' to loading module '%s'\n", > pmod->name, mod->name); > > ret = klp_enable_object(obj); > - if (!ret) > - return; > > -err: > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > - pmod->name, mod->name, ret); > +out: > + if (ret) > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > + pmod->name, mod->name, ret); > + return ret; > } > > static void klp_module_notify_going(struct klp_patch *patch, > @@ -930,6 +930,7 @@ disabled: > static int klp_module_notify(struct notifier_block *nb, unsigned long action, > void *data) > { > + int ret = 0; > struct module *mod = data; > struct klp_patch *patch; > struct klp_object *obj; > @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > if (action == MODULE_STATE_COMING) { > obj->mod = mod; > - klp_module_notify_coming(patch, obj); > + ret = klp_module_notify_coming(patch, obj); > + if (ret) > + obj->mod = NULL; > } else /* MODULE_STATE_GOING */ > klp_module_notify_going(patch, obj); > > -- > 2.2.2 > > -- > To unsubscribe from this list: send the line "unsubscribe live-patching" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Miroslav Benes SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: Prevent to enable uninitialized patch 2015-05-11 12:02 ` Miroslav Benes @ 2015-05-11 12:55 ` Minfei Huang 2015-05-11 22:49 ` Jiri Kosina 2015-05-12 8:25 ` Miroslav Benes 0 siblings, 2 replies; 6+ messages in thread From: Minfei Huang @ 2015-05-11 12:55 UTC (permalink / raw) To: Miroslav Benes Cc: Minfei Huang, jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel On 05/11/15 at 02:02P, Miroslav Benes wrote: > On Mon, 11 May 2015, Minfei Huang wrote: > > > From: Minfei Huang <minfei.huang@hotmail.com> > > > > The previous patches can be applied, while the corresponding module is > > loaded. Now the code cannot handle correct behavior to deal with the > > case that the patch fail to be initialized when the module is being > > loaded. > > > > In general, the patch will do relocation (if necessary) and > > obtain/verify function address before we start to enable patch. But we > > can still trigger to enable the patch (disable the patch firstly, then > > enable it), although the patch fail to be initialized in the function > > klp_module_notify_coming. > > > > To fix it, we can make obj->mod to NULL, if the object fails to be > > initialized. > > Hi, Miroslav. This patch is used to prevent the patch to be enabled. I will use the code to explain what I want to show you. 1) Patched a patch to fix the issue for module A. 2) livepatch will try to enable the patch, while the corresponding module is loaded ( call klp_module_notify_coming ) 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever the result of klp_module_notify_coming is. 4) livepatch may fail to call the klp_init_object_loaded or klp_enable_object 5) klp_module_notify_coming returns 6) For the userspace, we can enable the patch again ( disable the patch firstly, then enable the patch from the sysfs ) 7) In order to enable the patch, livepatch will call __klp_enable_patch 8) we can pass the limitation (klp_is_object_loaded), because the value of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 ) 9) the patch may be applied, although the patch is not initialized, if the value of func->old_addr is not NULL >From the above description, we can see the uninitialized patch ( the patch should be initialized by the klp_init_object_loaded in general ) can be applied to the kernel. Thanks Minfei > > Signed-off-by: Minfei Huang <minfei.huang@hotmail.com> > > Hi, > > just to be sure, is the following what makes you worried? > > The module comes and our notifier is called. We verify that it needs to be > patched and we call klp_module_notify_coming where the object (for this > module) is enabled. But that could fail somewhere and we print warning to > the log (pr_warn). Now, you can disable and enable patch, during which the > object for this very module is enabled again. And it could fail again. > > Is this correct? Do you want to prevent printing of the warning again and > again to the log? > > It could happen that the first enablement could fail because of something > which would not be true for the second try. In such case the module would > not be patched with your fix (it would be skipped in __klp_enable_patch > loop). > > It is possible that I do not understand the changelog and the patch > correctly, so please shed some light on this if necessary... > > Thanks, > Miroslav > > > --- > > kernel/livepatch/core.c | 23 +++++++++++++---------- > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > index 284e269..4bbcdda 100644 > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch) > > } > > EXPORT_SYMBOL_GPL(klp_register_patch); > > > > -static void klp_module_notify_coming(struct klp_patch *patch, > > +static int klp_module_notify_coming(struct klp_patch *patch, > > struct klp_object *obj) > > { > > struct module *pmod = patch->mod; > > struct module *mod = obj->mod; > > - int ret; > > + int ret = 0; > > > > ret = klp_init_object_loaded(patch, obj); > > if (ret) > > - goto err; > > + goto out; > > > > if (patch->state == KLP_DISABLED) > > - return; > > + goto out; > > > > pr_notice("applying patch '%s' to loading module '%s'\n", > > pmod->name, mod->name); > > > > ret = klp_enable_object(obj); > > - if (!ret) > > - return; > > > > -err: > > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > - pmod->name, mod->name, ret); > > +out: > > + if (ret) > > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > + pmod->name, mod->name, ret); > > + return ret; > > } > > > > static void klp_module_notify_going(struct klp_patch *patch, > > @@ -930,6 +930,7 @@ disabled: > > static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > void *data) > > { > > + int ret = 0; > > struct module *mod = data; > > struct klp_patch *patch; > > struct klp_object *obj; > > @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > > > if (action == MODULE_STATE_COMING) { > > obj->mod = mod; > > - klp_module_notify_coming(patch, obj); > > + ret = klp_module_notify_coming(patch, obj); > > + if (ret) > > + obj->mod = NULL; > > } else /* MODULE_STATE_GOING */ > > klp_module_notify_going(patch, obj); > > > > -- > > 2.2.2 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe live-patching" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Miroslav Benes > SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: Prevent to enable uninitialized patch 2015-05-11 12:55 ` Minfei Huang @ 2015-05-11 22:49 ` Jiri Kosina 2015-05-12 7:24 ` Minfei Huang 2015-05-12 8:25 ` Miroslav Benes 1 sibling, 1 reply; 6+ messages in thread From: Jiri Kosina @ 2015-05-11 22:49 UTC (permalink / raw) To: Minfei Huang Cc: Miroslav Benes, Minfei Huang, jpoimboe, sjenning, Vojtech Pavlik, live-patching, linux-kernel On Mon, 11 May 2015, Minfei Huang wrote: > 1) Patched a patch to fix the issue for module A. > 2) livepatch will try to enable the patch, while the corresponding > module is loaded ( call klp_module_notify_coming ) > 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever > the result of klp_module_notify_coming is. > 4) livepatch may fail to call the klp_init_object_loaded or > klp_enable_object > 5) klp_module_notify_coming returns > > 6) For the userspace, we can enable the patch again ( disable the patch > firstly, then enable the patch from the sysfs ) > 7) In order to enable the patch, livepatch will call __klp_enable_patch > 8) we can pass the limitation (klp_is_object_loaded), because the value > of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 ) > 9) the patch may be applied, although the patch is not initialized, if > the value of func->old_addr is not NULL > > From the above description, we can see the uninitialized patch ( the > patch should be initialized by the klp_init_object_loaded in general ) > can be applied to the kernel. This indeed looks like a valid breakage scenario. Could you please resend v2 of this patch with much more detailed description in the changelog? (i.e. some reformulated variation on the text above). Your original submission didn't describe the problem your patch is fixing at all. Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: Prevent to enable uninitialized patch 2015-05-11 22:49 ` Jiri Kosina @ 2015-05-12 7:24 ` Minfei Huang 0 siblings, 0 replies; 6+ messages in thread From: Minfei Huang @ 2015-05-12 7:24 UTC (permalink / raw) To: Jiri Kosina Cc: Miroslav Benes, Minfei Huang, jpoimboe, sjenning, Vojtech Pavlik, live-patching, linux-kernel On 05/12/15 at 12:49P, Jiri Kosina wrote: > On Mon, 11 May 2015, Minfei Huang wrote: > > > 1) Patched a patch to fix the issue for module A. > > 2) livepatch will try to enable the patch, while the corresponding > > module is loaded ( call klp_module_notify_coming ) > > 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever > > the result of klp_module_notify_coming is. > > 4) livepatch may fail to call the klp_init_object_loaded or > > klp_enable_object > > 5) klp_module_notify_coming returns > > > > 6) For the userspace, we can enable the patch again ( disable the patch > > firstly, then enable the patch from the sysfs ) > > 7) In order to enable the patch, livepatch will call __klp_enable_patch > > 8) we can pass the limitation (klp_is_object_loaded), because the value > > of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 ) > > 9) the patch may be applied, although the patch is not initialized, if > > the value of func->old_addr is not NULL > > > > From the above description, we can see the uninitialized patch ( the > > patch should be initialized by the klp_init_object_loaded in general ) > > can be applied to the kernel. > > This indeed looks like a valid breakage scenario. > > Could you please resend v2 of this patch with much more detailed > description in the changelog? (i.e. some reformulated variation on the > text above). Your original submission didn't describe the problem your > patch is fixing at all. > > Thanks, Thanks for your review. I will repost a new patch. Thanks Minfei > > -- > Jiri Kosina > SUSE Labs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] livepatch: Prevent to enable uninitialized patch 2015-05-11 12:55 ` Minfei Huang 2015-05-11 22:49 ` Jiri Kosina @ 2015-05-12 8:25 ` Miroslav Benes 1 sibling, 0 replies; 6+ messages in thread From: Miroslav Benes @ 2015-05-12 8:25 UTC (permalink / raw) To: Minfei Huang Cc: Minfei Huang, jpoimboe, sjenning, jkosina, vojtech, live-patching, linux-kernel On Mon, 11 May 2015, Minfei Huang wrote: > On 05/11/15 at 02:02P, Miroslav Benes wrote: > > On Mon, 11 May 2015, Minfei Huang wrote: > > > > > From: Minfei Huang <minfei.huang@hotmail.com> > > > > > > The previous patches can be applied, while the corresponding module is > > > loaded. Now the code cannot handle correct behavior to deal with the > > > case that the patch fail to be initialized when the module is being > > > loaded. > > > > > > In general, the patch will do relocation (if necessary) and > > > obtain/verify function address before we start to enable patch. But we > > > can still trigger to enable the patch (disable the patch firstly, then > > > enable it), although the patch fail to be initialized in the function > > > klp_module_notify_coming. > > > > > > To fix it, we can make obj->mod to NULL, if the object fails to be > > > initialized. > > > > > Hi, Miroslav. > > This patch is used to prevent the patch to be enabled. I will use the > code to explain what I want to show you. > > 1) Patched a patch to fix the issue for module A. > 2) livepatch will try to enable the patch, while the corresponding > module is loaded ( call klp_module_notify_coming ) > 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever > the result of klp_module_notify_coming is. > 4) livepatch may fail to call the klp_init_object_loaded or > klp_enable_object > 5) klp_module_notify_coming returns > > 6) For the userspace, we can enable the patch again ( disable the patch > firstly, then enable the patch from the sysfs ) > 7) In order to enable the patch, livepatch will call __klp_enable_patch > 8) we can pass the limitation (klp_is_object_loaded), because the value > of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 ) > 9) the patch may be applied, although the patch is not initialized, if > the value of func->old_addr is not NULL > > >From the above description, we can see the uninitialized patch ( the > patch should be initialized by the klp_init_object_loaded in general ) > can be applied to the kernel. Hi, thanks for an explanation. This is really valid. Concerning 9), func->old_addr should not be used for the modules (I know we had some discussion about that). So it could happen that func->old_addr is not initialized as you describe and the user gets warning from klp_enable_func. This should be fixed as you proposed. Please resend as Jiri requested Thanks Miroslav > > Thanks > Minfei > > > > Signed-off-by: Minfei Huang <minfei.huang@hotmail.com> > > > > Hi, > > > > just to be sure, is the following what makes you worried? > > > > The module comes and our notifier is called. We verify that it needs to be > > patched and we call klp_module_notify_coming where the object (for this > > module) is enabled. But that could fail somewhere and we print warning to > > the log (pr_warn). Now, you can disable and enable patch, during which the > > object for this very module is enabled again. And it could fail again. > > > > Is this correct? Do you want to prevent printing of the warning again and > > again to the log? > > > > It could happen that the first enablement could fail because of something > > which would not be true for the second try. In such case the module would > > not be patched with your fix (it would be skipped in __klp_enable_patch > > loop). > > > > It is possible that I do not understand the changelog and the patch > > correctly, so please shed some light on this if necessary... > > > > Thanks, > > Miroslav > > > > > --- > > > kernel/livepatch/core.c | 23 +++++++++++++---------- > > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > > index 284e269..4bbcdda 100644 > > > --- a/kernel/livepatch/core.c > > > +++ b/kernel/livepatch/core.c > > > @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch) > > > } > > > EXPORT_SYMBOL_GPL(klp_register_patch); > > > > > > -static void klp_module_notify_coming(struct klp_patch *patch, > > > +static int klp_module_notify_coming(struct klp_patch *patch, > > > struct klp_object *obj) > > > { > > > struct module *pmod = patch->mod; > > > struct module *mod = obj->mod; > > > - int ret; > > > + int ret = 0; > > > > > > ret = klp_init_object_loaded(patch, obj); > > > if (ret) > > > - goto err; > > > + goto out; > > > > > > if (patch->state == KLP_DISABLED) > > > - return; > > > + goto out; > > > > > > pr_notice("applying patch '%s' to loading module '%s'\n", > > > pmod->name, mod->name); > > > > > > ret = klp_enable_object(obj); > > > - if (!ret) > > > - return; > > > > > > -err: > > > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > > - pmod->name, mod->name, ret); > > > +out: > > > + if (ret) > > > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > > + pmod->name, mod->name, ret); > > > + return ret; > > > } > > > > > > static void klp_module_notify_going(struct klp_patch *patch, > > > @@ -930,6 +930,7 @@ disabled: > > > static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > > void *data) > > > { > > > + int ret = 0; > > > struct module *mod = data; > > > struct klp_patch *patch; > > > struct klp_object *obj; > > > @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > > > > > if (action == MODULE_STATE_COMING) { > > > obj->mod = mod; > > > - klp_module_notify_coming(patch, obj); > > > + ret = klp_module_notify_coming(patch, obj); > > > + if (ret) > > > + obj->mod = NULL; > > > } else /* MODULE_STATE_GOING */ > > > klp_module_notify_going(patch, obj); > > > > > > -- > > > 2.2.2 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe live-patching" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > > -- > > Miroslav Benes > > SUSE Labs > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-12 8:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-11 2:57 [PATCH] livepatch: Prevent to enable uninitialized patch Minfei Huang 2015-05-11 12:02 ` Miroslav Benes 2015-05-11 12:55 ` Minfei Huang 2015-05-11 22:49 ` Jiri Kosina 2015-05-12 7:24 ` Minfei Huang 2015-05-12 8:25 ` Miroslav Benes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®