On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 782fbb5..b3b8639 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -29,6 +29,7 @@
>  #include <linux/bug.h>
>  #include <linux/printk.h>
>  #include "patch.h"
> +#include "transition.h"
>  
>  static LIST_HEAD(klp_ops);
>  
> @@ -58,11 +59,42 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>       ops = container_of(fops, struct klp_ops, fops);
>  
>       rcu_read_lock();
> +
>       func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
>                                     stack_node);
> -     if (WARN_ON_ONCE(!func))
> +
> +     if (!func)
>               goto unlock;
>  
> +     /*
> +      * See the comment for the 2nd smp_wmb() in klp_init_transition() for
> +      * an explanation of why this read barrier is needed.
> +      */
> +     smp_rmb();
> +
> +     if (unlikely(func->transition)) {
> +
> +             /*
> +              * See the comment for the 1st smp_wmb() in
> +              * klp_init_transition() for an explanation of why this read
> +              * barrier is needed.
> +              */
> +             smp_rmb();

I would add here:

                WARN_ON_ONCE(current->patch_state == KLP_UNDEFINED);

We do not know in which context this is called, so the printk's are
not ideal. But it will get triggered only if there is a bug in
the livepatch implementation. It should happen on random locations
and rather early when a bug is introduced.

Anyway, better to die and catch the bug that let the system running
in an undefined state and produce cryptic errors later on.


> +             if (current->patch_state == KLP_UNPATCHED) {
> +                     /*
> +                      * Use the previously patched version of the function.
> +                      * If no previous patches exist, use the original
> +                      * function.
> +                      */
> +                     func = list_entry_rcu(func->stack_node.next,
> +                                           struct klp_func, stack_node);
> +
> +                     if (&func->stack_node == &ops->func_stack)
> +                             goto unlock;
> +             }
> +     }

I am staring into the code for too long now. I need to step back for a
while. I'll do another look when you send the next version. Anyway,
you did a great work. I speak mainly for the livepatch part and
I like it.

Best Regards,
Petr
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to