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

[Petr Mladek]

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.


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

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