Re: [PATCH v2 2/3] livepatch: add atomic replace

[Jason Baron]

On 09/12/2017 01:35 AM, Petr Mladek wrote:
> On Mon 2017-09-11 23:46:28, Jason Baron wrote:
>> On 09/11/2017 09:53 AM, Petr Mladek wrote:
>>> On Wed 2017-08-30 17:38:44, Jason Baron wrote:
>>>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>>> index 8d3df55..ee6d18b 100644
>>>> --- a/include/linux/livepatch.h
>>>> +++ b/include/linux/livepatch.h
>>>> @@ -119,10 +121,12 @@ struct klp_object {
>>>>   * @mod:	reference to the live patch module
>>>>   * @objs:	object entries for kernel objects to be patched
>>>>   * @immediate:  patch all funcs immediately, bypassing safety mechanisms
>>>> + * @replace:	replace all funcs, reverting functions that are no longer patched
>>>>   * @list:	list node for global list of registered patches
>>>>   * @kobj:	kobject for sysfs resources
>>>>   * @obj_list:	head of list for dynamically allocated struct klp_object
>>>>   * @enabled:	the patch is enabled (but operation may be incomplete)
>>>> + * @replaced:	the patch has been replaced an can not be re-enabled
>>>
>>> I finally understand why you do this. I forgot that even disabled
>>> patches are still in klp_patch list.
>>>
>>> This makes some sense for patches that fix different bugs and
>>> touch the same function. They should be applied and enabled
>>> in the right order because a later patch for the same function
>>> must be based on the code from the previous one.
>>>
>>> It makes less sense for cummulative patches that we use in kGraft.
>>> There basically every patch has the "replace" flag set. If
>>> we enable one patch it simply replaces any other one. Then
>>> ordering is not important. Each patch is standalone and
>>> consistent on its own.
>>>
>>>
>>> I see two problems with your approach. One is cosmetic.
>>> The names of the two flags replace/replaced are too similar.
>>> It is quite prone for mistakes ;-)
>>>
>>> Second, we could not remove module where any function is patched
>>> usign the "immediate" flag. But people might want to revert
>>> to this patch if the last one does not work as expected.
>>> After all the main purpose of livepatching is to fix
>>> problems without a system reboot. Therefore we should
>>> allow to enable the replaced patches even without
>>> removing the module.
>>>
>>
>> If the livepatch has the 'replace' bit set and not the 'immediate' bit,
>> then I believe that we could remove *all* types of previous livepatches
>> even if they have the 'immediate' flag set. That is, because of the
>> consistency model, we are sure that we are running only functions from
>> the cumulative replace patch.
> 
> You are partly right. The catch is if a function was previously
> patched using the immediate flag and the function is not longer
> patched by the new cumulative patch with the 'replace' flag.
> Then we need to create "no_op" for the function and the 'no_op'
> must inherit the immediate flag set. Then the consistency model
> does not guarantee that the code from the older patch is not running
> and we could not allow to remove the older patch.
> 

Agreed - the replace patch should 'inherit' the immediate flag. This
raises the question, what if say two patches have the immediate flag set
differently for the same function? This would be unlikely since
presumably we would set it the same way for all patches. In this case, I
think a reasonable thing to do would be to 'inherit' the immediate flag
from the immediately proceeding patch, since that's the one we are
reverting from.

> 
>> So I think it may be worth making a distinction between patches that
>> have 'replace' bit set and the immediate bit *unset*, and those that
>> have the 'replace' bit set and the immediate bit *set*. So it seems to
>> me that this patchset could just reject 'replace' patches that have the
>> 'immediate' flag set, and treat the 'immediate' flag being set as
>> separate todo item if there is interest?
> 
> I would do it the other way. If we apply a livepatch with both "replace"
> and "immediate" flag set then I would avoid calling module_put()
> for all the replaced patches. IMHO, it is a useful feature and
> this is safe and easy.
> 
> We could later add an optimization that would allow to remove
> patches that were not immediate and where all the functions
> were replaced without the immediate flag as well.

