Re: [PATCH v4 3/3] livepatch: add atomic replace

[Petr Mladek <pmladek@suse.com>]

On Thu 2017-10-12 17:12:29, Jason Baron wrote:
> Sometimes we would like to revert a particular fix. This is currently
> This is not easy because we want to keep all other fixes active and we
> could revert only the last applied patch.
> 
> One solution would be to apply new patch that implemented all
> the reverted functions like in the original code. It would work
> as expected but there will be unnecessary redirections. In addition,
> it would also require knowing which functions need to be reverted at
> build time.
> 
> A better solution would be to say that a new patch replaces
> an older one. This might be complicated if we want to replace
> a particular patch. But it is rather easy when a new cummulative
> patch replaces all others.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f53eed5..d1c7a06 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -283,8 +301,21 @@ static int klp_write_object_relocations(struct module *pmod,
>  	return ret;
>  }
>  
> +atomic_t klp_nop_release_count;
> +static DECLARE_WAIT_QUEUE_HEAD(klp_nop_release_wait);
> +
>  static void klp_kobj_release_object(struct kobject *kobj)
>  {
> +	struct klp_object *obj;
> +
> +	obj = container_of(kobj, struct klp_object, kobj);
> +	/* Free dynamically allocated object */
> +	if (!obj->funcs) {
> +		kfree(obj->name);
> +		kfree(obj);
> +		atomic_dec(&klp_nop_release_count);
> +		wake_up(&klp_nop_release_wait);

I would slightly optimize this by

		if (atomic_dec_and_test((&klp_nop_release_count))
			wake_up(&klp_nop_release_wait);

> +	}
>  }
>  
>  static struct kobj_type klp_ktype_object = {
> @@ -294,6 +325,16 @@ static struct kobj_type klp_ktype_object = {
>  
>  static void klp_kobj_release_func(struct kobject *kobj)
>  {
> +	struct klp_func *func;
> +
> +	func = container_of(kobj, struct klp_func, kobj);
> +	/* Free dynamically allocated functions */
> +	if (!func->new_func) {
> +		kfree(func->old_name);
> +		kfree(func);
> +		atomic_dec(&klp_nop_release_count);
> +		wake_up(&klp_nop_release_wait);

Same here

		if (atomic_dec_and_test((&klp_nop_release_count))
			wake_up(&klp_nop_release_wait);


> +	}
>  }
>  
>  static struct kobj_type klp_ktype_func = {
> @@ -436,8 +480,14 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	if (ret)
>  		return ret;
>  
> -	klp_for_each_func(obj, func) {
> -		ret = klp_init_func(obj, func);
> +	list_add(&obj->obj_entry, &patch->obj_list);
> +	INIT_LIST_HEAD(&obj->func_list);
> +
> +	if (nop)
> +		return 0;

Ah, this is something that I wanted to avoid. It makes the code
very hard to read and maintain. It forces us to duplicate
some code in klp_alloc_nop_func(). I think that I complained
about this in v2 already.

I understand that you actually kept it because of me.
It is related to the possibility to re-enable released
patches :-(

The klp_init_*() stuff is called from __klp_enable_patch()
for the "nop" functions now. And it has already been called
for the statically defined structures in klp_register_patch().
Therefore we need to avoid calling it twice for the static
structures.

One solution would be to do these operations for the statically
defined structures in __klp_enable_patch() as well. But this
would mean a big redesign of the code.

Another solution would be to give up on the idea that the replaced
patches might be re-enabled without re-loading. I am afraid
that this the only reasonable approach. It will help to avoid
also the extra klp_replaced_patches list. All this will help to
make the code much easier.

I am really sorry that I asked you to do this exercise and
support the patch re-enablement. It looked like a good idea.
I did not expect that it would be that complicated.

I stop reviewing this patch because it will look quite
different again. I will only keep some random comments
around that I added before finding this main design flaw.

Thanks a lot for the hard work. v4 looks much better than
v2 in many ways. I think that we are going on the right way.

> +
> +	klp_for_each_func_static(obj, func) {
> +		ret = klp_init_func(obj, func, false);
>  		if (ret)
>  			goto free;
>  	}
> @@ -456,6 +506,226 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	return ret;
>  }

[...]

> +/* Add 'nop' functions which simply return to the caller to run
> + * the original function. The 'nop' functions are added to a
> + * patch to facilitate a 'replace' mode
> + */
> +static int klp_add_nops(struct klp_patch *patch)
> +{
> +	struct klp_patch *old_patch;
> +	struct klp_object *old_obj;
> +	int err = 0;
> +
> +	if (!patch->replace)
> +		return 0;

It would be more sane if this returns -EINVAL and
we call the following in __klp_enable_patch().

	if (patch->replace) {
		ret = klp_add_nops(patch);
		...
	}

Otherwise, people might wonder why klp_add_nops() is always called.
They would need to look how it is implemented just to realize that
it is a NOP when patch->replace is not set.

> +
> +	list_for_each_entry(old_patch, &klp_patches, list) {
> +		if (old_patch == patch)
> +			break;
> +
> +		klp_for_each_object(old_patch, old_obj) {
> +			err = klp_add_nop_object(patch, old_obj);
> +			if (err) {
> +				klp_remove_nops(patch);
> +				return err;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int __klp_disable_patch(struct klp_patch *patch)
>  {
>  	if (klp_transition_patch)
> @@ -527,10 +798,22 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	if (WARN_ON(patch->enabled))
>  		return -EINVAL;
>  
> +	if (klp_is_patch_replaced(patch)) {
> +		list_move_tail(&patch->list, &klp_patches);
> +		replaced = true;
> +	}
> +
>  	/* enforce stacking: only the first disabled patch can be enabled */
>  	if (patch->list.prev != &klp_patches &&
> -	    !list_prev_entry(patch, list)->enabled)
> -		return -EBUSY;
> +	    !list_prev_entry(patch, list)->enabled) {
> +		ret = -EBUSY;
> +		goto cleanup_list;
> +	}
> +
> +	/* setup nops */

Please, remove the comment. It does not add much information.

> +	ret = klp_add_nops(patch);
> +	if (ret)
> +		goto cleanup_list;

It will be more obvious when we do:

	if (patch->replace) {
		ret = klp_add_nops(patch);
		if (ret)
			goto cleanup_list;
	}

>  
>  	/*
>  	 * A reference is taken on the patch module to prevent it from being

Best Regards,
Petr