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

[Petr Mladek]

On Thu 2017-09-28 15:15:00, Joe Lawrence wrote:
> 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 fails to patch,
> klp_module_coming()'s error path needs to unpatch and cleanup any kernel
> objects that were already patched by a previous patch.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..3d457e0bbe26 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -830,6 +830,42 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> +/*
> + * Revert patches (up to a given limit) to objects belonging to a given
> + * kernel module.  After unpatching such objects, the function also

"objects belonging to a module" sounds strange. In the livepatch
world: module == object. I would say something like:

	/* Remove parts of patches that touch a given kernel module.
	 * The list of proceed patches might be limited. When limit is
	 * NULL, all patches will be handled.
	 */


> + * frees them.  When limit is NULL, all patches to the given module will
> + * be reverted.
> + */
> +static void klp_cleanup_module_objects_limited(struct module *mod,
> +					       struct klp_patch *limit)
> +{

Same here with: "module_objects". What about?

     klp_cleanup_module_patches_limited(struct module *mod,
					struct klp_patch *limit)


> +	struct klp_patch *patch;
> +	struct klp_object *obj;
> +
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		if (patch == limit)
> +			break;
> +
> +		klp_for_each_object(patch, obj) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
> +
> +			/*
> +			 * Only unpatch the module if the patch is enabled or
> +			 * is in transition.
> +			 */
> +			if (patch->enabled || patch == klp_transition_patch) {
> +				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +					  patch->mod->name, obj->mod->name);
> +				klp_unpatch_object(obj);
> +			}
> +
> +			klp_free_object_loaded(obj);
> +			break;

Heh, I was about to say OK for this patch. But then I noticed
this strange "break". It was already in the original code and
I wondered if it was OK.

It is OK. It is an optimization because this code handles
only one struct object in each patch. But it made me
to complain about the wording above ;-)

> +		}
> +	}
> +}
> +
>  int klp_module_coming(struct module *mod)
>  {
>  	int ret;

Otherwise the patch looks fine to me. Thanks a lot for handling
it separately.

Best Regards,
Petr