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 3r0QjF61jfzDq5s for ; Thu, 5 May 2016 03:57:05 +1000 (AEST) Date: Wed, 4 May 2016 12:57:00 -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: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Message-ID: <20160504175700.nizuvflyisfd3lks@treble> References: <20160504144854.GT2749@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <20160504144854.GT2749@pathway.suse.cz> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, May 04, 2016 at 04:48:54PM +0200, Petr Mladek wrote: > On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote: > > Change livepatch to use a basic per-task consistency model. This is the > > foundation which will eventually enable us to patch those ~10% of > > security patches which change function or data semantics. This is the > > biggest remaining piece needed to make livepatch more generally useful. > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > new file mode 100644 > > index 0000000..92819bb > > --- /dev/null > > +++ b/kernel/livepatch/transition.c > > +/* > > + * klp_patch_task() - change the patched state of a task > > + * @task: The task to change > > + * > > + * Switches the patched state of the task to the set of functions in the target > > + * patch state. > > + */ > > +void klp_patch_task(struct task_struct *task) > > +{ > > + clear_tsk_thread_flag(task, TIF_PATCH_PENDING); > > + > > + /* > > + * The corresponding write barriers are in klp_init_transition() and > > + * klp_reverse_transition(). See the comments there for an explanation. > > + */ > > + smp_rmb(); > > + > > + task->patch_state = klp_target_state; > > +} > > + > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > > index bd12c6c..60d633f 100644 > > --- a/kernel/sched/idle.c > > +++ b/kernel/sched/idle.c > > @@ -9,6 +9,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -266,6 +267,9 @@ static void cpu_idle_loop(void) > > > > sched_ttwu_pending(); > > schedule_preempt_disabled(); > > + > > + if (unlikely(klp_patch_pending(current))) > > + klp_patch_task(current); > > } > > Some more ideas from the world of crazy races. I was shaking my head > if this was safe or not. > > The problem might be if the task get rescheduled between the check > for the pending stuff or inside the klp_patch_task() function. > This will get even more important when we use this construct > on some more locations, e.g. in some kthreads. > > If the task is sleeping on this strange locations, it might assign > strange values on strange times. > > I think that it is safe only because it is called with the > 'current' parameter and on a safe locations. It means that > the result is always safe and consistent. Also we could assign > an outdated value only when sleeping between reading klp_target_state > and storing task->patch_state. But if anyone modified > klp_target_state at this point, he also set TIF_PENDING_PATCH, > so the change will not get lost. > > I think that we should document that klp_patch_func() must be > called only from a safe location from within the affected task. > > I even suggest to avoid misuse by removing the struct *task_struct > parameter. It should always be called with current. Would the race involve two tasks trying to call klp_patch_task() for the same task at the same time? If so I don't think that would be a problem since they would both write the same value for task->patch_state. (Sorry if I'm being slow, I think I've managed to reach my quota of hard thinking for the day and I don't exactly follow what the race would be.) -- Josh