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

[Jason Baron]

On 10/17/2017 10:18 AM, Petr Mladek wrote:
> 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 :-(

hmmm, I actually think that it is not necessary - although I didn't try
removing it. I was concerned that we would call klp_init_func() for
functions that were already initialized. However, this only called right
after creating a brand new object, so there are no associated functions,
and thus I think the above early return is not necessary. I can try
removing it.

> 
> 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.
> 

It seems like the consensus is to abandon re-enable and go back to
rmmod/insmod for revert. I do like the rmmod/insmod revert path in that
it keeps module list smaller. However, there are cases where that is not
possible, but it sounds like in the interest of keeping the code simpler
for the common case, revert would not be possible in those cases, and we
would simply have to add a new livepatch module in those cases to fix
things.

Thanks,

-Jason