Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

[Miroslav Benes]

On Wed, 13 Sep 2017, Joe Lawrence wrote:

> Hi Miroslav,

Hi,

sorry for the late response. I'm also travelling now and we have SUSECon 
conference next week, so just a quick answer. It looks ok at first glance, 
but I need to take a proper look.

> I worked out the code that I posted earlier today and I think this could
> address the multiple-patch module_coming() issue you pointed out.
> 
> Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> callbacks" patchset, so it includes unpatching callbacks.  I can easily
> strip those out (and remove the additional debugging pr_'s) and make
> this a stand-alone patch that would apply before the callback patchset.

I think this would be better. Strip callbacks out and send this either 
separately (and base callbacks patch set on this), or make it 1/n of the 
series.

> See the test case below.
> 
> -- Joe
> 
> Test X
> ------
> 
> Multiple livepatches targeting the same klp_objects may be loaded at
> the same time.  If a target module loads and any of the livepatch's
> pre-patch callbacks fail, then the module is not allowed to load.
> Furthermore, any livepatches that that did succeed will be reverted
> (only the incoming module / klp_object) and their pre/post-unpatch
> callbacks executed.
> 
>   - load livepatch
>   - load livepatch2
>   - load livepatch3
>   - setup livepatch3 pre-patch return of -ENODEV
>   - load target module (should fail)
>   - disable livepatch3
>   - disable livepatch2
>   - disable livepatch
>   - unload livepatch3
>   - unload livepatch2
>   - unload livepatch
> 
> 
> Load three livepatches, each target a livepatch_callbacks_mod module and
> vmlinux:
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo.ko 
>   [   26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
>   [   26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
>   [   26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
>   [   26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
>   [   27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
>   [   27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo2.ko 
>   [   29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
>   [   29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
>   [   29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
>   [   30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
>   [   30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
> 
>   % insmod samples/livepatch/livepatch-callbacks-demo3.ko 
>   [   32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
>   [   32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
>   [   32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
>   [   33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
>   [   33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
> 
> Setup the third livepatch to fail its pre-patch callback when the target
> module is loaded:
> 
>   % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
> 
> Load the target module:
> 
>   % insmod samples/livepatch/livepatch-callbacks-mod.ko 
> 
> The first livepatch pre-patch callback succeeds, the klp_object is
> patched, and its post-patch callback is executed:
> 
>   [   38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
>   [   38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
>   [   38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> Likewise for the second livepatch:
> 
>   [   38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
>   [   38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
>   [   38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> But the third livepatch fails its pre-patch callback:
> 
>   [   38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
>   [   38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
> 
> We refuse to load the target module:
> 
>   [   38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
> 
> So we double back and unpatch (including pre-unpatch and post-unpatch
> callbacks) the first livepatch, then the second:
> 
>   [   38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
>   [   38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>   [   38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
>   [   38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> 
> Finally the module loader reports an error:
> 
>   [   38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
> 
> Clean it all up:
> 
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
>   [   41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
>   [   41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
>   [   42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
>   [   42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
>   
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
>   [   47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
>   [   47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
>   [   48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
>   [   48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> 
>   % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
>   [   53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
>   [   53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
>   [   54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
>   [   54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
> 
>   % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
>   % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
>   % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> 
> 
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> 
> >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@xxxxxxxxxx>
> Date: Wed, 13 Sep 2017 16:51:13 -0400
> Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
> 
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
> 
> 	list_for_each_entry(patch, &klp_patches, list) {
> 		klp_for_each_object(patch, obj) {
> 
> which means that if one of the kernel objects fail to patch for whatever
> reason, klp_module_coming()'s error path should double back and unpatch
> any previous kernel object that was patched for a previous patch.
> 
> Reported-by: Miroslav Benes <mbenes@xxxxxxx>
> Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx>
> ---
>  kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index aca62c4b8616..7f5192618cc8 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
>  				goto err;
>  			}
>  
> +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> @@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
>  	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
>  		patch->mod->name, obj->mod->name, obj->mod->name);
>  	mod->klp_alive = false;
> -	klp_free_object_loaded(obj);
> +
> +	/*
> +	 * Run back through the patch list and unpatch any klp_object that
> +	 * was patched before hitting an error above.
> +	 */
> +
> +	list_for_each_entry(patch, &klp_patches, list) {

I think it would be safer to use 
list_for_each_entry_{continue,from}_reverse() iterator (probably 
_continue_reverse(), because the current patch failed). That would unpatch 
the objects in the correct order (see your test case above) and it is 
also an optimization because you'd process only those patches which were 
walked through during the first loop.

> +
> +		if (!patch->enabled || patch == klp_transition_patch)
> +			continue;

Is the second part with klp_transition_patch correct? Yes, we need to skip 
disabled patches. No question about that. But klp_transition_patch seems 
odd. It is true, that (if I am not mistaken) klp_transition_patch is the 
last patch in patches list which is relevant (because we cannot 
enable/disable any random patch in the list). If that failed to patch, 
you wouldn't need to worry about it anyway, because you need to process 
previous patches only. Am I missing something? So I think it can stay, 
yes. But I'd like to understand it.

Miroslav

> +		klp_for_each_object(patch, obj) {
> +
> +			if (!obj->patched || !klp_is_module(obj) ||
> +			    strcmp(obj->name, mod->name))
> +				continue;
> +
> +			klp_pre_unpatch_callback(obj);
> +pr_err("JL: klp_unpatch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +			klp_unpatch_object(obj);
> +			klp_post_unpatch_callback(obj);
> +			klp_free_object_loaded(obj);
> +
> +			break;
> +		}
> +	}
> +
>  	mutex_unlock(&klp_mutex);
>  
>  	return ret;
> -- 
> 2.7.5
>