Re: [RFC PATCH 2/9] livepatch: separate enabled and patched states

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Feb 13, 2015 at 01:57:38PM +0100, Miroslav Benes wrote:
> On Mon, 9 Feb 2015, 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 boolean-type
> > variables instead of enums.
> 
> They are binary now but will it hold also in the future? I cannot come up 
> with any other possible state of the function right now, but that doesn't 
> mean there isn't any. It would be sad to return it back to enums one day 
> :)

I really can't think of any reason why they would become non-binary.
IMO it's more likely we could add more boolean variables, but if that
got out of hand we could just switch to using bit flags.

Either way I don't see a problem with changing them later if we need to.

> Also would it be useful to expose patched variable for functions and 
> objects in sysfs?

Not that I know of.  Do you have a use case in mind?  I view "patched"
as an internal variable, corresponding to whether the object or its
functions are registered with ftrace/klp_ops.  It doesn't mean "patched"
in a way that would really make sense to the user, because of the
gradual nature of the patching process.

> 
> Two small things below...

Agreed to both, thanks.

> 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  include/linux/livepatch.h | 15 ++++-----
> >  kernel/livepatch/core.c   | 79 +++++++++++++++++++++++------------------------
> >  2 files changed, 45 insertions(+), 49 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 95023fd..22a67d1 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
> > @@ -42,6 +37,7 @@ enum klp_state {
> >   * @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 */
> > @@ -59,8 +55,8 @@ struct klp_func {
> >  
> >  	/* internal */
> >  	struct kobject kobj;
> > -	enum klp_state state;
> >  	struct list_head stack_node;
> > +	int patched;
> >  };
> 
> @state remains in the comment above
> 
> >  /**
> > @@ -90,7 +86,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 +97,7 @@ struct klp_object {
> >  	/* internal */
> >  	struct kobject *kobj;
> >  	struct module *mod;
> > -	enum klp_state state;
> > +	int patched;
> >  };
> >  
> >  /**
> > @@ -111,6 +107,7 @@ struct klp_object {
> >   * @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 +117,7 @@ struct klp_patch {
> >  	/* internal */
> >  	struct list_head list;
> >  	struct kobject kobj;
> > -	enum klp_state state;
> > +	int enabled;
> >  };
> 
> Dtto
> 
> Miroslav

-- 
Josh