Message-ID: <b3961ef5-9f42-d692-07ab-18a4639d3aa2@akamai.com>
Date:   Thu, 14 Sep 2017 08:31:24 -0700
From:   Jason Baron <jbaron@...mai.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
        jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
        mbenes@...e.cz, gregkh@...uxfoundation.org
Subject: Re: [PATCH v2 2/3] livepatch: add atomic replace



On 09/14/2017 06:32 AM, Petr Mladek wrote:
> On Tue 2017-09-12 23:47:32, Jason Baron wrote:
>>
>>
>> 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.
> 
> Good question. I would personally use immediate = false if there
> is a mismatch. It might block the transition but it will not break
> the consistency model. The consistency and stability is always
> more important. The user could always either revert the transition.
> Or there is going to be a way to force it.
> 
> IMHO, there are basically two approaches how to use
> the immediate flag:
> 
> Some people might enable it only when it is safe and needed
> to patch a function that might sleep for long. These people
> either want to be on the safe side. Or they want to remove
> disabled patches when possible. Your approach would be fine
> in this case.
> 
> Another group of people might always enable the immediate flag
> when it is safe. They would disable the immediate flag only when
> the patch does a semantic change. These people want to keep
> it easy and avoid potential troubles with a transition
> (complex code, might get blocked, ...). This is the reason
> to be conservative. The cumulative patch is going to replace
> all existing patches. If one of them needed the complex
> consistency model, we should use it as well.
> 

Ok, makes sense.

> 
>>>>>> 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?
> 
> yes
> 
>> if so, this seems to be a common kernel pattern:
>>
>> kobject_del(kobj);
>> kobject_put(kobj);
> 
> IMHO, this is used for other reason.
> 
> kobject_del() removes the object from the hierachy. Therefore
> it prevents new operations. But it does not wait for the exiting
> operations to finish. Therefore there still might be users that
> access the data even after this function finishes.

I looked into this further - and it does appear to wait until all
operations finish. In kernfs_drain() the code does:

wait_event(root->deactivate_waitq, atomic_read(&kn->active) ==
KN_DEACTIVATED_BIAS);

The call stack is:

kobject_del()
 sysfs_remove_dir()
  kernfs_remove()
   __kernfs_remove()
    kernfs_drain()

And __kernfs_remove() has already done a 'deactivate' prior:

/* prevent any new usage under @kn by deactivating all nodes */

pos = NULL;

while ((pos = kernfs_next_descendant_post(pos, kn)))

       if (kernfs_active(pos))

               atomic_add(KN_DEACTIVATED_BIAS, &pos->active);


So I *think* doing the kobject_del() first is sufficient. It may be
worth some better documented if that is the case...


Thanks,

-Jason


> 
> kobject_put() is enough if you do not mind how the object
> might be visible. It would remove it once the last reference
> on the object is removed. See the following code in
> kobject_cleanup.
> 
> 	/* remove from sysfs if the caller did not do it */
> 	if (kobj->state_in_sysfs) {
> 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
> 			 kobject_name(kobj), kobj);
> 		kobject_del(kobj);
> 	}
> 
> In each case, you must not free the data accessible from the kobject
> handlers until kobj_type->release method is called.
> 
> 
> IMHO, the solution is not that complicated after all.
> If you are able to distinguish statically and dynamically
> defined klp_func and klp_obj structures, you might
> just modify the existing kobject release methods:
> 
> 
> atomic_t klp_no_op_release_count;
> static DECLARE_WAIT_QUEUE_HEAD(klp_no_op_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_no_op_release_count);
> 		wake_up(&klp_no_op_release_wait);
> 	}
> }
> 
> static void klp_kobj_release_func(struct kobject *kobj)
> {
> 	struct klp_func *func;
> 
> 	obj = container_of(kobj, struct klp_func, kobj);
> 	/* Free dynamically allocated functions */
> 	if (!func->new_func) {
> 		kfree(func->old_name);
> 		kfree(func);
> 		atomic_dec(&klp_no_op_release_count);
> 		wake_up(&klp_no_op_release_wait);
> 	}
> }
> 
> 
> then we could have
> 
> 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);
> 			atomic_inc(&klp_no_op_release_count);
> 			kobject_put(&func->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);
> 		atomic_inc(&klp_no_op_release_count);
> 		kobject_put(&obj->kobj);
> 	}
> 	INIT_LIST_HEAD(&patch->obj_list);
> 
> 	wait_event(&klp_no_op_release_wait,
> 		   atomic_read(&klp_no_op_release_count) == 0);
> }
> 
> Add we should call this function also in klp_complete_transition()
> to avoid code duplication.
> 
> 
>>>>>> +		}
>>>>>> +		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
>>>
>>>>>
>>>>>> +			}
>>>>>> +		}
>>>>>> +		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().
> 
> Ah, I have missed this. Then we are on the safe side.
> 
> Best Regards,
> Petr
> 

Powered by blists - more mailing lists