ok.

> 
> I guess that you know this. But just for sure note that there
> are two possibilities. The "immediate" flag might be set either
> for the entire patch (struct klp_patch) or for particular
> functions (struct klp_func).
> 
> 
>> Then, the 'replace' patch with 'immediate' not set, conceptually does a
>> 'disable' and an 'unregister' of all previous live patches, such that
>> they could be removed (rmmod) and re-inserted.
> 
> Yes, if you consider also the flags used by "no_op" functions.
> 
>

Agreed. I'm thinking it might make sense to split this out as separate
patch.


>>> One solution would be to remove the replaced patches from
>>> klp_patch list. And enforce stacking the following way
>>> in __klp_enable_patch():
>>>
>>> 	/*
>>> 	 * Enforce stacking: only the first disabled patch can be
>>> 	 * enabled. The only exception is a patch that atomically
>>> 	 * replaces all other patches and was disabled by
>>> 	 * another patch of this type.
>>> 	 */
>>> 	if (list_empty(&patch->list)) {
>>> 		if (!patch->replace)
>>> 			return -EBUSY;
>>> 		list_add_tail(&patch->list, &klp_patches);
>>> 	} else if (patch->list.prev != &klp_patches &&
>>> 		   !list_prev_entry(patch, list)->enabled) {
>>> 		return -EBUSY;
>>> 	}
>>>
>>> Well, I would add this support later in a separate patch.
>>> We would need to (re)generate the no_op stuff in __klp_enable_patch()
>>> for this. Also we would need to make sure that this patch
>>> could not longer get enabled once klp_unregister_patch()
>>> is called.
>>>
>>> For now, I would just remove the replaced patches from
>>> klp_patches list and refuse enabling patches that are
>>> not in the list. I mean to use a check of
>>> list_empty(&patch->list) instead of the extra
>>> "replaced" variable.
>>
>> Ok, that makes sense to me as a way to remove 'replaced'.
>>
>>>
>>>
>>>>   * @finish:	for waiting till it is safe to remove the patch module
>>>>   */
>>>>  struct klp_patch {
>>>> @@ -130,12 +134,14 @@ struct klp_patch {
>>>>  	struct module *mod;
>>>>  	struct klp_object *objs;
>>>>  	bool immediate;
>>>> +	bool replace;
>>>>  
>>>>  	/* internal */
>>>>  	struct list_head list;
>>>>  	struct kobject kobj;
>>>>  	struct list_head obj_list;
>>>>  	bool enabled;
>>>> +	bool replaced;
>>>>  	struct completion finish;
>>>>  };
>>>>  
>>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>>> index 6004be3..21cecc5 100644
>>>> --- a/kernel/livepatch/core.c
>>>> +++ b/kernel/livepatch/core.c
>>>> @@ -600,13 +603,38 @@ static void klp_free_patch(struct klp_patch *patch)
>>>>  		list_del(&patch->list);
>>>>  }
>>>>  
>>>> -static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>> +void klp_patch_free_no_ops(struct klp_patch *patch)
>>>> +{
>>>> +	struct klp_object *obj, *tmp_obj;
>>>> +	struct klp_func *func, *tmp_func;
>>>> +
>>>> +	klp_for_each_object(patch, obj) {
>>>> +		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
>>>> +					 func_entry) {
>>>> +			list_del_init(&func->func_entry);
>>>> +			kobject_put(&func->kobj);
>>>> +			kfree(func->old_name);
>>>> +			kfree(func);
>>>
>>> kobject_put() is asynchronous. The structure should be freed using
>>> the kobject release method.
>>>
>>> The question is how secure this should be. We might want to block
>>> other operations with the patch until all the release methods
>>> are called. But this might be tricky as there is not a single
>>> root kobject that would get destroyed at the end.
>>>
>>> A crazy solution would be to define a global atomic counter.
>>> It might get increased with each kobject_put() call and
>>> descreased in each release method. Then we could wait
>>> until it gets zero.
>>>
>>> It should be safe to wait with klp_mutex taken. Note that this
>>> is not possible with patch->kobj() where the "the enable"
>>> attribute takes the mutex as well, see
>>> enabled_store() in kernel/livepatch/core.c.
>>
>> Thanks for pointing this out - it looks like the livepatch code uses
>> wait_for_completion() with special kobj callbacks. Perhaps, there could
>> be a nop callback, but I'd have to look at this more closely...
> 
> The completion is usable for the root kobject but you do not free
> it here. It might be pretty complicated if you need separate
> completion for all the freed kobjects.
> 
> A solution might be a global atomic counter and a waitqueue.
> Feel free to ask for more details.
> 

So the issue is that the user might access some of the klp_* data
structures via /sysfs after we have already freed them? If so, this
seems to be a common kernel pattern:

kobject_del(kobj);
kobject_put(kobj);


> 
>>>> +		}
>>>> +		INIT_LIST_HEAD(&obj->func_list);
>>>> +	}
>>>> +	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, obj_entry) {
>>>> +		list_del_init(&obj->obj_entry);
>>>> +		kobject_put(&obj->kobj);
>>>> +		kfree(obj->name);
>>>> +		kfree(obj);
>>>
>>> Same here.
>>>
>>>> +	}
>>>> +	INIT_LIST_HEAD(&patch->obj_list);
>>>> +}
>>>> +
> 
> 
>>>> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
>>>> index b004a1f..d5e620e 100644
>>>> --- a/kernel/livepatch/transition.c
>>>> +++ b/kernel/livepatch/transition.c
>>>> @@ -70,6 +72,7 @@ static void klp_synchronize_transition(void)
>>>>  	schedule_on_each_cpu(klp_sync);
>>>>  }
>>>>  
>>>> +
>>>
>>> extra empty line?
>>>
>>>>  /*
>>>>   * The transition to the target patch state is complete.  Clean up the data
>>>>   * structures.
>>>> @@ -81,14 +84,39 @@ static void klp_complete_transition(void)
>>>>  	struct task_struct *g, *task;
>>>>  	unsigned int cpu;
>>>>  	bool immediate_func = false;
>>>> +	bool no_op = false;
>>>> +	struct klp_patch *prev_patch;
>>>> +
>>>> +	/*
>>>> +	 * for replace patches, we disable all previous patches, and replace
>>>> +	 * the dynamic no-op functions by removing the ftrace hook. After
>>>> +	 * klp_synchronize_transition() is called its safe to free the
>>>> +	 * the dynamic no-op functions, done by klp_patch_free_no_ops()
>>>> +	 */
>>>> +	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
>>>> +		prev_patch = klp_transition_patch;
>>>> +		while (prev_patch->list.prev != &klp_patches) {
>>>> +			prev_patch = list_prev_entry(prev_patch, list);
>>>
>>> I wonder if the following code might be more obvious:
>>>
>>> 	list_for_each_entry(old_patch, &klp_patches, list) {
>>> 		if (old_patch = klp_transition_patch)
>>> 			continue;
>>>
>>>> +			if (prev_patch->enabled) {
>>>> +				klp_unpatch_objects(prev_patch, false);
>>>> +				prev_patch->enabled = false;
>>>
>>> I suggested to do this in __klp_disable_patch() in the last review.
>>> But we need something else.
>>>
>>> The fact is that patch->enable is normally manipulated at the
>>> beginning of the transition, see __klp_enable_patch()
>>> and __klp_disable_patch(). Therefore we are kind of
>>> inconsistent when we manipulate it here for the replaced
>>> patchses.>
>>
>> ok, the suggestion is that for the non-immediate replace we do a
>> 'special' disable here, once we know that the transition patch has been
>> successfully enabled.
>>
>>> But we must keep the patches enabled during the entire
>>> transition. Othewise, klp_module_coming() and klp_module_going()
>>> would not patch/unpatch the objects correctly.
>>
>> I think the patch does, its only when its been successfully enabled that
>> it goes and disables the previous patches.
>>
>>>
>>> In fact, I think that we should set
>>> klp_transition_patch->enabled = false in klp_complete_transition()
>>> instead of in __klp_disable_patch(). I mean that flag should reflect
>>> whether the ftrace handlers see the patch or not. Therefore it
>>> should be always true during the transition. Then we would not
>>> need the extra check for klp_transition_patch in coming/going
>>> handlers. Well, this should be done in separate patch.
>>> I could prepare it if you want.
>>
>> I don't quite see why this is needed for this patch series
> 
> Yes, please, see the last two sentences of my paragraph. I suggested
> to do this in a separate patch. I also offered to provide it.
> 
>> I think the added nops should be handled the way all the other
>> functions are...
> 
> It is not about no_ops. It is about the "enabled" flag
> in struct klp_patch. This patchset handles this flag
> differently for replaced patches than we do it for
> a normally disabled ones.
> 
> Well, it is a detail that I wanted to point out.
> The handling of replaced patches looks fine after all.
> Feel free to handle the "enabled" flag as you do now.
> 
> 
>>
>>>
>>>
>>>> +				prev_patch->replaced = true;
>>>> +				module_put(prev_patch->mod);
>>>
>>> In each case, we could do this only when the related no_op
>>> functions were not applied using "immediate" flag.
>>
>> yes.
>>
>>>
>>> I am not 100% sure that it is really necessary. But I would feel
>>> more comfortable when we do this after klp_synchronize_transition();
>>> so that any ftrace handler could not see the related
>>> functions on the stack.
>>
>> Which bit are you referring to here?
> 
> I mean that we should call module_put() for all the replaced patches
> later in klp_complete_transition(). In particular, we should do so
> after we call klp_synchronize_transition() for
> klp_target_state == KLP_PATCHED variant.
>

ok.

> 
>>>
>>>> +			}
>>>> +		}
>>>> +		klp_unpatch_objects(klp_transition_patch, true);
>>>> +		no_op = true;
>>>> +	}
>>>>  
>>>>  	if (klp_target_state == KLP_UNPATCHED) {
>>>>  		/*
>>>>  		 * All tasks have transitioned to KLP_UNPATCHED so we can now
>>>>  		 * remove the new functions from the func_stack.
>>>>  		 */
>>>> -		klp_unpatch_objects(klp_transition_patch);
>>>> +		klp_unpatch_objects(klp_transition_patch, false);
>>>
>>> This code patch made me think about a situation when
>>> the "enable" operation was reverted during the transition.
>>> Then the target state is KLP_UNPATCHED and no_op stuff
>>> is there.
>>>
>>> Well, in this case, we need to keep the no_op stuff
>>> until either the the patch is enabled egain and the
>>> transition finishes or we need to remove/free it
>>> klp_unregister_patch().
>>
>> In the case that klp_cancel_transition() calls klp_complete_transition()
>> and we remove the nop functions, we also remove all the regular
>> 'functions', which should be re-added on the function stacks if the
>> patch is re-enabled. So I think this is fine.
> 
> But we do not remove no_op when klp_target_state == KLP_PATCHED.
> And this might never happen when we call klp_reverse_transition()
> in the middle of the transition.
> 
> I see two solutions:
> 
> Either we could postpone generating the no_op functions
> until __klp_enable_patch() call and always free
> them in klp_finish_transition().
> 
> Or we need to check if some no_op functions are still
> there when __klp_unregister_patch() is called and free
> them there.
> 
> 
> In the long term, we would need the first solution
> because it would allow to re-enable the replaced
> patches. But it will be more complicated to reuse
> the existing code, especially the init functions.
> 
> Feel free to keep it easy and implement
> the 2nd possibility in this patch(set).
> 

Indeed I took the 2nd possibility in this patch already - see the call
to klp_patch_free_no_ops() in klp_unregister_patch().

Thanks,

-Jason