Re: [2/5] livepatch: Allow architectures to specify an alternate ftrace location

[Michael Ellerman]

On Wed, 2016-13-04 at 12:53:20 UTC, Michael Ellerman wrote:
> When livepatch tries to patch a function it takes the function address
> and asks ftrace to install the livepatch handler at that location.
> ftrace will look for an mcount call site at that exact address.
> 
> On powerpc the mcount location is not the first instruction of the
> function, and in fact it's not at a constant offset from the start of
> the function. To accommodate this add a hook which arch code can
> override to customise the behaviour.
> 
> Signed-off-by: Torsten Duwe 
> Signed-off-by: Balbir Singh 
> Signed-off-by: Petr Mladek 
> Signed-off-by: Michael Ellerman 

Applied to powerpc next.

https://git.kernel.org/powerpc/c/28e7cbd3e0f5fefec892842d13

cheers
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/5] livepatch: Allow architectures to specify an alternate ftrace location

[Miroslav Benes]

On Thu, 14 Apr 2016, Michael Ellerman wrote:

> On Thu, 2016-04-14 at 14:01 +0200, Miroslav Benes wrote:
> > On Wed, 13 Apr 2016, Michael Ellerman wrote:
> 
> > >  static void klp_disable_func(struct klp_func *func)
> > >  {
> > >   struct klp_ops *ops;
> > > @@ -312,8 +325,14 @@ static void klp_disable_func(struct klp_func *func)
> > >   return;
> > >  
> > >   if (list_is_singular(>func_stack)) {
> > > + unsigned long ftrace_loc;
> > 
> > This is a nit, but could you move the definition up to have them all in 
> > one place to be consistent with the rest of the code? The same applies to 
> > klp_enable_func() below.
> 
> Hmm, actually I moved it in there because you pointed out we only needed it
> inside the if:
> 
> http://lkml.kernel.org/r/alpine.lnx.2.00.1603151113020.20...@pobox.suse.cz
> 
> Thinking about it, we need ftrace_loc only in cases where we call 
> ftrace_set_filter_ip() right? So we can move klp_get_ftrace_location() 
> call to appropriate if branch both in klp_disable_func() and 
> klp_enable_func().
> 
> But I guess you meant the function call, not the variable declaration.

Exactly.

> Personally I think it's better this way, as the variable is in scope for the
> shortest possible amount of time, but I can change it if you want me to.

No, it is nothing I would insist on.

Thanks,
Miroslav
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/5] livepatch: Allow architectures to specify an alternate ftrace location

[Michael Ellerman]

On Thu, 2016-04-14 at 14:01 +0200, Miroslav Benes wrote:
> On Wed, 13 Apr 2016, Michael Ellerman wrote:
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index d68fbf63b083..b0476bb30f92 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -298,6 +298,19 @@ unlock:
> >   rcu_read_unlock();
> >  }
> >  
> > +/*
> > + * Convert a function address into the appropriate ftrace location.
> > + *
> > + * Usually this is just the address of the function, but on some 
> > architectures
> > + * it's more complicated so allow them to provide a custom behaviour.
> > + */
> > +#ifndef klp_get_ftrace_location
> > +static unsigned long klp_get_ftrace_location(unsigned long faddr)
> > +{
> > + return faddr;
> > +}
> > +#endif
> 
> Whoah, what an ugly hack :)
 
Hey it's a "cool trick" ;)

> Note to my future self - This is what you want to do if you need a weak 
> static inline function.
> 
> static inline is probably unnecessary here so __weak function would be 
> enough. It would introduce powerpc-specific livepatch.c though because of 
> it and this is not worth it.

Yeah that was my logic, at least for now. We can always change it in future
to be weak if anyone cares deeply.

