Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Thu, 20 Nov 2014, Seth Jennings wrote:

> On Thu, Nov 20, 2014 at 11:35:52AM -0600, Josh Poimboeuf wrote:
> > On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
> > > 
> > > On Sun, 16 Nov 2014, Seth Jennings wrote:
> > > 
> > > > This commit introduces code for the live patching core.  It implements
> > > > an ftrace-based mechanism and kernel interface for doing live patching
> > > > of kernel and kernel module functions.
> > > > 
> > > > It represents the greatest common functionality set between kpatch and
> > > > kgraft and can accept patches built using either method.
> > > > 
> > > > This first version does not implement any consistency mechanism that
> > > > ensures that old and new code do not run together.  In practice, ~90% of
> > > > CVEs are safe to apply in this way, since they simply add a conditional
> > > > check.  However, any function change that can not execute safely with
> > > > the old version of the function can _not_ be safely applied in this
> > > > version.
> > > > 
> > > > Signed-off-by: Seth Jennings 
> > > 
> > > Hi,
> > > 
> > > below is the patch which merges the internal and external data structures 
> > > (so it is only one part of our original patch for version 1). Apart from 
> > > that I tried to make minimal changes to the code. Only unnecessary 
> > > kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
> > > as it made more sense in this approach, I think.
> > > 
> > > I hope this clearly shows our point of view stated previously. What do 
> > > you say?
> > 
> > Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
> > ok with this patch (though I do have a few comments below).
> 
> Thanks Josh :)
> 
> Miroslav, before you send out a revision on this patch, I'm merging it
> for v3 right now.  I'll fixup any trivial fixes from this email.
> 
> I'm putting the finishing touches on v3 now.  Hopefully it will make
> everyone happy, or happier, with your changes merged.  Should be getting
> close...
> 
> Thanks,
> Seth

Ok, thank you for the fixes and merging. I'll take a closer look at v3 to 
be sure that everything is ok.

Mira
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Thu, 20 Nov 2014, Josh Poimboeuf wrote:

> On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
> > 
> > On Sun, 16 Nov 2014, Seth Jennings wrote:
> > 
> > > This commit introduces code for the live patching core.  It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
> > > 
> > > It represents the greatest common functionality set between kpatch and
> > > kgraft and can accept patches built using either method.
> > > 
> > > This first version does not implement any consistency mechanism that
> > > ensures that old and new code do not run together.  In practice, ~90% of
> > > CVEs are safe to apply in this way, since they simply add a conditional
> > > check.  However, any function change that can not execute safely with
> > > the old version of the function can _not_ be safely applied in this
> > > version.
> > > 
> > > Signed-off-by: Seth Jennings 
> > 
> > Hi,
> > 
> > below is the patch which merges the internal and external data structures 
> > (so it is only one part of our original patch for version 1). Apart from 
> > that I tried to make minimal changes to the code. Only unnecessary 
> > kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
> > as it made more sense in this approach, I think.
> > 
> > I hope this clearly shows our point of view stated previously. What do 
> > you say?
> 
> Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
> ok with this patch (though I do have a few comments below).

Great.

> > Next, I'll look at the three level hierarchy and sysfs directory and see 
> > if we can make it simpler yet keep its advantages.
> > 
> > Regards,
> > 
> > Miroslav Benes
> > SUSE Labs
> > 
> > -- >8 --
> > From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
> > From: Miroslav Benes 
> > Date: Wed, 19 Nov 2014 16:06:35 +0100
> > Subject: [PATCH] Remove the data duplication in internal and public 
> > structures
> > 
> > The split of internal and external structures is cleaner and makes the API 
> > more
> > stable. But it makes the code more complicated. It requires more space and 
> > data
> > copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
> > causes confusion.
> > 
> > The API is not a real issue for live patching. We take care neither of 
> > backward
> > nor forward compatibility. The dependency between a patch and kernel is even
> > more strict than by version. They have to use the same configuration and the
> > same build environment.
> > 
> > This patch merge the external and internal structures into one.  The 
> > structures
> > are initialized using ".item = value" syntax. Therefore the API is 
> > basically as
> > stable as it was before. We could later even hide it under some helper 
> > macros
> > if requested.
> > 
> > For the purpose if this patch, we used the prefix "lpc". It allows to make 
> > as
> > less changes as possible and show the real effect. If the patch is 
> > accepted, it
> > would make sense to merge it into the original patch and even use another
> > common prefix, for example the proposed "klp".
> > 
> > Signed-off-by: Miroslav Benes 
> > ---
> >  include/linux/livepatch.h |  47 +--
> >  kernel/livepatch/core.c   | 338 
> > --
> >  2 files changed, 121 insertions(+), 264 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 8b68fef..f16de32 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -21,10 +21,23 @@
> >  #define _LINUX_LIVEPATCH_H_
> >  
> >  #include 
> > +#include 
> >  
> >  /* TODO: add kernel-doc for structures once agreed upon */
> >  
> > -struct lp_func {
> > +enum lpc_state {
> > +   LPC_DISABLED,
> > +   LPC_ENABLED
> > +};
> > +
> > +struct lpc_func {
> > +   /* internal */
> 
> Would it be a little clearer to list the external fields first?

It would. It was not possible in v1 of my patch as flexible array needs to 
be the last in struct (struct lpc_func funcs[]). Now as it is a pointer it 
makes perfect sense to have the external fields first.

> > +   struct kobject *kobj;
> > +   struct ftrace_ops fops;
> > +   enum lpc_state state;
> > +   unsigned long new_addr;
> 
> I think this internal new_addr field isn't really needed since we
> already have the external new_func field.

True.

> > +
> > +   /* external */
> > const char *old_name; /* function to be patched */
> > void *new_func; /* replacement function in patch module */
> > /*
> > @@ -38,7 +51,7 @@ struct lp_func {
> > unsigned long old_addr;
> >  };

[...]

> > -static struct lpc_func *lpc_create_func(struct kobject *root,
> > -   struct lp_func *userfunc)
> > +static int lpc_init_funcs(struct lpc_object *obj)
> >  {
> > struct lpc_func *func;
> > struct ftrace_ops *ops;
> > -   int ret;
> > -
> > -   /* alloc */

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Thu, 20 Nov 2014, Josh Poimboeuf wrote:

 On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
  
  On Sun, 16 Nov 2014, Seth Jennings wrote:
  
   This commit introduces code for the live patching core.  It implements
   an ftrace-based mechanism and kernel interface for doing live patching
   of kernel and kernel module functions.
   
   It represents the greatest common functionality set between kpatch and
   kgraft and can accept patches built using either method.
   
   This first version does not implement any consistency mechanism that
   ensures that old and new code do not run together.  In practice, ~90% of
   CVEs are safe to apply in this way, since they simply add a conditional
   check.  However, any function change that can not execute safely with
   the old version of the function can _not_ be safely applied in this
   version.
   
   Signed-off-by: Seth Jennings sjenn...@redhat.com
  
  Hi,
  
  below is the patch which merges the internal and external data structures 
  (so it is only one part of our original patch for version 1). Apart from 
  that I tried to make minimal changes to the code. Only unnecessary 
  kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
  as it made more sense in this approach, I think.
  
  I hope this clearly shows our point of view stated previously. What do 
  you say?
 
 Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
 ok with this patch (though I do have a few comments below).

Great.

  Next, I'll look at the three level hierarchy and sysfs directory and see 
  if we can make it simpler yet keep its advantages.
  
  Regards,
  
  Miroslav Benes
  SUSE Labs
  
  -- 8 --
  From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
  From: Miroslav Benes mbe...@suse.cz
  Date: Wed, 19 Nov 2014 16:06:35 +0100
  Subject: [PATCH] Remove the data duplication in internal and public 
  structures
  
  The split of internal and external structures is cleaner and makes the API 
  more
  stable. But it makes the code more complicated. It requires more space and 
  data
  copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
  causes confusion.
  
  The API is not a real issue for live patching. We take care neither of 
  backward
  nor forward compatibility. The dependency between a patch and kernel is even
  more strict than by version. They have to use the same configuration and the
  same build environment.
  
  This patch merge the external and internal structures into one.  The 
  structures
  are initialized using .item = value syntax. Therefore the API is 
  basically as
  stable as it was before. We could later even hide it under some helper 
  macros
  if requested.
  
  For the purpose if this patch, we used the prefix lpc. It allows to make 
  as
  less changes as possible and show the real effect. If the patch is 
  accepted, it
  would make sense to merge it into the original patch and even use another
  common prefix, for example the proposed klp.
  
  Signed-off-by: Miroslav Benes mbe...@suse.cz
  ---
   include/linux/livepatch.h |  47 +--
   kernel/livepatch/core.c   | 338 
  --
   2 files changed, 121 insertions(+), 264 deletions(-)
  
  diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
  index 8b68fef..f16de32 100644
  --- a/include/linux/livepatch.h
  +++ b/include/linux/livepatch.h
  @@ -21,10 +21,23 @@
   #define _LINUX_LIVEPATCH_H_
   
   #include linux/module.h
  +#include linux/ftrace.h
   
   /* TODO: add kernel-doc for structures once agreed upon */
   
  -struct lp_func {
  +enum lpc_state {
  +   LPC_DISABLED,
  +   LPC_ENABLED
  +};
  +
  +struct lpc_func {
  +   /* internal */
 
 Would it be a little clearer to list the external fields first?

It would. It was not possible in v1 of my patch as flexible array needs to 
be the last in struct (struct lpc_func funcs[]). Now as it is a pointer it 
makes perfect sense to have the external fields first.

  +   struct kobject *kobj;
  +   struct ftrace_ops fops;
  +   enum lpc_state state;
  +   unsigned long new_addr;
 
 I think this internal new_addr field isn't really needed since we
 already have the external new_func field.

True.

  +
  +   /* external */
  const char *old_name; /* function to be patched */
  void *new_func; /* replacement function in patch module */
  /*
  @@ -38,7 +51,7 @@ struct lp_func {
  unsigned long old_addr;
   };

[...]

  -static struct lpc_func *lpc_create_func(struct kobject *root,
  -   struct lp_func *userfunc)
  +static int lpc_init_funcs(struct lpc_object *obj)
   {
  struct lpc_func *func;
  struct ftrace_ops *ops;
  -   int ret;
  -
  -   /* alloc */
  -   func = kzalloc(sizeof(*func), GFP_KERNEL);
  -   if (!func)
  -   return NULL;
  -
  -   /* init */
  -   INIT_LIST_HEAD(func-list);
  -   func-old_name = userfunc-old_name;
  -   

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Thu, 20 Nov 2014, Seth Jennings wrote:

 On Thu, Nov 20, 2014 at 11:35:52AM -0600, Josh Poimboeuf wrote:
  On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
   
   On Sun, 16 Nov 2014, Seth Jennings wrote:
   
This commit introduces code for the live patching core.  It implements
an ftrace-based mechanism and kernel interface for doing live patching
of kernel and kernel module functions.

It represents the greatest common functionality set between kpatch and
kgraft and can accept patches built using either method.

This first version does not implement any consistency mechanism that
ensures that old and new code do not run together.  In practice, ~90% of
CVEs are safe to apply in this way, since they simply add a conditional
check.  However, any function change that can not execute safely with
the old version of the function can _not_ be safely applied in this
version.

Signed-off-by: Seth Jennings sjenn...@redhat.com
   
   Hi,
   
   below is the patch which merges the internal and external data structures 
   (so it is only one part of our original patch for version 1). Apart from 
   that I tried to make minimal changes to the code. Only unnecessary 
   kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
   as it made more sense in this approach, I think.
   
   I hope this clearly shows our point of view stated previously. What do 
   you say?
  
  Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
  ok with this patch (though I do have a few comments below).
 
 Thanks Josh :)
 
 Miroslav, before you send out a revision on this patch, I'm merging it
 for v3 right now.  I'll fixup any trivial fixes from this email.
 
 I'm putting the finishing touches on v3 now.  Hopefully it will make
 everyone happy, or happier, with your changes merged.  Should be getting
 close...
 
 Thanks,
 Seth

Ok, thank you for the fixes and merging. I'll take a closer look at v3 to 
be sure that everything is ok.

Mira
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Thu, Nov 20, 2014 at 11:35:52AM -0600, Josh Poimboeuf wrote:
> On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
> > 
> > On Sun, 16 Nov 2014, Seth Jennings wrote:
> > 
> > > This commit introduces code for the live patching core.  It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
> > > 
> > > It represents the greatest common functionality set between kpatch and
> > > kgraft and can accept patches built using either method.
> > > 
> > > This first version does not implement any consistency mechanism that
> > > ensures that old and new code do not run together.  In practice, ~90% of
> > > CVEs are safe to apply in this way, since they simply add a conditional
> > > check.  However, any function change that can not execute safely with
> > > the old version of the function can _not_ be safely applied in this
> > > version.
> > > 
> > > Signed-off-by: Seth Jennings 
> > 
> > Hi,
> > 
> > below is the patch which merges the internal and external data structures 
> > (so it is only one part of our original patch for version 1). Apart from 
> > that I tried to make minimal changes to the code. Only unnecessary 
> > kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
> > as it made more sense in this approach, I think.
> > 
> > I hope this clearly shows our point of view stated previously. What do 
> > you say?
> 
> Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
> ok with this patch (though I do have a few comments below).

Thanks Josh :)

Miroslav, before you send out a revision on this patch, I'm merging it
for v3 right now.  I'll fixup any trivial fixes from this email.

I'm putting the finishing touches on v3 now.  Hopefully it will make
everyone happy, or happier, with your changes merged.  Should be getting
close...

Thanks,
Seth

> 
> > Next, I'll look at the three level hierarchy and sysfs directory and see 
> > if we can make it simpler yet keep its advantages.
> > 
> > Regards,
> > 
> > Miroslav Benes
> > SUSE Labs
> > 
> > -- >8 --
> > From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
> > From: Miroslav Benes 
> > Date: Wed, 19 Nov 2014 16:06:35 +0100
> > Subject: [PATCH] Remove the data duplication in internal and public 
> > structures
> > 
> > The split of internal and external structures is cleaner and makes the API 
> > more
> > stable. But it makes the code more complicated. It requires more space and 
> > data
> > copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
> > causes confusion.
> > 
> > The API is not a real issue for live patching. We take care neither of 
> > backward
> > nor forward compatibility. The dependency between a patch and kernel is even
> > more strict than by version. They have to use the same configuration and the
> > same build environment.
> > 
> > This patch merge the external and internal structures into one.  The 
> > structures
> > are initialized using ".item = value" syntax. Therefore the API is 
> > basically as
> > stable as it was before. We could later even hide it under some helper 
> > macros
> > if requested.
> > 
> > For the purpose if this patch, we used the prefix "lpc". It allows to make 
> > as
> > less changes as possible and show the real effect. If the patch is 
> > accepted, it
> > would make sense to merge it into the original patch and even use another
> > common prefix, for example the proposed "klp".
> > 
> > Signed-off-by: Miroslav Benes 
> > ---
> >  include/linux/livepatch.h |  47 +--
> >  kernel/livepatch/core.c   | 338 
> > --
> >  2 files changed, 121 insertions(+), 264 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 8b68fef..f16de32 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -21,10 +21,23 @@
> >  #define _LINUX_LIVEPATCH_H_
> >  
> >  #include 
> > +#include 
> >  
> >  /* TODO: add kernel-doc for structures once agreed upon */
> >  
> > -struct lp_func {
> > +enum lpc_state {
> > +   LPC_DISABLED,
> > +   LPC_ENABLED
> > +};
> > +
> > +struct lpc_func {
> > +   /* internal */
> 
> Would it be a little clearer to list the external fields first?
> 
> > +   struct kobject *kobj;
> > +   struct ftrace_ops fops;
> > +   enum lpc_state state;
> > +   unsigned long new_addr;
> 
> I think this internal new_addr field isn't really needed since we
> already have the external new_func field.
> 
> > +
> > +   /* external */
> > const char *old_name; /* function to be patched */
> > void *new_func; /* replacement function in patch module */
> > /*
> > @@ -38,7 +51,7 @@ struct lp_func {
> > unsigned long old_addr;
> >  };
> >  
> > -struct lp_reloc {
> > +struct lpc_reloc {
> > unsigned long dest;
> > unsigned long src;
> > unsigned long type;
> > @@ -47,21 +60,33 @@ struct lp_reloc {
> > 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Josh Poimboeuf]

On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> > This commit introduces code for the live patching core.  It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
> > 
> > It represents the greatest common functionality set between kpatch and
> > kgraft and can accept patches built using either method.
> > 
> > This first version does not implement any consistency mechanism that
> > ensures that old and new code do not run together.  In practice, ~90% of
> > CVEs are safe to apply in this way, since they simply add a conditional
> > check.  However, any function change that can not execute safely with
> > the old version of the function can _not_ be safely applied in this
> > version.
> > 
> > Signed-off-by: Seth Jennings 
> 
> Hi,
> 
> below is the patch which merges the internal and external data structures 
> (so it is only one part of our original patch for version 1). Apart from 
> that I tried to make minimal changes to the code. Only unnecessary 
> kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
> as it made more sense in this approach, I think.
> 
> I hope this clearly shows our point of view stated previously. What do 
> you say?

Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
ok with this patch (though I do have a few comments below).

> Next, I'll look at the three level hierarchy and sysfs directory and see 
> if we can make it simpler yet keep its advantages.
> 
> Regards,
> 
> Miroslav Benes
> SUSE Labs
> 
> -- >8 --
> From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
> From: Miroslav Benes 
> Date: Wed, 19 Nov 2014 16:06:35 +0100
> Subject: [PATCH] Remove the data duplication in internal and public structures
> 
> The split of internal and external structures is cleaner and makes the API 
> more
> stable. But it makes the code more complicated. It requires more space and 
> data
> copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
> causes confusion.
> 
> The API is not a real issue for live patching. We take care neither of 
> backward
> nor forward compatibility. The dependency between a patch and kernel is even
> more strict than by version. They have to use the same configuration and the
> same build environment.
> 
> This patch merge the external and internal structures into one.  The 
> structures
> are initialized using ".item = value" syntax. Therefore the API is basically 
> as
> stable as it was before. We could later even hide it under some helper macros
> if requested.
> 
> For the purpose if this patch, we used the prefix "lpc". It allows to make as
> less changes as possible and show the real effect. If the patch is accepted, 
> it
> would make sense to merge it into the original patch and even use another
> common prefix, for example the proposed "klp".
> 
> Signed-off-by: Miroslav Benes 
> ---
>  include/linux/livepatch.h |  47 +--
>  kernel/livepatch/core.c   | 338 
> --
>  2 files changed, 121 insertions(+), 264 deletions(-)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 8b68fef..f16de32 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -21,10 +21,23 @@
>  #define _LINUX_LIVEPATCH_H_
>  
>  #include 
> +#include 
>  
>  /* TODO: add kernel-doc for structures once agreed upon */
>  
> -struct lp_func {
> +enum lpc_state {
> + LPC_DISABLED,
> + LPC_ENABLED
> +};
> +
> +struct lpc_func {
> + /* internal */

Would it be a little clearer to list the external fields first?

> + struct kobject *kobj;
> + struct ftrace_ops fops;
> + enum lpc_state state;
> + unsigned long new_addr;

I think this internal new_addr field isn't really needed since we
already have the external new_func field.

> +
> + /* external */
>   const char *old_name; /* function to be patched */
>   void *new_func; /* replacement function in patch module */
>   /*
> @@ -38,7 +51,7 @@ struct lp_func {
>   unsigned long old_addr;
>  };
>  
> -struct lp_reloc {
> +struct lpc_reloc {
>   unsigned long dest;
>   unsigned long src;
>   unsigned long type;
> @@ -47,21 +60,33 @@ struct lp_reloc {
>   int external;
>  };
>  
> -struct lp_object {
> +struct lpc_object {
> + /* internal */
> + struct kobject *kobj;
> + struct module *mod; /* module associated with object */
> + enum lpc_state state;
> +
> + /* external */
>   const char *name; /* "vmlinux" or module name */
> - struct lp_func *funcs;
> - struct lp_reloc *relocs;
> + struct lpc_reloc *relocs;
> + struct lpc_func *funcs;
>  };
>  
> -struct lp_patch {
> +struct lpc_patch {
> + /* internal */
> + struct list_head list;
> + struct kobject kobj;
> + enum lpc_state state;
> +
> + /* external */

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Thu, Nov 20, 2014 at 09:19:54AM -0600, Josh Poimboeuf wrote:
> On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
> > +static int lpc_module_notify(struct notifier_block *nb, unsigned long 
> > action,
> > +   void *data)
> > +{
> > +   struct module *mod = data;
> > +   struct lpc_patch *patch;
> > +   struct lpc_object *obj;
> > +
> > +   mutex_lock(_mutex);
> > +
> > +   if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> > +   goto out;
> 
> I think we can get the mutex here instead of above so it doesn't block
> other module actions (and then you can also get rid of the "out" label).

Sure.

Thanks,
Seth

> 
> > +
> > +   list_for_each_entry(patch, _patches, list) {
> > +   if (patch->state == LPC_DISABLED)
> > +   continue;
> > +   list_for_each_entry(obj, >objs, list) {
> > +   if (strcmp(obj->name, mod->name))
> > +   continue;
> > +   if (action == MODULE_STATE_COMING) {
> > +   obj->mod = mod;
> > +   lpc_module_notify_coming(patch->mod, obj);
> > +   } else /* MODULE_STATE_GOING */
> > +   lpc_module_notify_going(patch->mod, obj);
> > +   break;
> > +   }
> > +   }
> > +out:
> > +   mutex_unlock(_mutex);
> > +   return 0;
> > +}
> 
> -- 
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Josh Poimboeuf]

On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
> +static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> + struct lpc_patch *patch;
> + struct lpc_object *obj;
> +
> + mutex_lock(_mutex);
> +
> + if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> + goto out;

I think we can get the mutex here instead of above so it doesn't block
other module actions (and then you can also get rid of the "out" label).

> +
> + list_for_each_entry(patch, _patches, list) {
> + if (patch->state == LPC_DISABLED)
> + continue;
> + list_for_each_entry(obj, >objs, list) {
> + if (strcmp(obj->name, mod->name))
> + continue;
> + if (action == MODULE_STATE_COMING) {
> + obj->mod = mod;
> + lpc_module_notify_coming(patch->mod, obj);
> + } else /* MODULE_STATE_GOING */
> + lpc_module_notify_going(patch->mod, obj);
> + break;
> + }
> + }
> +out:
> + mutex_unlock(_mutex);
> + return 0;
> +}

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Wed, 19 Nov 2014, Seth Jennings wrote:

> On Tue, Nov 18, 2014 at 03:45:22PM +0100, Miroslav Benes wrote:
> > 
> > On Sun, 16 Nov 2014, Seth Jennings wrote:
> > 
> > [...]
> > 
> > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > > new file mode 100644
> > > index 000..8b68fef
> > > --- /dev/null
> > > +++ b/include/linux/livepatch.h
> > > @@ -0,0 +1,68 @@
> > > +/*
> > > + * livepatch.h - Live Kernel Patching Core
> > > + *
> > > + * Copyright (C) 2014 Seth Jennings 
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * as published by the Free Software Foundation; either version 2
> > > + * of the License, or (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, see .
> > > + */
> > > +
> > > +#ifndef _LINUX_LIVEPATCH_H_
> > > +#define _LINUX_LIVEPATCH_H_
> > > +
> > > +#include 
> > > +
> > 
> > I think we need something like 
> > 
> > #if IS_ENABLED(CONFIG_LIVE_PATCHING)
> > 
> > here. Otherwise kernel module with live patch itself would be built 
> > even with live patching support disabled (as the structures and needed 
> > functions are declared).
> 
> What do you think of this (already includes s/lp/klp/ change)?
> 
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 0143b73..a9821f3 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -21,6 +21,7 @@
>  #define _LINUX_LIVEPATCH_H_
>  
>  #include 
> +#include 
>  
>  /* TODO: add kernel-doc for structures once agreed upon */
>  
> @@ -58,11 +59,20 @@ struct klp_patch {
> struct klp_object *objs;
>  };
>  
> -int klp_register_patch(struct klp_patch *);
> -int klp_unregister_patch(struct klp_patch *);
> -int klp_enable_patch(struct klp_patch *);
> -int klp_disable_patch(struct klp_patch *);
> +#ifdef CONFIG_LIVE_PATCHING
>  
> -#include 
> +extern int klp_register_patch(struct klp_patch *);
> +extern int klp_unregister_patch(struct klp_patch *);
> +extern int klp_enable_patch(struct klp_patch *);
> +extern int klp_disable_patch(struct klp_patch *);
> +
> +#else /* !CONFIG_LIVE_PATCHING */
> +
> +static int klp_register_patch(struct klp_patch *k) { return -ENOSYS; }
> +static int klp_unregister_patch(struct klp_patch *k) { return -ENOSYS; }
> +static int klp_enable_patch(struct klp_patch *k) { return -ENOSYS; }
> +static int klp_disable_patch(struct klp_patch *k) { return -ENOSYS; }
> +
> +#endif
> 
> 
> This seems to be the way many headers handle this.  Patch modules built
> against a kernel that doesn't support live patching will build cleanly,
> but will always fail to load.
> 
> Seth

Hm, I would still vote for build failure. I think it doesn't make sense to 
build patch module against a kernel that doesn't support live patching and 
it is better to let the user know (and not potentially someone else who 
would load it and fail). Afaik the other headers handle it your way 
because otherwise the code would be spoiled by #ifdefs in .c files. 
However I think that our case is a bit different.

Anyway it is better to use #if (IS_ENABLED(CONFIG_LIVE_PATCHING)) than 
simple #ifdef (see Documentation/CodingStyle) and make the functions 
static inlined for !CONFIG_LIVE_PATCHING case.

Mira

> > 
> > > +/* TODO: add kernel-doc for structures once agreed upon */
> > > +
> > > +struct lp_func {
> > > + const char *old_name; /* function to be patched */
> > > + void *new_func; /* replacement function in patch module */
> > > + /*
> > > +  * The old_addr field is optional and can be used to resolve
> > > +  * duplicate symbol names in the vmlinux object.  If this
> > > +  * information is not present, the symbol is located by name
> > > +  * with kallsyms. If the name is not unique and old_addr is
> > > +  * not provided, the patch application fails as there is no
> > > +  * way to resolve the ambiguity.
> > > +  */
> > > + unsigned long old_addr;
> > > +};
> > > +
> > > +struct lp_reloc {
> > > + unsigned long dest;
> > > + unsigned long src;
> > > + unsigned long type;
> > > + const char *name;
> > > + int addend;
> > > + int external;
> > > +};
> > > +
> > > +struct lp_object {
> > > + const char *name; /* "vmlinux" or module name */
> > > + struct lp_func *funcs;
> > > + struct lp_reloc *relocs;
> > > +};
> > > +
> > > +struct lp_patch {
> > > + struct module *mod; /* module containing the patch */
> > > + struct lp_object *objs;
> > > +};
> > > +
> > > +int lp_register_patch(struct lp_patch *);
> > > +int 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Sun, 16 Nov 2014, Seth Jennings wrote:

> This commit introduces code for the live patching core.  It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
> 
> It represents the greatest common functionality set between kpatch and
> kgraft and can accept patches built using either method.
> 
> This first version does not implement any consistency mechanism that
> ensures that old and new code do not run together.  In practice, ~90% of
> CVEs are safe to apply in this way, since they simply add a conditional
> check.  However, any function change that can not execute safely with
> the old version of the function can _not_ be safely applied in this
> version.
> 
> Signed-off-by: Seth Jennings 

Hi,

below is the patch which merges the internal and external data structures 
(so it is only one part of our original patch for version 1). Apart from 
that I tried to make minimal changes to the code. Only unnecessary 
kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
as it made more sense in this approach, I think.

I hope this clearly shows our point of view stated previously. What do 
you say?

Next, I'll look at the three level hierarchy and sysfs directory and see 
if we can make it simpler yet keep its advantages.

Regards,

Miroslav Benes
SUSE Labs

-- >8 --
>From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
From: Miroslav Benes 
Date: Wed, 19 Nov 2014 16:06:35 +0100
Subject: [PATCH] Remove the data duplication in internal and public structures

The split of internal and external structures is cleaner and makes the API more
stable. But it makes the code more complicated. It requires more space and data
copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
causes confusion.

The API is not a real issue for live patching. We take care neither of backward
nor forward compatibility. The dependency between a patch and kernel is even
more strict than by version. They have to use the same configuration and the
same build environment.

This patch merge the external and internal structures into one.  The structures
are initialized using ".item = value" syntax. Therefore the API is basically as
stable as it was before. We could later even hide it under some helper macros
if requested.

For the purpose if this patch, we used the prefix "lpc". It allows to make as
less changes as possible and show the real effect. If the patch is accepted, it
would make sense to merge it into the original patch and even use another
common prefix, for example the proposed "klp".

Signed-off-by: Miroslav Benes 
---
 include/linux/livepatch.h |  47 +--
 kernel/livepatch/core.c   | 338 --
 2 files changed, 121 insertions(+), 264 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8b68fef..f16de32 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,10 +21,23 @@
 #define _LINUX_LIVEPATCH_H_
 
 #include 
+#include 
 
 /* TODO: add kernel-doc for structures once agreed upon */
 
-struct lp_func {
+enum lpc_state {
+   LPC_DISABLED,
+   LPC_ENABLED
+};
+
+struct lpc_func {
+   /* internal */
+   struct kobject *kobj;
+   struct ftrace_ops fops;
+   enum lpc_state state;
+   unsigned long new_addr;
+
+   /* external */
const char *old_name; /* function to be patched */
void *new_func; /* replacement function in patch module */
/*
@@ -38,7 +51,7 @@ struct lp_func {
unsigned long old_addr;
 };
 
-struct lp_reloc {
+struct lpc_reloc {
unsigned long dest;
unsigned long src;
unsigned long type;
@@ -47,21 +60,33 @@ struct lp_reloc {
int external;
 };
 
-struct lp_object {
+struct lpc_object {
+   /* internal */
+   struct kobject *kobj;
+   struct module *mod; /* module associated with object */
+   enum lpc_state state;
+
+   /* external */
const char *name; /* "vmlinux" or module name */
-   struct lp_func *funcs;
-   struct lp_reloc *relocs;
+   struct lpc_reloc *relocs;
+   struct lpc_func *funcs;
 };
 
-struct lp_patch {
+struct lpc_patch {
+   /* internal */
+   struct list_head list;
+   struct kobject kobj;
+   enum lpc_state state;
+
+   /* external */
struct module *mod; /* module containing the patch */
-   struct lp_object *objs;
+   struct lpc_object *objs;
 };
 
-int lp_register_patch(struct lp_patch *);
-int lp_unregister_patch(struct lp_patch *);
-int lp_enable_patch(struct lp_patch *);
-int lp_disable_patch(struct lp_patch *);
+extern int lpc_register_patch(struct lpc_patch *);
+extern int lpc_unregister_patch(struct lpc_patch *);
+extern int lpc_enable_patch(struct lpc_patch *);
+extern int lpc_disable_patch(struct lpc_patch *);
 
 #include 
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Sun, 16 Nov 2014, Seth Jennings wrote:

 This commit introduces code for the live patching core.  It implements
 an ftrace-based mechanism and kernel interface for doing live patching
 of kernel and kernel module functions.
 
 It represents the greatest common functionality set between kpatch and
 kgraft and can accept patches built using either method.
 
 This first version does not implement any consistency mechanism that
 ensures that old and new code do not run together.  In practice, ~90% of
 CVEs are safe to apply in this way, since they simply add a conditional
 check.  However, any function change that can not execute safely with
 the old version of the function can _not_ be safely applied in this
 version.
 
 Signed-off-by: Seth Jennings sjenn...@redhat.com

Hi,

below is the patch which merges the internal and external data structures 
(so it is only one part of our original patch for version 1). Apart from 
that I tried to make minimal changes to the code. Only unnecessary 
kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
as it made more sense in this approach, I think.

I hope this clearly shows our point of view stated previously. What do 
you say?

Next, I'll look at the three level hierarchy and sysfs directory and see 
if we can make it simpler yet keep its advantages.

Regards,

Miroslav Benes
SUSE Labs

-- 8 --
From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
From: Miroslav Benes mbe...@suse.cz
Date: Wed, 19 Nov 2014 16:06:35 +0100
Subject: [PATCH] Remove the data duplication in internal and public structures

The split of internal and external structures is cleaner and makes the API more
stable. But it makes the code more complicated. It requires more space and data
copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
causes confusion.

The API is not a real issue for live patching. We take care neither of backward
nor forward compatibility. The dependency between a patch and kernel is even
more strict than by version. They have to use the same configuration and the
same build environment.

This patch merge the external and internal structures into one.  The structures
are initialized using .item = value syntax. Therefore the API is basically as
stable as it was before. We could later even hide it under some helper macros
if requested.

For the purpose if this patch, we used the prefix lpc. It allows to make as
less changes as possible and show the real effect. If the patch is accepted, it
would make sense to merge it into the original patch and even use another
common prefix, for example the proposed klp.

Signed-off-by: Miroslav Benes mbe...@suse.cz
---
 include/linux/livepatch.h |  47 +--
 kernel/livepatch/core.c   | 338 --
 2 files changed, 121 insertions(+), 264 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8b68fef..f16de32 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,10 +21,23 @@
 #define _LINUX_LIVEPATCH_H_
 
 #include linux/module.h
+#include linux/ftrace.h
 
 /* TODO: add kernel-doc for structures once agreed upon */
 
-struct lp_func {
+enum lpc_state {
+   LPC_DISABLED,
+   LPC_ENABLED
+};
+
+struct lpc_func {
+   /* internal */
+   struct kobject *kobj;
+   struct ftrace_ops fops;
+   enum lpc_state state;
+   unsigned long new_addr;
+
+   /* external */
const char *old_name; /* function to be patched */
void *new_func; /* replacement function in patch module */
/*
@@ -38,7 +51,7 @@ struct lp_func {
unsigned long old_addr;
 };
 
-struct lp_reloc {
+struct lpc_reloc {
unsigned long dest;
unsigned long src;
unsigned long type;
@@ -47,21 +60,33 @@ struct lp_reloc {
int external;
 };
 
-struct lp_object {
+struct lpc_object {
+   /* internal */
+   struct kobject *kobj;
+   struct module *mod; /* module associated with object */
+   enum lpc_state state;
+
+   /* external */
const char *name; /* vmlinux or module name */
-   struct lp_func *funcs;
-   struct lp_reloc *relocs;
+   struct lpc_reloc *relocs;
+   struct lpc_func *funcs;
 };
 
-struct lp_patch {
+struct lpc_patch {
+   /* internal */
+   struct list_head list;
+   struct kobject kobj;
+   enum lpc_state state;
+
+   /* external */
struct module *mod; /* module containing the patch */
-   struct lp_object *objs;
+   struct lpc_object *objs;
 };
 
-int lp_register_patch(struct lp_patch *);
-int lp_unregister_patch(struct lp_patch *);
-int lp_enable_patch(struct lp_patch *);
-int lp_disable_patch(struct lp_patch *);
+extern int lpc_register_patch(struct lpc_patch *);
+extern int lpc_unregister_patch(struct lpc_patch *);
+extern int lpc_enable_patch(struct lpc_patch *);
+extern int lpc_disable_patch(struct lpc_patch *);
 
 #include asm/livepatch.h
 
diff 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Wed, 19 Nov 2014, Seth Jennings wrote:

 On Tue, Nov 18, 2014 at 03:45:22PM +0100, Miroslav Benes wrote:
  
  On Sun, 16 Nov 2014, Seth Jennings wrote:
  
  [...]
  
   diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
   new file mode 100644
   index 000..8b68fef
   --- /dev/null
   +++ b/include/linux/livepatch.h
   @@ -0,0 +1,68 @@
   +/*
   + * livepatch.h - Live Kernel Patching Core
   + *
   + * Copyright (C) 2014 Seth Jennings sjenn...@redhat.com
   + *
   + * This program is free software; you can redistribute it and/or
   + * modify it under the terms of the GNU General Public License
   + * as published by the Free Software Foundation; either version 2
   + * of the License, or (at your option) any later version.
   + *
   + * This program is distributed in the hope that it will be useful,
   + * but WITHOUT ANY WARRANTY; without even the implied warranty of
   + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   + * GNU General Public License for more details.
   + *
   + * You should have received a copy of the GNU General Public License
   + * along with this program; if not, see http://www.gnu.org/licenses/.
   + */
   +
   +#ifndef _LINUX_LIVEPATCH_H_
   +#define _LINUX_LIVEPATCH_H_
   +
   +#include linux/module.h
   +
  
  I think we need something like 
  
  #if IS_ENABLED(CONFIG_LIVE_PATCHING)
  
  here. Otherwise kernel module with live patch itself would be built 
  even with live patching support disabled (as the structures and needed 
  functions are declared).
 
 What do you think of this (already includes s/lp/klp/ change)?
 
 
 diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
 index 0143b73..a9821f3 100644
 --- a/include/linux/livepatch.h
 +++ b/include/linux/livepatch.h
 @@ -21,6 +21,7 @@
  #define _LINUX_LIVEPATCH_H_
  
  #include linux/module.h
 +#include asm/livepatch.h
  
  /* TODO: add kernel-doc for structures once agreed upon */
  
 @@ -58,11 +59,20 @@ struct klp_patch {
 struct klp_object *objs;
  };
  
 -int klp_register_patch(struct klp_patch *);
 -int klp_unregister_patch(struct klp_patch *);
 -int klp_enable_patch(struct klp_patch *);
 -int klp_disable_patch(struct klp_patch *);
 +#ifdef CONFIG_LIVE_PATCHING
  
 -#include asm/livepatch.h
 +extern int klp_register_patch(struct klp_patch *);
 +extern int klp_unregister_patch(struct klp_patch *);
 +extern int klp_enable_patch(struct klp_patch *);
 +extern int klp_disable_patch(struct klp_patch *);
 +
 +#else /* !CONFIG_LIVE_PATCHING */
 +
 +static int klp_register_patch(struct klp_patch *k) { return -ENOSYS; }
 +static int klp_unregister_patch(struct klp_patch *k) { return -ENOSYS; }
 +static int klp_enable_patch(struct klp_patch *k) { return -ENOSYS; }
 +static int klp_disable_patch(struct klp_patch *k) { return -ENOSYS; }
 +
 +#endif
 
 
 This seems to be the way many headers handle this.  Patch modules built
 against a kernel that doesn't support live patching will build cleanly,
 but will always fail to load.
 
 Seth

Hm, I would still vote for build failure. I think it doesn't make sense to 
build patch module against a kernel that doesn't support live patching and 
it is better to let the user know (and not potentially someone else who 
would load it and fail). Afaik the other headers handle it your way 
because otherwise the code would be spoiled by #ifdefs in .c files. 
However I think that our case is a bit different.

Anyway it is better to use #if (IS_ENABLED(CONFIG_LIVE_PATCHING)) than 
simple #ifdef (see Documentation/CodingStyle) and make the functions 
static inlined for !CONFIG_LIVE_PATCHING case.

Mira

  
   +/* TODO: add kernel-doc for structures once agreed upon */
   +
   +struct lp_func {
   + const char *old_name; /* function to be patched */
   + void *new_func; /* replacement function in patch module */
   + /*
   +  * The old_addr field is optional and can be used to resolve
   +  * duplicate symbol names in the vmlinux object.  If this
   +  * information is not present, the symbol is located by name
   +  * with kallsyms. If the name is not unique and old_addr is
   +  * not provided, the patch application fails as there is no
   +  * way to resolve the ambiguity.
   +  */
   + unsigned long old_addr;
   +};
   +
   +struct lp_reloc {
   + unsigned long dest;
   + unsigned long src;
   + unsigned long type;
   + const char *name;
   + int addend;
   + int external;
   +};
   +
   +struct lp_object {
   + const char *name; /* vmlinux or module name */
   + struct lp_func *funcs;
   + struct lp_reloc *relocs;
   +};
   +
   +struct lp_patch {
   + struct module *mod; /* module containing the patch */
   + struct lp_object *objs;
   +};
   +
   +int lp_register_patch(struct lp_patch *);
   +int lp_unregister_patch(struct lp_patch *);
   +int lp_enable_patch(struct lp_patch *);
   +int lp_disable_patch(struct lp_patch *);
   +
   +#include asm/livepatch.h
  
  and #endif for CONFIG_LIVE_PATCHING here.
  
   +
   +#endif /* 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Josh Poimboeuf]

On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
 +static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
 + void *data)
 +{
 + struct module *mod = data;
 + struct lpc_patch *patch;
 + struct lpc_object *obj;
 +
 + mutex_lock(lpc_mutex);
 +
 + if (action != MODULE_STATE_COMING  action != MODULE_STATE_GOING)
 + goto out;

I think we can get the mutex here instead of above so it doesn't block
other module actions (and then you can also get rid of the out label).

 +
 + list_for_each_entry(patch, lpc_patches, list) {
 + if (patch-state == LPC_DISABLED)
 + continue;
 + list_for_each_entry(obj, patch-objs, list) {
 + if (strcmp(obj-name, mod-name))
 + continue;
 + if (action == MODULE_STATE_COMING) {
 + obj-mod = mod;
 + lpc_module_notify_coming(patch-mod, obj);
 + } else /* MODULE_STATE_GOING */
 + lpc_module_notify_going(patch-mod, obj);
 + break;
 + }
 + }
 +out:
 + mutex_unlock(lpc_mutex);
 + return 0;
 +}

-- 
Josh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Thu, Nov 20, 2014 at 09:19:54AM -0600, Josh Poimboeuf wrote:
 On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
  +static int lpc_module_notify(struct notifier_block *nb, unsigned long 
  action,
  +   void *data)
  +{
  +   struct module *mod = data;
  +   struct lpc_patch *patch;
  +   struct lpc_object *obj;
  +
  +   mutex_lock(lpc_mutex);
  +
  +   if (action != MODULE_STATE_COMING  action != MODULE_STATE_GOING)
  +   goto out;
 
 I think we can get the mutex here instead of above so it doesn't block
 other module actions (and then you can also get rid of the out label).

Sure.

Thanks,
Seth

 
  +
  +   list_for_each_entry(patch, lpc_patches, list) {
  +   if (patch-state == LPC_DISABLED)
  +   continue;
  +   list_for_each_entry(obj, patch-objs, list) {
  +   if (strcmp(obj-name, mod-name))
  +   continue;
  +   if (action == MODULE_STATE_COMING) {
  +   obj-mod = mod;
  +   lpc_module_notify_coming(patch-mod, obj);
  +   } else /* MODULE_STATE_GOING */
  +   lpc_module_notify_going(patch-mod, obj);
  +   break;
  +   }
  +   }
  +out:
  +   mutex_unlock(lpc_mutex);
  +   return 0;
  +}
 
 -- 
 Josh
 --
 To unsubscribe from this list: send the line unsubscribe live-patching in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Josh Poimboeuf]

On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
 
 On Sun, 16 Nov 2014, Seth Jennings wrote:
 
  This commit introduces code for the live patching core.  It implements
  an ftrace-based mechanism and kernel interface for doing live patching
  of kernel and kernel module functions.
  
  It represents the greatest common functionality set between kpatch and
  kgraft and can accept patches built using either method.
  
  This first version does not implement any consistency mechanism that
  ensures that old and new code do not run together.  In practice, ~90% of
  CVEs are safe to apply in this way, since they simply add a conditional
  check.  However, any function change that can not execute safely with
  the old version of the function can _not_ be safely applied in this
  version.
  
  Signed-off-by: Seth Jennings sjenn...@redhat.com
 
 Hi,
 
 below is the patch which merges the internal and external data structures 
 (so it is only one part of our original patch for version 1). Apart from 
 that I tried to make minimal changes to the code. Only unnecessary 
 kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
 as it made more sense in this approach, I think.
 
 I hope this clearly shows our point of view stated previously. What do 
 you say?

Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
ok with this patch (though I do have a few comments below).

 Next, I'll look at the three level hierarchy and sysfs directory and see 
 if we can make it simpler yet keep its advantages.
 
 Regards,
 
 Miroslav Benes
 SUSE Labs
 
 -- 8 --
 From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
 From: Miroslav Benes mbe...@suse.cz
 Date: Wed, 19 Nov 2014 16:06:35 +0100
 Subject: [PATCH] Remove the data duplication in internal and public structures
 
 The split of internal and external structures is cleaner and makes the API 
 more
 stable. But it makes the code more complicated. It requires more space and 
 data
 copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
 causes confusion.
 
 The API is not a real issue for live patching. We take care neither of 
 backward
 nor forward compatibility. The dependency between a patch and kernel is even
 more strict than by version. They have to use the same configuration and the
 same build environment.
 
 This patch merge the external and internal structures into one.  The 
 structures
 are initialized using .item = value syntax. Therefore the API is basically 
 as
 stable as it was before. We could later even hide it under some helper macros
 if requested.
 
 For the purpose if this patch, we used the prefix lpc. It allows to make as
 less changes as possible and show the real effect. If the patch is accepted, 
 it
 would make sense to merge it into the original patch and even use another
 common prefix, for example the proposed klp.
 
 Signed-off-by: Miroslav Benes mbe...@suse.cz
 ---
  include/linux/livepatch.h |  47 +--
  kernel/livepatch/core.c   | 338 
 --
  2 files changed, 121 insertions(+), 264 deletions(-)
 
 diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
 index 8b68fef..f16de32 100644
 --- a/include/linux/livepatch.h
 +++ b/include/linux/livepatch.h
 @@ -21,10 +21,23 @@
  #define _LINUX_LIVEPATCH_H_
  
  #include linux/module.h
 +#include linux/ftrace.h
  
  /* TODO: add kernel-doc for structures once agreed upon */
  
 -struct lp_func {
 +enum lpc_state {
 + LPC_DISABLED,
 + LPC_ENABLED
 +};
 +
 +struct lpc_func {
 + /* internal */

Would it be a little clearer to list the external fields first?

 + struct kobject *kobj;
 + struct ftrace_ops fops;
 + enum lpc_state state;
 + unsigned long new_addr;

I think this internal new_addr field isn't really needed since we
already have the external new_func field.

 +
 + /* external */
   const char *old_name; /* function to be patched */
   void *new_func; /* replacement function in patch module */
   /*
 @@ -38,7 +51,7 @@ struct lp_func {
   unsigned long old_addr;
  };
  
 -struct lp_reloc {
 +struct lpc_reloc {
   unsigned long dest;
   unsigned long src;
   unsigned long type;
 @@ -47,21 +60,33 @@ struct lp_reloc {
   int external;
  };
  
 -struct lp_object {
 +struct lpc_object {
 + /* internal */
 + struct kobject *kobj;
 + struct module *mod; /* module associated with object */
 + enum lpc_state state;
 +
 + /* external */
   const char *name; /* vmlinux or module name */
 - struct lp_func *funcs;
 - struct lp_reloc *relocs;
 + struct lpc_reloc *relocs;
 + struct lpc_func *funcs;
  };
  
 -struct lp_patch {
 +struct lpc_patch {
 + /* internal */
 + struct list_head list;
 + struct kobject kobj;
 + enum lpc_state state;
 +
 + /* external */
   struct module *mod; /* module containing the patch */
 - struct lp_object 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Thu, Nov 20, 2014 at 11:35:52AM -0600, Josh Poimboeuf wrote:
 On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
  
  On Sun, 16 Nov 2014, Seth Jennings wrote:
  
   This commit introduces code for the live patching core.  It implements
   an ftrace-based mechanism and kernel interface for doing live patching
   of kernel and kernel module functions.
   
   It represents the greatest common functionality set between kpatch and
   kgraft and can accept patches built using either method.
   
   This first version does not implement any consistency mechanism that
   ensures that old and new code do not run together.  In practice, ~90% of
   CVEs are safe to apply in this way, since they simply add a conditional
   check.  However, any function change that can not execute safely with
   the old version of the function can _not_ be safely applied in this
   version.
   
   Signed-off-by: Seth Jennings sjenn...@redhat.com
  
  Hi,
  
  below is the patch which merges the internal and external data structures 
  (so it is only one part of our original patch for version 1). Apart from 
  that I tried to make minimal changes to the code. Only unnecessary 
  kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
  as it made more sense in this approach, I think.
  
  I hope this clearly shows our point of view stated previously. What do 
  you say?
 
 Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
 ok with this patch (though I do have a few comments below).

Thanks Josh :)

Miroslav, before you send out a revision on this patch, I'm merging it
for v3 right now.  I'll fixup any trivial fixes from this email.

I'm putting the finishing touches on v3 now.  Hopefully it will make
everyone happy, or happier, with your changes merged.  Should be getting
close...

Thanks,
Seth

 
  Next, I'll look at the three level hierarchy and sysfs directory and see 
  if we can make it simpler yet keep its advantages.
  
  Regards,
  
  Miroslav Benes
  SUSE Labs
  
  -- 8 --
  From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
  From: Miroslav Benes mbe...@suse.cz
  Date: Wed, 19 Nov 2014 16:06:35 +0100
  Subject: [PATCH] Remove the data duplication in internal and public 
  structures
  
  The split of internal and external structures is cleaner and makes the API 
  more
  stable. But it makes the code more complicated. It requires more space and 
  data
  copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
  causes confusion.
  
  The API is not a real issue for live patching. We take care neither of 
  backward
  nor forward compatibility. The dependency between a patch and kernel is even
  more strict than by version. They have to use the same configuration and the
  same build environment.
  
  This patch merge the external and internal structures into one.  The 
  structures
  are initialized using .item = value syntax. Therefore the API is 
  basically as
  stable as it was before. We could later even hide it under some helper 
  macros
  if requested.
  
  For the purpose if this patch, we used the prefix lpc. It allows to make 
  as
  less changes as possible and show the real effect. If the patch is 
  accepted, it
  would make sense to merge it into the original patch and even use another
  common prefix, for example the proposed klp.
  
  Signed-off-by: Miroslav Benes mbe...@suse.cz
  ---
   include/linux/livepatch.h |  47 +--
   kernel/livepatch/core.c   | 338 
  --
   2 files changed, 121 insertions(+), 264 deletions(-)
  
  diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
  index 8b68fef..f16de32 100644
  --- a/include/linux/livepatch.h
  +++ b/include/linux/livepatch.h
  @@ -21,10 +21,23 @@
   #define _LINUX_LIVEPATCH_H_
   
   #include linux/module.h
  +#include linux/ftrace.h
   
   /* TODO: add kernel-doc for structures once agreed upon */
   
  -struct lp_func {
  +enum lpc_state {
  +   LPC_DISABLED,
  +   LPC_ENABLED
  +};
  +
  +struct lpc_func {
  +   /* internal */
 
 Would it be a little clearer to list the external fields first?
 
  +   struct kobject *kobj;
  +   struct ftrace_ops fops;
  +   enum lpc_state state;
  +   unsigned long new_addr;
 
 I think this internal new_addr field isn't really needed since we
 already have the external new_func field.
 
  +
  +   /* external */
  const char *old_name; /* function to be patched */
  void *new_func; /* replacement function in patch module */
  /*
  @@ -38,7 +51,7 @@ struct lp_func {
  unsigned long old_addr;
   };
   
  -struct lp_reloc {
  +struct lpc_reloc {
  unsigned long dest;
  unsigned long src;
  unsigned long type;
  @@ -47,21 +60,33 @@ struct lp_reloc {
  int external;
   };
   
  -struct lp_object {
  +struct lpc_object {
  +   /* internal */
  +   struct kobject *kobj;
  +   struct module *mod; /* module associated with object */
  +   enum 

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Tue, Nov 18, 2014 at 03:45:22PM +0100, Miroslav Benes wrote:
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> [...]
> 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > new file mode 100644
> > index 000..8b68fef
> > --- /dev/null
> > +++ b/include/linux/livepatch.h
> > @@ -0,0 +1,68 @@
> > +/*
> > + * livepatch.h - Live Kernel Patching Core
> > + *
> > + * Copyright (C) 2014 Seth Jennings 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see .
> > + */
> > +
> > +#ifndef _LINUX_LIVEPATCH_H_
> > +#define _LINUX_LIVEPATCH_H_
> > +
> > +#include 
> > +
> 
> I think we need something like 
> 
> #if IS_ENABLED(CONFIG_LIVE_PATCHING)
> 
> here. Otherwise kernel module with live patch itself would be built 
> even with live patching support disabled (as the structures and needed 
> functions are declared).

What do you think of this (already includes s/lp/klp/ change)?


diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0143b73..a9821f3 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,6 +21,7 @@
 #define _LINUX_LIVEPATCH_H_
 
 #include 
+#include 
 
 /* TODO: add kernel-doc for structures once agreed upon */
 
@@ -58,11 +59,20 @@ struct klp_patch {
struct klp_object *objs;
 };
 
-int klp_register_patch(struct klp_patch *);
-int klp_unregister_patch(struct klp_patch *);
-int klp_enable_patch(struct klp_patch *);
-int klp_disable_patch(struct klp_patch *);
+#ifdef CONFIG_LIVE_PATCHING
 
-#include 
+extern int klp_register_patch(struct klp_patch *);
+extern int klp_unregister_patch(struct klp_patch *);
+extern int klp_enable_patch(struct klp_patch *);
+extern int klp_disable_patch(struct klp_patch *);
+
+#else /* !CONFIG_LIVE_PATCHING */
+
+static int klp_register_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_unregister_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_enable_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_disable_patch(struct klp_patch *k) { return -ENOSYS; }
+
+#endif


This seems to be the way many headers handle this.  Patch modules built
against a kernel that doesn't support live patching will build cleanly,
but will always fail to load.

Seth

> 
> > +/* TODO: add kernel-doc for structures once agreed upon */
> > +
> > +struct lp_func {
> > +   const char *old_name; /* function to be patched */
> > +   void *new_func; /* replacement function in patch module */
> > +   /*
> > +* The old_addr field is optional and can be used to resolve
> > +* duplicate symbol names in the vmlinux object.  If this
> > +* information is not present, the symbol is located by name
> > +* with kallsyms. If the name is not unique and old_addr is
> > +* not provided, the patch application fails as there is no
> > +* way to resolve the ambiguity.
> > +*/
> > +   unsigned long old_addr;
> > +};
> > +
> > +struct lp_reloc {
> > +   unsigned long dest;
> > +   unsigned long src;
> > +   unsigned long type;
> > +   const char *name;
> > +   int addend;
> > +   int external;
> > +};
> > +
> > +struct lp_object {
> > +   const char *name; /* "vmlinux" or module name */
> > +   struct lp_func *funcs;
> > +   struct lp_reloc *relocs;
> > +};
> > +
> > +struct lp_patch {
> > +   struct module *mod; /* module containing the patch */
> > +   struct lp_object *objs;
> > +};
> > +
> > +int lp_register_patch(struct lp_patch *);
> > +int lp_unregister_patch(struct lp_patch *);
> > +int lp_enable_patch(struct lp_patch *);
> > +int lp_disable_patch(struct lp_patch *);
> > +
> > +#include 
> 
> and #endif for CONFIG_LIVE_PATCHING here.
> 
> > +
> > +#endif /* _LINUX_LIVEPATCH_H_ */
> 
> Thanks,
> --
> Miroslav Benes
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Wed, Nov 19, 2014 at 04:27:39PM +0100, Miroslav Benes wrote:
> 
> Hi,
> 
> during rewriting our code I came across few more things. See below.
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> [...]
> 
> > +/**
> > + * module notifier
> > + */
> > +
> > +static void lpc_module_notify_coming(struct module *pmod,
> > +struct lpc_object *obj)
> > +{
> > +   struct module *mod = obj->mod;
> > +   int ret;
> > +
> > +   pr_notice("applying patch '%s' to loading module '%s'\n",
> > + mod->name, pmod->name);
> 
> This looks strange. I guess the arguments should be swapped.

Indeed, you are correct :)

> 
> > +   obj->mod = mod;
> 
> And this is redundant.

True again!

> 
> > +   ret = lpc_enable_object(pmod, obj);
> > +   if (ret)
> > +   pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > +   pmod->name, mod->name, ret);
> > +}
> > +
> > +static void lpc_module_notify_going(struct module *pmod,
> > +   struct lpc_object *obj)
> > +{
> > +   struct module *mod = obj->mod;
> > +   int ret;
> > +
> > +   pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > + pmod->name, mod->name);
> > +   ret = lpc_disable_object(obj);
> > +   if (ret)
> > +   pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> > +   pmod->name, mod->name, ret);
> > +   obj->mod = NULL;
> > +}
> > +
> > +static int lpc_module_notify(struct notifier_block *nb, unsigned long 
> > action,
> > +   void *data)
> > +{
> > +   struct module *mod = data;
> > +   struct lpc_patch *patch;
> > +   struct lpc_object *obj;
> > +
> > +   mutex_lock(_mutex);
> > +
> > +   if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> > +   goto out;
> > +
> > +   list_for_each_entry(patch, _patches, list) {
> > +   if (patch->state == LPC_DISABLED)
> > +   continue;
> > +   list_for_each_entry(obj, >objs, list) {
> > +   if (strcmp(obj->name, mod->name))
> > +   continue;
> > +   if (action == MODULE_STATE_COMING) {
> > +   obj->mod = mod;
> > +   lpc_module_notify_coming(patch->mod, obj);
> > +   } else /* MODULE_STATE_GOING */
> > +   lpc_module_notify_going(patch->mod, obj);
> > +   break;
> > +   }
> > +   }
> > +out:
> > +   mutex_unlock(_mutex);
> > +   return 0;
> > +}
> 
> [...]
> 
> > +static struct lpc_object *lpc_create_object(struct kobject *root,
> > +   struct lp_object *userobj)
> > +{
> > +   struct lpc_object *obj;
> > +   int ret;
> > +
> > +   /* alloc */
> > +   obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > +   if (!obj)
> > +   return NULL;
> > +
> > +   /* init */
> > +   INIT_LIST_HEAD(>list);
> > +   obj->name = userobj->name;
> > +   obj->relocs = userobj->relocs;
> > +   obj->state = LPC_DISABLED;
> > +   /* obj->mod set by lpc_object_module_get() */
> > +   INIT_LIST_HEAD(>funcs);
> 
> There is nothing like lpc_object_module_get() in the code. Did you mean 
> lpc_find_object_module()?

Yes, this comment should be removed or updated.

Thanks,
Seth

> 
> Thank you,
> --
> Miroslav Benes
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

Hi,

during rewriting our code I came across few more things. See below.

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

> +/**
> + * module notifier
> + */
> +
> +static void lpc_module_notify_coming(struct module *pmod,
> +  struct lpc_object *obj)
> +{
> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("applying patch '%s' to loading module '%s'\n",
> +   mod->name, pmod->name);

This looks strange. I guess the arguments should be swapped.

> + obj->mod = mod;

And this is redundant.

> + ret = lpc_enable_object(pmod, obj);
> + if (ret)
> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> +}
> +
> +static void lpc_module_notify_going(struct module *pmod,
> + struct lpc_object *obj)
> +{
> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +   pmod->name, mod->name);
> + ret = lpc_disable_object(obj);
> + if (ret)
> + pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> + obj->mod = NULL;
> +}
> +
> +static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> + struct lpc_patch *patch;
> + struct lpc_object *obj;
> +
> + mutex_lock(_mutex);
> +
> + if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> + goto out;
> +
> + list_for_each_entry(patch, _patches, list) {
> + if (patch->state == LPC_DISABLED)
> + continue;
> + list_for_each_entry(obj, >objs, list) {
> + if (strcmp(obj->name, mod->name))
> + continue;
> + if (action == MODULE_STATE_COMING) {
> + obj->mod = mod;
> + lpc_module_notify_coming(patch->mod, obj);
> + } else /* MODULE_STATE_GOING */
> + lpc_module_notify_going(patch->mod, obj);
> + break;
> + }
> + }
> +out:
> + mutex_unlock(_mutex);
> + return 0;
> +}

[...]

> +static struct lpc_object *lpc_create_object(struct kobject *root,
> + struct lp_object *userobj)
> +{
> + struct lpc_object *obj;
> + int ret;
> +
> + /* alloc */
> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> + if (!obj)
> + return NULL;
> +
> + /* init */
> + INIT_LIST_HEAD(>list);
> + obj->name = userobj->name;
> + obj->relocs = userobj->relocs;
> + obj->state = LPC_DISABLED;
> + /* obj->mod set by lpc_object_module_get() */
> + INIT_LIST_HEAD(>funcs);

There is nothing like lpc_object_module_get() in the code. Did you mean 
lpc_find_object_module()?

Thank you,
--
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

Hi,

during rewriting our code I came across few more things. See below.

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

 +/**
 + * module notifier
 + */
 +
 +static void lpc_module_notify_coming(struct module *pmod,
 +  struct lpc_object *obj)
 +{
 + struct module *mod = obj-mod;
 + int ret;
 +
 + pr_notice(applying patch '%s' to loading module '%s'\n,
 +   mod-name, pmod-name);

This looks strange. I guess the arguments should be swapped.

 + obj-mod = mod;

And this is redundant.

 + ret = lpc_enable_object(pmod, obj);
 + if (ret)
 + pr_warn(failed to apply patch '%s' to module '%s' (%d)\n,
 + pmod-name, mod-name, ret);
 +}
 +
 +static void lpc_module_notify_going(struct module *pmod,
 + struct lpc_object *obj)
 +{
 + struct module *mod = obj-mod;
 + int ret;
 +
 + pr_notice(reverting patch '%s' on unloading module '%s'\n,
 +   pmod-name, mod-name);
 + ret = lpc_disable_object(obj);
 + if (ret)
 + pr_warn(failed to revert patch '%s' on module '%s' (%d)\n,
 + pmod-name, mod-name, ret);
 + obj-mod = NULL;
 +}
 +
 +static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
 + void *data)
 +{
 + struct module *mod = data;
 + struct lpc_patch *patch;
 + struct lpc_object *obj;
 +
 + mutex_lock(lpc_mutex);
 +
 + if (action != MODULE_STATE_COMING  action != MODULE_STATE_GOING)
 + goto out;
 +
 + list_for_each_entry(patch, lpc_patches, list) {
 + if (patch-state == LPC_DISABLED)
 + continue;
 + list_for_each_entry(obj, patch-objs, list) {
 + if (strcmp(obj-name, mod-name))
 + continue;
 + if (action == MODULE_STATE_COMING) {
 + obj-mod = mod;
 + lpc_module_notify_coming(patch-mod, obj);
 + } else /* MODULE_STATE_GOING */
 + lpc_module_notify_going(patch-mod, obj);
 + break;
 + }
 + }
 +out:
 + mutex_unlock(lpc_mutex);
 + return 0;
 +}

[...]

 +static struct lpc_object *lpc_create_object(struct kobject *root,
 + struct lp_object *userobj)
 +{
 + struct lpc_object *obj;
 + int ret;
 +
 + /* alloc */
 + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
 + if (!obj)
 + return NULL;
 +
 + /* init */
 + INIT_LIST_HEAD(obj-list);
 + obj-name = userobj-name;
 + obj-relocs = userobj-relocs;
 + obj-state = LPC_DISABLED;
 + /* obj-mod set by lpc_object_module_get() */
 + INIT_LIST_HEAD(obj-funcs);

There is nothing like lpc_object_module_get() in the code. Did you mean 
lpc_find_object_module()?

Thank you,
--
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Wed, Nov 19, 2014 at 04:27:39PM +0100, Miroslav Benes wrote:
 
 Hi,
 
 during rewriting our code I came across few more things. See below.
 
 On Sun, 16 Nov 2014, Seth Jennings wrote:
 
 [...]
 
  +/**
  + * module notifier
  + */
  +
  +static void lpc_module_notify_coming(struct module *pmod,
  +struct lpc_object *obj)
  +{
  +   struct module *mod = obj-mod;
  +   int ret;
  +
  +   pr_notice(applying patch '%s' to loading module '%s'\n,
  + mod-name, pmod-name);
 
 This looks strange. I guess the arguments should be swapped.

Indeed, you are correct :)

 
  +   obj-mod = mod;
 
 And this is redundant.

True again!

 
  +   ret = lpc_enable_object(pmod, obj);
  +   if (ret)
  +   pr_warn(failed to apply patch '%s' to module '%s' (%d)\n,
  +   pmod-name, mod-name, ret);
  +}
  +
  +static void lpc_module_notify_going(struct module *pmod,
  +   struct lpc_object *obj)
  +{
  +   struct module *mod = obj-mod;
  +   int ret;
  +
  +   pr_notice(reverting patch '%s' on unloading module '%s'\n,
  + pmod-name, mod-name);
  +   ret = lpc_disable_object(obj);
  +   if (ret)
  +   pr_warn(failed to revert patch '%s' on module '%s' (%d)\n,
  +   pmod-name, mod-name, ret);
  +   obj-mod = NULL;
  +}
  +
  +static int lpc_module_notify(struct notifier_block *nb, unsigned long 
  action,
  +   void *data)
  +{
  +   struct module *mod = data;
  +   struct lpc_patch *patch;
  +   struct lpc_object *obj;
  +
  +   mutex_lock(lpc_mutex);
  +
  +   if (action != MODULE_STATE_COMING  action != MODULE_STATE_GOING)
  +   goto out;
  +
  +   list_for_each_entry(patch, lpc_patches, list) {
  +   if (patch-state == LPC_DISABLED)
  +   continue;
  +   list_for_each_entry(obj, patch-objs, list) {
  +   if (strcmp(obj-name, mod-name))
  +   continue;
  +   if (action == MODULE_STATE_COMING) {
  +   obj-mod = mod;
  +   lpc_module_notify_coming(patch-mod, obj);
  +   } else /* MODULE_STATE_GOING */
  +   lpc_module_notify_going(patch-mod, obj);
  +   break;
  +   }
  +   }
  +out:
  +   mutex_unlock(lpc_mutex);
  +   return 0;
  +}
 
 [...]
 
  +static struct lpc_object *lpc_create_object(struct kobject *root,
  +   struct lp_object *userobj)
  +{
  +   struct lpc_object *obj;
  +   int ret;
  +
  +   /* alloc */
  +   obj = kzalloc(sizeof(*obj), GFP_KERNEL);
  +   if (!obj)
  +   return NULL;
  +
  +   /* init */
  +   INIT_LIST_HEAD(obj-list);
  +   obj-name = userobj-name;
  +   obj-relocs = userobj-relocs;
  +   obj-state = LPC_DISABLED;
  +   /* obj-mod set by lpc_object_module_get() */
  +   INIT_LIST_HEAD(obj-funcs);
 
 There is nothing like lpc_object_module_get() in the code. Did you mean 
 lpc_find_object_module()?

Yes, this comment should be removed or updated.

Thanks,
Seth

 
 Thank you,
 --
 Miroslav Benes
 SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Tue, Nov 18, 2014 at 03:45:22PM +0100, Miroslav Benes wrote:
 
 On Sun, 16 Nov 2014, Seth Jennings wrote:
 
 [...]
 
  diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
  new file mode 100644
  index 000..8b68fef
  --- /dev/null
  +++ b/include/linux/livepatch.h
  @@ -0,0 +1,68 @@
  +/*
  + * livepatch.h - Live Kernel Patching Core
  + *
  + * Copyright (C) 2014 Seth Jennings sjenn...@redhat.com
  + *
  + * This program is free software; you can redistribute it and/or
  + * modify it under the terms of the GNU General Public License
  + * as published by the Free Software Foundation; either version 2
  + * of the License, or (at your option) any later version.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + *
  + * You should have received a copy of the GNU General Public License
  + * along with this program; if not, see http://www.gnu.org/licenses/.
  + */
  +
  +#ifndef _LINUX_LIVEPATCH_H_
  +#define _LINUX_LIVEPATCH_H_
  +
  +#include linux/module.h
  +
 
 I think we need something like 
 
 #if IS_ENABLED(CONFIG_LIVE_PATCHING)
 
 here. Otherwise kernel module with live patch itself would be built 
 even with live patching support disabled (as the structures and needed 
 functions are declared).

What do you think of this (already includes s/lp/klp/ change)?


diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0143b73..a9821f3 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,6 +21,7 @@
 #define _LINUX_LIVEPATCH_H_
 
 #include linux/module.h
+#include asm/livepatch.h
 
 /* TODO: add kernel-doc for structures once agreed upon */
 
@@ -58,11 +59,20 @@ struct klp_patch {
struct klp_object *objs;
 };
 
-int klp_register_patch(struct klp_patch *);
-int klp_unregister_patch(struct klp_patch *);
-int klp_enable_patch(struct klp_patch *);
-int klp_disable_patch(struct klp_patch *);
+#ifdef CONFIG_LIVE_PATCHING
 
-#include asm/livepatch.h
+extern int klp_register_patch(struct klp_patch *);
+extern int klp_unregister_patch(struct klp_patch *);
+extern int klp_enable_patch(struct klp_patch *);
+extern int klp_disable_patch(struct klp_patch *);
+
+#else /* !CONFIG_LIVE_PATCHING */
+
+static int klp_register_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_unregister_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_enable_patch(struct klp_patch *k) { return -ENOSYS; }
+static int klp_disable_patch(struct klp_patch *k) { return -ENOSYS; }
+
+#endif


This seems to be the way many headers handle this.  Patch modules built
against a kernel that doesn't support live patching will build cleanly,
but will always fail to load.

Seth

 
  +/* TODO: add kernel-doc for structures once agreed upon */
  +
  +struct lp_func {
  +   const char *old_name; /* function to be patched */
  +   void *new_func; /* replacement function in patch module */
  +   /*
  +* The old_addr field is optional and can be used to resolve
  +* duplicate symbol names in the vmlinux object.  If this
  +* information is not present, the symbol is located by name
  +* with kallsyms. If the name is not unique and old_addr is
  +* not provided, the patch application fails as there is no
  +* way to resolve the ambiguity.
  +*/
  +   unsigned long old_addr;
  +};
  +
  +struct lp_reloc {
  +   unsigned long dest;
  +   unsigned long src;
  +   unsigned long type;
  +   const char *name;
  +   int addend;
  +   int external;
  +};
  +
  +struct lp_object {
  +   const char *name; /* vmlinux or module name */
  +   struct lp_func *funcs;
  +   struct lp_reloc *relocs;
  +};
  +
  +struct lp_patch {
  +   struct module *mod; /* module containing the patch */
  +   struct lp_object *objs;
  +};
  +
  +int lp_register_patch(struct lp_patch *);
  +int lp_unregister_patch(struct lp_patch *);
  +int lp_enable_patch(struct lp_patch *);
  +int lp_disable_patch(struct lp_patch *);
  +
  +#include asm/livepatch.h
 
 and #endif for CONFIG_LIVE_PATCHING here.
 
  +
  +#endif /* _LINUX_LIVEPATCH_H_ */
 
 Thanks,
 --
 Miroslav Benes
 SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> new file mode 100644
> index 000..8b68fef
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,68 @@
> +/*
> + * livepatch.h - Live Kernel Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#ifndef _LINUX_LIVEPATCH_H_
> +#define _LINUX_LIVEPATCH_H_
> +
> +#include 
> +

I think we need something like 

#if IS_ENABLED(CONFIG_LIVE_PATCHING)

here. Otherwise kernel module with live patch itself would be built 
even with live patching support disabled (as the structures and needed 
functions are declared).

> +/* TODO: add kernel-doc for structures once agreed upon */
> +
> +struct lp_func {
> + const char *old_name; /* function to be patched */
> + void *new_func; /* replacement function in patch module */
> + /*
> +  * The old_addr field is optional and can be used to resolve
> +  * duplicate symbol names in the vmlinux object.  If this
> +  * information is not present, the symbol is located by name
> +  * with kallsyms. If the name is not unique and old_addr is
> +  * not provided, the patch application fails as there is no
> +  * way to resolve the ambiguity.
> +  */
> + unsigned long old_addr;
> +};
> +
> +struct lp_reloc {
> + unsigned long dest;
> + unsigned long src;
> + unsigned long type;
> + const char *name;
> + int addend;
> + int external;
> +};
> +
> +struct lp_object {
> + const char *name; /* "vmlinux" or module name */
> + struct lp_func *funcs;
> + struct lp_reloc *relocs;
> +};
> +
> +struct lp_patch {
> + struct module *mod; /* module containing the patch */
> + struct lp_object *objs;
> +};
> +
> +int lp_register_patch(struct lp_patch *);
> +int lp_unregister_patch(struct lp_patch *);
> +int lp_enable_patch(struct lp_patch *);
> +int lp_disable_patch(struct lp_patch *);
> +
> +#include 

and #endif for CONFIG_LIVE_PATCHING here.

> +
> +#endif /* _LINUX_LIVEPATCH_H_ */

Thanks,
--
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Tue, Nov 18, 2014 at 03:11:43PM +0100, Miroslav Benes wrote:
> 
> Hi,
> 
> thank you for the revision. I'll rebase our patches on top of that. Anyway 
> there is a small bug in a header file. See below.
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> [...]
> 
> > diff --git a/arch/x86/include/asm/livepatch.h 
> b/arch/x86/include/asm/livepatch.h
> > new file mode 100644
> > index 000..c5fab45
> > --- /dev/null
> > +++ b/arch/x86/include/asm/livepatch.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * livepatch.h - x86-specific Live Kernel Patching Core
> > + *
> > + * Copyright (C) 2014 Seth Jennings 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see .
> > + */
> > +
> > +#ifndef _ASM_X86_LIVEPATCH_H
> > +#define _ASM_X86_LIVEPATCH_H
> > +
> > +#include 
> > +
> > +#ifdef CONFIG_LIVE_PATCHING
> > +extern int lpc_write_module_reloc(struct module *mod, unsigned long 
> type,
> > +   unsigned long loc, unsigned long value);
> > +
> > +#else
> > +static int lpc_write_module_reloc(struct module *mod, unsigned long 
> type,
> > +   unsigned long loc, unsigned long value);
> > +{
> > + pr_err("Kernel does not support live patching\n");
> > + return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#endif /* _ASM_X86_LIVEPATCH_H */
> 
> There should not be a semicolon at the end of the function header in #else 
> branch.

Good catch.

Also, I'm adding "build with LIVE_PATCHING=n" as a test step :-/

Thanks,
Seth

> 
> Regards,
> 
> ---
> Miroslav Benes
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

Hi,

thank you for the revision. I'll rebase our patches on top of that. Anyway 
there is a small bug in a header file. See below.

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

> diff --git a/arch/x86/include/asm/livepatch.h 
b/arch/x86/include/asm/livepatch.h
> new file mode 100644
> index 000..c5fab45
> --- /dev/null
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -0,0 +1,38 @@
> +/*
> + * livepatch.h - x86-specific Live Kernel Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#ifndef _ASM_X86_LIVEPATCH_H
> +#define _ASM_X86_LIVEPATCH_H
> +
> +#include 
> +
> +#ifdef CONFIG_LIVE_PATCHING
> +extern int lpc_write_module_reloc(struct module *mod, unsigned long 
type,
> +   unsigned long loc, unsigned long value);
> +
> +#else
> +static int lpc_write_module_reloc(struct module *mod, unsigned long 
type,
> +   unsigned long loc, unsigned long value);
> +{
> + pr_err("Kernel does not support live patching\n");
> + return -ENOSYS;
> +}
> +#endif
> +
> +#endif /* _ASM_X86_LIVEPATCH_H */

There should not be a semicolon at the end of the function header in #else 
branch.

Regards,

---
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

 diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
 new file mode 100644
 index 000..8b68fef
 --- /dev/null
 +++ b/include/linux/livepatch.h
 @@ -0,0 +1,68 @@
 +/*
 + * livepatch.h - Live Kernel Patching Core
 + *
 + * Copyright (C) 2014 Seth Jennings sjenn...@redhat.com
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2
 + * of the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, see http://www.gnu.org/licenses/.
 + */
 +
 +#ifndef _LINUX_LIVEPATCH_H_
 +#define _LINUX_LIVEPATCH_H_
 +
 +#include linux/module.h
 +

I think we need something like 

#if IS_ENABLED(CONFIG_LIVE_PATCHING)

here. Otherwise kernel module with live patch itself would be built 
even with live patching support disabled (as the structures and needed 
functions are declared).

 +/* TODO: add kernel-doc for structures once agreed upon */
 +
 +struct lp_func {
 + const char *old_name; /* function to be patched */
 + void *new_func; /* replacement function in patch module */
 + /*
 +  * The old_addr field is optional and can be used to resolve
 +  * duplicate symbol names in the vmlinux object.  If this
 +  * information is not present, the symbol is located by name
 +  * with kallsyms. If the name is not unique and old_addr is
 +  * not provided, the patch application fails as there is no
 +  * way to resolve the ambiguity.
 +  */
 + unsigned long old_addr;
 +};
 +
 +struct lp_reloc {
 + unsigned long dest;
 + unsigned long src;
 + unsigned long type;
 + const char *name;
 + int addend;
 + int external;
 +};
 +
 +struct lp_object {
 + const char *name; /* vmlinux or module name */
 + struct lp_func *funcs;
 + struct lp_reloc *relocs;
 +};
 +
 +struct lp_patch {
 + struct module *mod; /* module containing the patch */
 + struct lp_object *objs;
 +};
 +
 +int lp_register_patch(struct lp_patch *);
 +int lp_unregister_patch(struct lp_patch *);
 +int lp_enable_patch(struct lp_patch *);
 +int lp_disable_patch(struct lp_patch *);
 +
 +#include asm/livepatch.h

and #endif for CONFIG_LIVE_PATCHING here.

 +
 +#endif /* _LINUX_LIVEPATCH_H_ */

Thanks,
--
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Miroslav Benes]

Hi,

thank you for the revision. I'll rebase our patches on top of that. Anyway 
there is a small bug in a header file. See below.

On Sun, 16 Nov 2014, Seth Jennings wrote:

[...]

 diff --git a/arch/x86/include/asm/livepatch.h 
b/arch/x86/include/asm/livepatch.h
 new file mode 100644
 index 000..c5fab45
 --- /dev/null
 +++ b/arch/x86/include/asm/livepatch.h
 @@ -0,0 +1,38 @@
 +/*
 + * livepatch.h - x86-specific Live Kernel Patching Core
 + *
 + * Copyright (C) 2014 Seth Jennings sjenn...@redhat.com
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2
 + * of the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, see http://www.gnu.org/licenses/.
 + */
 +
 +#ifndef _ASM_X86_LIVEPATCH_H
 +#define _ASM_X86_LIVEPATCH_H
 +
 +#include linux/module.h
 +
 +#ifdef CONFIG_LIVE_PATCHING
 +extern int lpc_write_module_reloc(struct module *mod, unsigned long 
type,
 +   unsigned long loc, unsigned long value);
 +
 +#else
 +static int lpc_write_module_reloc(struct module *mod, unsigned long 
type,
 +   unsigned long loc, unsigned long value);
 +{
 + pr_err(Kernel does not support live patching\n);
 + return -ENOSYS;
 +}
 +#endif
 +
 +#endif /* _ASM_X86_LIVEPATCH_H */

There should not be a semicolon at the end of the function header in #else 
branch.

Regards,

---
Miroslav Benes
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Tue, Nov 18, 2014 at 03:11:43PM +0100, Miroslav Benes wrote:
 
 Hi,
 
 thank you for the revision. I'll rebase our patches on top of that. Anyway 
 there is a small bug in a header file. See below.
 
 On Sun, 16 Nov 2014, Seth Jennings wrote:
 
 [...]
 
  diff --git a/arch/x86/include/asm/livepatch.h 
 b/arch/x86/include/asm/livepatch.h
  new file mode 100644
  index 000..c5fab45
  --- /dev/null
  +++ b/arch/x86/include/asm/livepatch.h
  @@ -0,0 +1,38 @@
  +/*
  + * livepatch.h - x86-specific Live Kernel Patching Core
  + *
  + * Copyright (C) 2014 Seth Jennings sjenn...@redhat.com
  + *
  + * This program is free software; you can redistribute it and/or
  + * modify it under the terms of the GNU General Public License
  + * as published by the Free Software Foundation; either version 2
  + * of the License, or (at your option) any later version.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + *
  + * You should have received a copy of the GNU General Public License
  + * along with this program; if not, see http://www.gnu.org/licenses/.
  + */
  +
  +#ifndef _ASM_X86_LIVEPATCH_H
  +#define _ASM_X86_LIVEPATCH_H
  +
  +#include linux/module.h
  +
  +#ifdef CONFIG_LIVE_PATCHING
  +extern int lpc_write_module_reloc(struct module *mod, unsigned long 
 type,
  +   unsigned long loc, unsigned long value);
  +
  +#else
  +static int lpc_write_module_reloc(struct module *mod, unsigned long 
 type,
  +   unsigned long loc, unsigned long value);
  +{
  + pr_err(Kernel does not support live patching\n);
  + return -ENOSYS;
  +}
  +#endif
  +
  +#endif /* _ASM_X86_LIVEPATCH_H */
 
 There should not be a semicolon at the end of the function header in #else 
 branch.

Good catch.

Also, I'm adding build with LIVE_PATCHING=n as a test step :-/

Thanks,
Seth

 
 Regards,
 
 ---
 Miroslav Benes
 SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Mon, Nov 17, 2014 at 10:45:58AM -0800, Greg KH wrote:
> On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
> > +#ifdef CONFIG_X86_32
> > +int lpc_write_module_reloc(struct module *mod, unsigned long type,
> > +  unsigned long loc, unsigned long value)
> > +{
> > +   pr_err("Live patching not supported on 32-bit x86\n");
> > +   return -ENOSYS;
> > +}
> 
> Why not just prevent the code from being built on x86_32 instead of
> putting this in the file?

Yep. Masami saw this too and recommended a ARCH_HAVE_LIVE_PATCHING flag
set by the archs that support it.  I'll make the change.

Thanks for the review!

Seth

> 
> thanks,
> 
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Greg KH]

On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
> +#ifdef CONFIG_X86_32
> +int lpc_write_module_reloc(struct module *mod, unsigned long type,
> +unsigned long loc, unsigned long value)
> +{
> + pr_err("Live patching not supported on 32-bit x86\n");
> + return -ENOSYS;
> +}

Why not just prevent the code from being built on x86_32 instead of
putting this in the file?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Greg KH]

On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
 +#ifdef CONFIG_X86_32
 +int lpc_write_module_reloc(struct module *mod, unsigned long type,
 +unsigned long loc, unsigned long value)
 +{
 + pr_err(Live patching not supported on 32-bit x86\n);
 + return -ENOSYS;
 +}

Why not just prevent the code from being built on x86_32 instead of
putting this in the file?

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Mon, Nov 17, 2014 at 10:45:58AM -0800, Greg KH wrote:
 On Sun, Nov 16, 2014 at 07:29:23PM -0600, Seth Jennings wrote:
  +#ifdef CONFIG_X86_32
  +int lpc_write_module_reloc(struct module *mod, unsigned long type,
  +  unsigned long loc, unsigned long value)
  +{
  +   pr_err(Live patching not supported on 32-bit x86\n);
  +   return -ENOSYS;
  +}
 
 Why not just prevent the code from being built on x86_32 instead of
 putting this in the file?

Yep. Masami saw this too and recommended a ARCH_HAVE_LIVE_PATCHING flag
set by the archs that support it.  I'll make the change.

Thanks for the review!

Seth

 
 thanks,
 
 greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/