Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376872 > unrolled thread

Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched  states Chris J Arges <chris.j.arges@canonical.com> - 2016-04-12 16:50 +0200
    Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched  states Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-12 19:20 +0200
      Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched  states Chris J Arges <chris.j.arges@canonical.com> - 2016-04-12 19:40 +0200
        Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched  states Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-12 20:30 +0200

#1376872 — Re: [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states

On Fri, Mar 25, 2016 at 02:34:55PM -0500, Josh Poimboeuf wrote:
> Once we have a consistency model, patches and their objects will be
> enabled and disabled at different times.  For example, when a patch is
> disabled, its loaded objects' funcs can remain registered with ftrace
> indefinitely until the unpatching operation is complete and they're no
> longer in use.
> 
> It's less confusing if we give them different names: patches can be
> enabled or disabled; objects (and their funcs) can be patched or
> unpatched:
> 
> - Enabled means that a patch is logically enabled (but not necessarily
>   fully applied).
> 
> - Patched means that an object's funcs are registered with ftrace and
>   added to the klp_ops func stack.
> 
> Also, since these states are binary, represent them with booleans
> instead of ints.
> 

Josh,

Awesome work here first of all!

Looking through the patchset a bit I see the following bools:
- functions: patched, transitioning
- objects: patched
- patches: enabled

It seems this reflects the following states at a patch level:
	disabled - module inserted, not yet logically enabled
	enabled - logically enabled, but not all objects/functions patched
	transitioning - objects/functions are being applied or reverted
	patched - all objects/functions patched

However each object and function could have the same state and the parent object
just reflects the 'aggregate state'. For example if all funcs in an object are
patched then the object is also patched.

Perhaps we need more states (or maybe there will be more in the future), but
wouldn't this just be easier to have something like for each patch, object, and
function?

enum klp_state{
	KLP_DISABLED,
	KLP_ENABLED,
	KLP_TRANSITION,
	KLP_PATCHED,
}


I'm happy to help out too.

--chris


> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  include/linux/livepatch.h | 17 ++++-------
>  kernel/livepatch/core.c   | 72 +++++++++++++++++++++++------------------------
>  2 files changed, 42 insertions(+), 47 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index bd830d5..6d45dc7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -28,11 +28,6 @@
>  
>  #include <asm/livepatch.h>
>  
> -enum klp_state {
> -	KLP_DISABLED,
> -	KLP_ENABLED
> -};
> -
>  /**
>   * struct klp_func - function structure for live patching
>   * @old_name:	name of the function to be patched
> @@ -41,8 +36,8 @@ enum klp_state {
>   *		can be found (optional)
>   * @old_addr:	the address of the function being patched
>   * @kobj:	kobject for sysfs resources
> - * @state:	tracks function-level patch application state
>   * @stack_node:	list node for klp_ops func_stack list
> + * @patched:	the func has been added to the klp_ops list
>   */
>  struct klp_func {
>  	/* external */
> @@ -60,8 +55,8 @@ struct klp_func {
>  	/* internal */
>  	unsigned long old_addr;
>  	struct kobject kobj;
> -	enum klp_state state;
>  	struct list_head stack_node;
> +	bool patched;
>  };
>  
>  /**
> @@ -90,7 +85,7 @@ struct klp_reloc {
>   * @kobj:	kobject for sysfs resources
>   * @mod:	kernel module associated with the patched object
>   * 		(NULL for vmlinux)
> - * @state:	tracks object-level patch application state
> + * @patched:	the object's funcs have been add to the klp_ops list
>   */
>  struct klp_object {
>  	/* external */
> @@ -101,7 +96,7 @@ struct klp_object {
>  	/* internal */
>  	struct kobject kobj;
>  	struct module *mod;
> -	enum klp_state state;
> +	bool patched;
>  };
>  
>  /**
> @@ -110,7 +105,7 @@ struct klp_object {
>   * @objs:	object entries for kernel objects to be patched
>   * @list:	list node for global list of registered patches
>   * @kobj:	kobject for sysfs resources
> - * @state:	tracks patch-level application state
> + * @enabled:	the patch is enabled (but operation may be incomplete)
>   */
>  struct klp_patch {
>  	/* external */
> @@ -120,7 +115,7 @@ struct klp_patch {
>  	/* internal */
>  	struct list_head list;
>  	struct kobject kobj;
> -	enum klp_state state;
> +	bool enabled;
>  };
>  
>  #define klp_for_each_object(patch, obj) \
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d68fbf6..be1e106 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -298,11 +298,11 @@ unlock:
>  	rcu_read_unlock();
>  }
>  
> -static void klp_disable_func(struct klp_func *func)
> +static void klp_unpatch_func(struct klp_func *func)
>  {
>  	struct klp_ops *ops;
>  
> -	if (WARN_ON(func->state != KLP_ENABLED))
> +	if (WARN_ON(!func->patched))
>  		return;
>  	if (WARN_ON(!func->old_addr))
>  		return;
> @@ -322,10 +322,10 @@ static void klp_disable_func(struct klp_func *func)
>  		list_del_rcu(&func->stack_node);
>  	}
>  
> -	func->state = KLP_DISABLED;
> +	func->patched = false;
>  }
>  
> -static int klp_enable_func(struct klp_func *func)
> +static int klp_patch_func(struct klp_func *func)
>  {
>  	struct klp_ops *ops;
>  	int ret;
> @@ -333,7 +333,7 @@ static int klp_enable_func(struct klp_func *func)
>  	if (WARN_ON(!func->old_addr))
>  		return -EINVAL;
>  
> -	if (WARN_ON(func->state != KLP_DISABLED))
> +	if (WARN_ON(func->patched))
>  		return -EINVAL;
>  
>  	ops = klp_find_ops(func->old_addr);
> @@ -372,7 +372,7 @@ static int klp_enable_func(struct klp_func *func)
>  		list_add_rcu(&func->stack_node, &ops->func_stack);
>  	}
>  
> -	func->state = KLP_ENABLED;
> +	func->patched = true;
>  
>  	return 0;
>  
> @@ -383,36 +383,36 @@ err:
>  	return ret;
>  }
>  
> -static void klp_disable_object(struct klp_object *obj)
> +static void klp_unpatch_object(struct klp_object *obj)
>  {
>  	struct klp_func *func;
>  
>  	klp_for_each_func(obj, func)
> -		if (func->state == KLP_ENABLED)
> -			klp_disable_func(func);
> +		if (func->patched)
> +			klp_unpatch_func(func);
>  
> -	obj->state = KLP_DISABLED;
> +	obj->patched = false;
>  }
>  
> -static int klp_enable_object(struct klp_object *obj)
> +static int klp_patch_object(struct klp_object *obj)
>  {
>  	struct klp_func *func;
>  	int ret;
>  
> -	if (WARN_ON(obj->state != KLP_DISABLED))
> +	if (WARN_ON(obj->patched))
>  		return -EINVAL;
>  
>  	if (WARN_ON(!klp_is_object_loaded(obj)))
>  		return -EINVAL;
>  
>  	klp_for_each_func(obj, func) {
> -		ret = klp_enable_func(func);
> +		ret = klp_patch_func(func);
>  		if (ret) {
> -			klp_disable_object(obj);
> +			klp_unpatch_object(obj);
>  			return ret;
>  		}
>  	}
> -	obj->state = KLP_ENABLED;
> +	obj->patched = true;
>  
>  	return 0;
>  }
> @@ -423,17 +423,17 @@ static int __klp_disable_patch(struct klp_patch *patch)
>  
>  	/* enforce stacking: only the last enabled patch can be disabled */
>  	if (!list_is_last(&patch->list, &klp_patches) &&
> -	    list_next_entry(patch, list)->state == KLP_ENABLED)
> +	    list_next_entry(patch, list)->enabled)
>  		return -EBUSY;
>  
>  	pr_notice("disabling patch '%s'\n", patch->mod->name);
>  
>  	klp_for_each_object(patch, obj) {
> -		if (obj->state == KLP_ENABLED)
> -			klp_disable_object(obj);
> +		if (obj->patched)
> +			klp_unpatch_object(obj);
>  	}
>  
> -	patch->state = KLP_DISABLED;
> +	patch->enabled = false;
>  
>  	return 0;
>  }
> @@ -457,7 +457,7 @@ int klp_disable_patch(struct klp_patch *patch)
>  		goto err;
>  	}
>  
> -	if (patch->state == KLP_DISABLED) {
> +	if (!patch->enabled) {
>  		ret = -EINVAL;
>  		goto err;
>  	}
> @@ -475,12 +475,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	struct klp_object *obj;
>  	int ret;
>  
> -	if (WARN_ON(patch->state != KLP_DISABLED))
> +	if (WARN_ON(patch->enabled))
>  		return -EINVAL;
>  
>  	/* enforce stacking: only the first disabled patch can be enabled */
>  	if (patch->list.prev != &klp_patches &&
> -	    list_prev_entry(patch, list)->state == KLP_DISABLED)
> +	    !list_prev_entry(patch, list)->enabled)
>  		return -EBUSY;
>  
>  	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> @@ -492,12 +492,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  		if (!klp_is_object_loaded(obj))
>  			continue;
>  
> -		ret = klp_enable_object(obj);
> +		ret = klp_patch_object(obj);
>  		if (ret)
>  			goto unregister;
>  	}
>  
> -	patch->state = KLP_ENABLED;
> +	patch->enabled = true;
>  
>  	return 0;
>  
> @@ -555,20 +555,20 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	if (ret)
>  		return -EINVAL;
>  
> -	if (val != KLP_DISABLED && val != KLP_ENABLED)
> +	if (val > 1)
>  		return -EINVAL;
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
>  
>  	mutex_lock(&klp_mutex);
>  
> -	if (val == patch->state) {
> +	if (patch->enabled == val) {
>  		/* already in requested state */
>  		ret = -EINVAL;
>  		goto err;
>  	}
>  
> -	if (val == KLP_ENABLED) {
> +	if (val) {
>  		ret = __klp_enable_patch(patch);
>  		if (ret)
>  			goto err;
> @@ -593,7 +593,7 @@ static ssize_t enabled_show(struct kobject *kobj,
>  	struct klp_patch *patch;
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
> -	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->state);
> +	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
>  }
>  
>  static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
> @@ -684,7 +684,7 @@ static void klp_free_patch(struct klp_patch *patch)
>  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  {
>  	INIT_LIST_HEAD(&func->stack_node);
> -	func->state = KLP_DISABLED;
> +	func->patched = false;
>  
>  	/* The format for the sysfs directory is <function,sympos> where sympos
>  	 * is the nth occurrence of this symbol in kallsyms for the patched
> @@ -729,7 +729,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	if (!obj->funcs)
>  		return -EINVAL;
>  
> -	obj->state = KLP_DISABLED;
> +	obj->patched = false;
>  	obj->mod = NULL;
>  
>  	klp_find_object_module(obj);
> @@ -770,7 +770,7 @@ static int klp_init_patch(struct klp_patch *patch)
>  
>  	mutex_lock(&klp_mutex);
>  
> -	patch->state = KLP_DISABLED;
> +	patch->enabled = false;
>  
>  	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
>  				   klp_root_kobj, "%s", patch->mod->name);
> @@ -816,7 +816,7 @@ int klp_unregister_patch(struct klp_patch *patch)
>  		goto out;
>  	}
>  
> -	if (patch->state == KLP_ENABLED) {
> +	if (patch->enabled) {
>  		ret = -EBUSY;
>  		goto out;
>  	}
> @@ -897,13 +897,13 @@ int klp_module_coming(struct module *mod)
>  				goto err;
>  			}
>  
> -			if (patch->state == KLP_DISABLED)
> +			if (!patch->enabled)
>  				break;
>  
>  			pr_notice("applying patch '%s' to loading module '%s'\n",
>  				  patch->mod->name, obj->mod->name);
>  
> -			ret = klp_enable_object(obj);
> +			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
>  					patch->mod->name, obj->mod->name, ret);
> @@ -954,10 +954,10 @@ void klp_module_going(struct module *mod)
>  			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
>  				continue;
>  
> -			if (patch->state != KLP_DISABLED) {
> +			if (patch->enabled) {
>  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
>  					  patch->mod->name, obj->mod->name);
> -				klp_disable_object(obj);
> +				klp_unpatch_object(obj);
>  			}
>  
>  			klp_free_object_loaded(obj);
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [next] | [standalone]

#1377033

On Tue, Apr 12, 2016 at 09:44:43AM -0500, Chris J Arges wrote:
> On Fri, Mar 25, 2016 at 02:34:55PM -0500, Josh Poimboeuf wrote:
> > Once we have a consistency model, patches and their objects will be
> > enabled and disabled at different times.  For example, when a patch is
> > disabled, its loaded objects' funcs can remain registered with ftrace
> > indefinitely until the unpatching operation is complete and they're no
> > longer in use.
> > 
> > It's less confusing if we give them different names: patches can be
> > enabled or disabled; objects (and their funcs) can be patched or
> > unpatched:
> > 
> > - Enabled means that a patch is logically enabled (but not necessarily
> >   fully applied).
> > 
> > - Patched means that an object's funcs are registered with ftrace and
> >   added to the klp_ops func stack.
> > 
> > Also, since these states are binary, represent them with booleans
> > instead of ints.
> > 
> 
> Josh,
> 
> Awesome work here first of all!
> 
> Looking through the patchset a bit I see the following bools:
> - functions: patched, transitioning
> - objects: patched
> - patches: enabled
> 
> It seems this reflects the following states at a patch level:
> 	disabled - module inserted, not yet logically enabled
> 	enabled - logically enabled, but not all objects/functions patched
> 	transitioning - objects/functions are being applied or reverted
> 	patched - all objects/functions patched
> 
> However each object and function could have the same state and the parent object
> just reflects the 'aggregate state'. For example if all funcs in an object are
> patched then the object is also patched.
> 
> Perhaps we need more states (or maybe there will be more in the future), but
> wouldn't this just be easier to have something like for each patch, object, and
> function?
> 
> enum klp_state{
> 	KLP_DISABLED,
> 	KLP_ENABLED,
> 	KLP_TRANSITION,
> 	KLP_PATCHED,
> }
> 
> 
> I'm happy to help out too.

Thanks for the comments.  First let me try to explain why I chose two
bools rather than a single state variable.

At a func level, it's always in one of the following states:

  patched=0 transition=0: unpatched
  patched=0 transition=1: unpatched, temporary starting state
  patched=1 transition=1: patched, may be visible to some tasks
  patched=1 transition=0: patched, visible to all tasks

And for unpatching, it goes in the reverse order:

  patched=1 transition=0: patched, visible to all tasks
  patched=1 transition=1: patched, may be visible to some tasks
  patched=0 transition=1: unpatched, temporary ending state
  patched=0 transition=0: unpatched

(note to self, put the above in a comment somewhere)

Now, we could convert the states from two bools into a single enum.  But
I think it would complicate the code.  Because nowhere in the code does
it need to access the full state.  In some places it accesses
func->patched and in other places it accesses func->transition, but it
never needs to access both.

So for example, the following check in klp_ftrace_handler():

	if (unlikely(func->transition)) { 

would change to:

	if (unlikely(func->state == KLP_ENABLED || func->state == KLP_TRANSITION)) {

Sure, we could use a helper function to make that more readable.  But
with the bools its clearer and you don't need a helper function.

As another example, see the following code in klp_complete_transition():

	klp_for_each_object(klp_transition_patch, obj)
		klp_for_each_func(obj, func)
			func->transition = false;

The last line would have to be changed to something like:

			if (patching...)
				func->state = KLP_PATCHED;
			else /* unpatching */
				func->state = KLP_DISABLED;
		
So that's why I picked two bools over a single state variable: it seems
to make the code simpler.

As to the other idea about copying the func states to the object and
patch level, I get the feeling that would also complicate the code.  We
patch and transition at a function granularity, so the "real" state is
at the func level.  Proliferating that state to objects and patches
might be tricky to get right, and it could make it harder to understand
exactly what's going on in the code.  And I don't really see a benefit
to doing that.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1377044

On Tue, Apr 12, 2016 at 12:16:00PM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 12, 2016 at 09:44:43AM -0500, Chris J Arges wrote:
> > On Fri, Mar 25, 2016 at 02:34:55PM -0500, Josh Poimboeuf wrote:
> > > Once we have a consistency model, patches and their objects will be
> > > enabled and disabled at different times.  For example, when a patch is
> > > disabled, its loaded objects' funcs can remain registered with ftrace
> > > indefinitely until the unpatching operation is complete and they're no
> > > longer in use.
> > > 
> > > It's less confusing if we give them different names: patches can be
> > > enabled or disabled; objects (and their funcs) can be patched or
> > > unpatched:
> > > 
> > > - Enabled means that a patch is logically enabled (but not necessarily
> > >   fully applied).
> > > 
> > > - Patched means that an object's funcs are registered with ftrace and
> > >   added to the klp_ops func stack.
> > > 
> > > Also, since these states are binary, represent them with booleans
> > > instead of ints.
> > > 
> > 
> > Josh,
> > 
> > Awesome work here first of all!
> > 
> > Looking through the patchset a bit I see the following bools:
> > - functions: patched, transitioning
> > - objects: patched
> > - patches: enabled
> > 
> > It seems this reflects the following states at a patch level:
> > 	disabled - module inserted, not yet logically enabled
> > 	enabled - logically enabled, but not all objects/functions patched
> > 	transitioning - objects/functions are being applied or reverted
> > 	patched - all objects/functions patched
> > 
> > However each object and function could have the same state and the parent object
> > just reflects the 'aggregate state'. For example if all funcs in an object are
> > patched then the object is also patched.
> > 
> > Perhaps we need more states (or maybe there will be more in the future), but
> > wouldn't this just be easier to have something like for each patch, object, and
> > function?
> > 
> > enum klp_state{
> > 	KLP_DISABLED,
> > 	KLP_ENABLED,
> > 	KLP_TRANSITION,
> > 	KLP_PATCHED,
> > }
> > 
> > 
> > I'm happy to help out too.
> 
> Thanks for the comments.  First let me try to explain why I chose two
> bools rather than a single state variable.
> 
> At a func level, it's always in one of the following states:
> 
>   patched=0 transition=0: unpatched
>   patched=0 transition=1: unpatched, temporary starting state
>   patched=1 transition=1: patched, may be visible to some tasks
>   patched=1 transition=0: patched, visible to all tasks
> 
> And for unpatching, it goes in the reverse order:
> 
>   patched=1 transition=0: patched, visible to all tasks
>   patched=1 transition=1: patched, may be visible to some tasks
>   patched=0 transition=1: unpatched, temporary ending state
>   patched=0 transition=0: unpatched
> 
> (note to self, put the above in a comment somewhere)
>

This is helpful and makes more sense.
 
> Now, we could convert the states from two bools into a single enum.  But
> I think it would complicate the code.  Because nowhere in the code does
> it need to access the full state.  In some places it accesses
> func->patched and in other places it accesses func->transition, but it
> never needs to access both.
> 
> So for example, the following check in klp_ftrace_handler():
> 
> 	if (unlikely(func->transition)) { 
> 
> would change to:
> 
> 	if (unlikely(func->state == KLP_ENABLED || func->state == KLP_TRANSITION)) {
> 
> Sure, we could use a helper function to make that more readable.  But
> with the bools its clearer and you don't need a helper function.
> 
> As another example, see the following code in klp_complete_transition():
> 
> 	klp_for_each_object(klp_transition_patch, obj)
> 		klp_for_each_func(obj, func)
> 			func->transition = false;
> 
> The last line would have to be changed to something like:
> 
> 			if (patching...)
> 				func->state = KLP_PATCHED;
> 			else /* unpatching */
> 				func->state = KLP_DISABLED;
> 		
> So that's why I picked two bools over a single state variable: it seems
> to make the code simpler.
> 
> As to the other idea about copying the func states to the object and
> patch level, I get the feeling that would also complicate the code.  We
> patch and transition at a function granularity, so the "real" state is
> at the func level.  Proliferating that state to objects and patches
> might be tricky to get right, and it could make it harder to understand
> exactly what's going on in the code.  And I don't really see a benefit
> to doing that.
> 
> -- 
> Josh
> 

I think keeping it simple makes a ton of sense, thanks for the explanation.

I'm also envisioning how to troubleshoot patches that don't converge.

So if someone wanted to check if a function has been fully patched on all tasks
they would check:

/sys/kernel/livepatch/<patch>/transition
/sys/kernel/livepatch/<patch>/patched

And see if patched=1 and transition=0 things are applied.

However if patched=1 and transition=1 then if a user wanted to dig down and see
which pid's we were waiting on we could do:

 cat /proc/*/patch_status

and check if any pid's patch_status values still contain KLP_UNIVERSE_OLD.

If we wanted to see which function in the task needs patching we could:
 cat /proc/<pid>/stack
and see if anything in that stack contains the functions in question.

Anyway I'll keep looking at this patchset, patching using the samples/livepatch
code works for me without issue so far.

--chris

[toc] | [prev] | [next] | [standalone]

#1377108

On Tue, Apr 12, 2016 at 12:35:49PM -0500, Chris J Arges wrote:
> On Tue, Apr 12, 2016 at 12:16:00PM -0500, Josh Poimboeuf wrote:
> > On Tue, Apr 12, 2016 at 09:44:43AM -0500, Chris J Arges wrote:
> > > On Fri, Mar 25, 2016 at 02:34:55PM -0500, Josh Poimboeuf wrote:
> > > > Once we have a consistency model, patches and their objects will be
> > > > enabled and disabled at different times.  For example, when a patch is
> > > > disabled, its loaded objects' funcs can remain registered with ftrace
> > > > indefinitely until the unpatching operation is complete and they're no
> > > > longer in use.
> > > > 
> > > > It's less confusing if we give them different names: patches can be
> > > > enabled or disabled; objects (and their funcs) can be patched or
> > > > unpatched:
> > > > 
> > > > - Enabled means that a patch is logically enabled (but not necessarily
> > > >   fully applied).
> > > > 
> > > > - Patched means that an object's funcs are registered with ftrace and
> > > >   added to the klp_ops func stack.
> > > > 
> > > > Also, since these states are binary, represent them with booleans
> > > > instead of ints.
> > > > 
> > > 
> > > Josh,
> > > 
> > > Awesome work here first of all!
> > > 
> > > Looking through the patchset a bit I see the following bools:
> > > - functions: patched, transitioning
> > > - objects: patched
> > > - patches: enabled
> > > 
> > > It seems this reflects the following states at a patch level:
> > > 	disabled - module inserted, not yet logically enabled
> > > 	enabled - logically enabled, but not all objects/functions patched
> > > 	transitioning - objects/functions are being applied or reverted
> > > 	patched - all objects/functions patched
> > > 
> > > However each object and function could have the same state and the parent object
> > > just reflects the 'aggregate state'. For example if all funcs in an object are
> > > patched then the object is also patched.
> > > 
> > > Perhaps we need more states (or maybe there will be more in the future), but
> > > wouldn't this just be easier to have something like for each patch, object, and
> > > function?
> > > 
> > > enum klp_state{
> > > 	KLP_DISABLED,
> > > 	KLP_ENABLED,
> > > 	KLP_TRANSITION,
> > > 	KLP_PATCHED,
> > > }
> > > 
> > > 
> > > I'm happy to help out too.
> > 
> > Thanks for the comments.  First let me try to explain why I chose two
> > bools rather than a single state variable.
> > 
> > At a func level, it's always in one of the following states:
> > 
> >   patched=0 transition=0: unpatched
> >   patched=0 transition=1: unpatched, temporary starting state
> >   patched=1 transition=1: patched, may be visible to some tasks
> >   patched=1 transition=0: patched, visible to all tasks
> > 
> > And for unpatching, it goes in the reverse order:
> > 
> >   patched=1 transition=0: patched, visible to all tasks
> >   patched=1 transition=1: patched, may be visible to some tasks
> >   patched=0 transition=1: unpatched, temporary ending state
> >   patched=0 transition=0: unpatched
> > 
> > (note to self, put the above in a comment somewhere)
> >
> 
> This is helpful and makes more sense.
>  
> > Now, we could convert the states from two bools into a single enum.  But
> > I think it would complicate the code.  Because nowhere in the code does
> > it need to access the full state.  In some places it accesses
> > func->patched and in other places it accesses func->transition, but it
> > never needs to access both.
> > 
> > So for example, the following check in klp_ftrace_handler():
> > 
> > 	if (unlikely(func->transition)) { 
> > 
> > would change to:
> > 
> > 	if (unlikely(func->state == KLP_ENABLED || func->state == KLP_TRANSITION)) {
> > 
> > Sure, we could use a helper function to make that more readable.  But
> > with the bools its clearer and you don't need a helper function.
> > 
> > As another example, see the following code in klp_complete_transition():
> > 
> > 	klp_for_each_object(klp_transition_patch, obj)
> > 		klp_for_each_func(obj, func)
> > 			func->transition = false;
> > 
> > The last line would have to be changed to something like:
> > 
> > 			if (patching...)
> > 				func->state = KLP_PATCHED;
> > 			else /* unpatching */
> > 				func->state = KLP_DISABLED;
> > 		
> > So that's why I picked two bools over a single state variable: it seems
> > to make the code simpler.
> > 
> > As to the other idea about copying the func states to the object and
> > patch level, I get the feeling that would also complicate the code.  We
> > patch and transition at a function granularity, so the "real" state is
> > at the func level.  Proliferating that state to objects and patches
> > might be tricky to get right, and it could make it harder to understand
> > exactly what's going on in the code.  And I don't really see a benefit
> > to doing that.
> > 
> > -- 
> > Josh
> > 
> 
> I think keeping it simple makes a ton of sense, thanks for the explanation.
> 
> I'm also envisioning how to troubleshoot patches that don't converge.
> 
> So if someone wanted to check if a function has been fully patched on all tasks
> they would check:
> 
> /sys/kernel/livepatch/<patch>/transition
> /sys/kernel/livepatch/<patch>/patched
> 
> And see if patched=1 and transition=0 things are applied.
> 
> However if patched=1 and transition=1 then if a user wanted to dig down and see
> which pid's we were waiting on we could do:
> 
>  cat /proc/*/patch_status
> 
> and check if any pid's patch_status values still contain KLP_UNIVERSE_OLD.
> 
> If we wanted to see which function in the task needs patching we could:
>  cat /proc/<pid>/stack
> and see if anything in that stack contains the functions in question.

Yeah.  It's not ideal that the user has to check multiple places to see
the "state".  But IMO that's not a big deal, and a user space tool
should be able to make it more user friendly.

> Anyway I'll keep looking at this patchset, patching using the
> samples/livepatch code works for me without issue so far.

Ok, thanks!

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web