> >  static void klp_disable_func(struct klp_func *func)
> >  {
> >   struct klp_ops *ops;
> > @@ -312,8 +325,14 @@ static void klp_disable_func(struct klp_func *func)
> >   return;
> >  
> >   if (list_is_singular(>func_stack)) {
> > + unsigned long ftrace_loc;
> 
> This is a nit, but could you move the definition up to have them all in 
> one place to be consistent with the rest of the code? The same applies to 
> klp_enable_func() below.

Hmm, actually I moved it in there because you pointed out we only needed it
inside the if:

http://lkml.kernel.org/r/alpine.lnx.2.00.1603151113020.20...@pobox.suse.cz

Thinking about it, we need ftrace_loc only in cases where we call 
ftrace_set_filter_ip() right? So we can move klp_get_ftrace_location() 
call to appropriate if branch both in klp_disable_func() and 
klp_enable_func().

But I guess you meant the function call, not the variable declaration.

Personally I think it's better this way, as the variable is in scope for the
shortest possible amount of time, but I can change it if you want me to.

cheers

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/5] livepatch: Allow architectures to specify an alternate ftrace location

[Miroslav Benes]

On Wed, 13 Apr 2016, Michael Ellerman wrote:

> When livepatch tries to patch a function it takes the function address
> and asks ftrace to install the livepatch handler at that location.
> ftrace will look for an mcount call site at that exact address.
> 
> On powerpc the mcount location is not the first instruction of the
> function, and in fact it's not at a constant offset from the start of
> the function. To accommodate this add a hook which arch code can
> override to customise the behaviour.
> 
> Signed-off-by: Torsten Duwe 
> Signed-off-by: Balbir Singh 
> Signed-off-by: Petr Mladek 
> Signed-off-by: Michael Ellerman 
> ---
>  kernel/livepatch/core.c | 34 +++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d68fbf63b083..b0476bb30f92 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -298,6 +298,19 @@ unlock:
>   rcu_read_unlock();
>  }
>  
> +/*
> + * Convert a function address into the appropriate ftrace location.
> + *
> + * Usually this is just the address of the function, but on some 
> architectures
> + * it's more complicated so allow them to provide a custom behaviour.
> + */
> +#ifndef klp_get_ftrace_location
> +static unsigned long klp_get_ftrace_location(unsigned long faddr)
> +{
> + return faddr;
> +}
> +#endif

Whoah, what an ugly hack :)

Note to my future self - This is what you want to do if you need a weak 
static inline function.

static inline is probably unnecessary here so __weak function would be 
enough. It would introduce powerpc-specific livepatch.c though because of 
it and this is not worth it.

>  static void klp_disable_func(struct klp_func *func)
>  {
>   struct klp_ops *ops;
> @@ -312,8 +325,14 @@ static void klp_disable_func(struct klp_func *func)
>   return;
>  
>   if (list_is_singular(>func_stack)) {
> + unsigned long ftrace_loc;

This is a nit, but could you move the definition up to have them all in 
one place to be consistent with the rest of the code? The same applies to 
klp_enable_func() below.

> +
> + ftrace_loc = klp_get_ftrace_location(func->old_addr);
> + if (WARN_ON(!ftrace_loc))
> + return;
> +
>   WARN_ON(unregister_ftrace_function(>fops));
> - WARN_ON(ftrace_set_filter_ip(>fops, func->old_addr, 1, 0));
> + WARN_ON(ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0));
>  
>   list_del_rcu(>stack_node);
>   list_del(>node);
> @@ -338,6 +357,15 @@ static int klp_enable_func(struct klp_func *func)
>  
>   ops = klp_find_ops(func->old_addr);
>   if (!ops) {
> + unsigned long ftrace_loc;

Here.

> +
> + ftrace_loc = klp_get_ftrace_location(func->old_addr);
> + if (!ftrace_loc) {
> + pr_err("failed to find location for function '%s'\n",
> + func->old_name);
> + return -EINVAL;
> + }
> +
>   ops = kzalloc(sizeof(*ops), GFP_KERNEL);
>   if (!ops)
>   return -ENOMEM;
> @@ -352,7 +380,7 @@ static int klp_enable_func(struct klp_func *func)
>   INIT_LIST_HEAD(>func_stack);
>   list_add_rcu(>stack_node, >func_stack);
>  
> - ret = ftrace_set_filter_ip(>fops, func->old_addr, 0, 0);
> + ret = ftrace_set_filter_ip(>fops, ftrace_loc, 0, 0);
>   if (ret) {
>   pr_err("failed to set ftrace filter for function '%s' 
> (%d)\n",
>  func->old_name, ret);
> @@ -363,7 +391,7 @@ static int klp_enable_func(struct klp_func *func)
>   if (ret) {
>   pr_err("failed to register ftrace handler for function 
> '%s' (%d)\n",
>  func->old_name, ret);
> - ftrace_set_filter_ip(>fops, func->old_addr, 1, 0);
> + ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0);
>   goto err;
>   }

Otherwise it is ok.

Miroslav
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/5] livepatch: Allow architectures to specify an alternate ftrace location

[Michael Ellerman]

When livepatch tries to patch a function it takes the function address
and asks ftrace to install the livepatch handler at that location.
ftrace will look for an mcount call site at that exact address.

On powerpc the mcount location is not the first instruction of the
function, and in fact it's not at a constant offset from the start of
the function. To accommodate this add a hook which arch code can
override to customise the behaviour.

Signed-off-by: Torsten Duwe 
Signed-off-by: Balbir Singh 
Signed-off-by: Petr Mladek 
Signed-off-by: Michael Ellerman 
---
 kernel/livepatch/core.c | 34 +++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d68fbf63b083..b0476bb30f92 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -298,6 +298,19 @@ unlock:
rcu_read_unlock();
 }
 
