From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3r1WgL70S8zDqCJ for ; Fri, 6 May 2016 22:44:14 +1000 (AEST) Date: Fri, 6 May 2016 07:44:10 -0500 From: Josh Poimboeuf To: Petr Mladek Cc: Jessica Yu , Jiri Kosina , Miroslav Benes , Ingo Molnar , Peter Zijlstra , Michael Ellerman , Heiko Carstens , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, Vojtech Pavlik , Jiri Slaby , Chris J Arges , Andy Lutomirski Subject: Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Message-ID: <20160506124410.cljepquopw73cong@treble> References: <20160506113301.GB2749@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <20160506113301.GB2749@pathway.suse.cz> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Fri, May 06, 2016 at 01:33:01PM +0200, Petr Mladek wrote: > 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 > > #include > > #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. Ok, makes sense. > > + 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. Thanks for the helpful reviews! I'll be on vacation again next week so I get a break too :-) -- Josh