+/*
+ * Convert a function address into the appropriate ftrace location.
+ *
+ * Usually this is just the address of the function, but on some architectures
+ * it's more complicated so allow them to provide a custom behaviour.
+ */
+#ifndef klp_get_ftrace_location
+static unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+   return faddr;
+}
+#endif
+
 static void klp_disable_func(struct klp_func *func)
 {
struct klp_ops *ops;
@@ -312,8 +325,14 @@ static void klp_disable_func(struct klp_func *func)
return;
 
if (list_is_singular(>func_stack)) {
+   unsigned long ftrace_loc;
+
+   ftrace_loc = klp_get_ftrace_location(func->old_addr);
+   if (WARN_ON(!ftrace_loc))
+   return;
+
WARN_ON(unregister_ftrace_function(>fops));
-   WARN_ON(ftrace_set_filter_ip(>fops, func->old_addr, 1, 0));
+   WARN_ON(ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0));
 
list_del_rcu(>stack_node);
list_del(>node);
@@ -338,6 +357,15 @@ static int klp_enable_func(struct klp_func *func)
 
ops = klp_find_ops(func->old_addr);
if (!ops) {
+   unsigned long ftrace_loc;
+
+   ftrace_loc = klp_get_ftrace_location(func->old_addr);
+   if (!ftrace_loc) {
+   pr_err("failed to find location for function '%s'\n",
+   func->old_name);
+   return -EINVAL;
+   }
+
ops = kzalloc(sizeof(*ops), GFP_KERNEL);
if (!ops)
return -ENOMEM;
@@ -352,7 +380,7 @@ static int klp_enable_func(struct klp_func *func)
INIT_LIST_HEAD(>func_stack);
list_add_rcu(>stack_node, >func_stack);
 
-   ret = ftrace_set_filter_ip(>fops, func->old_addr, 0, 0);
+   ret = ftrace_set_filter_ip(>fops, ftrace_loc, 0, 0);
if (ret) {
pr_err("failed to set ftrace filter for function '%s' 
(%d)\n",
   func->old_name, ret);
@@ -363,7 +391,7 @@ static int klp_enable_func(struct klp_func *func)
if (ret) {
pr_err("failed to register ftrace handler for function 
'%s' (%d)\n",
   func->old_name, ret);
-   ftrace_set_filter_ip(>fops, func->old_addr, 1, 0);
+   ftrace_set_filter_ip(>fops, ftrace_loc, 1, 0);
goto err;
}
 
-- 
2.5.0

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev