From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755272AbcEEL5L (ORCPT ); Thu, 5 May 2016 07:57:11 -0400 Received: from mx2.suse.de ([195.135.220.15]:46336 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755137AbcEEL5J (ORCPT ); Thu, 5 May 2016 07:57:09 -0400 Date: Thu, 5 May 2016 13:57:01 +0200 From: Petr Mladek To: Josh Poimboeuf 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: <20160505115701.GY2749@pathway.suse.cz> References: <20160504144854.GT2749@pathway.suse.cz> <20160504175700.nizuvflyisfd3lks@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160504175700.nizuvflyisfd3lks@treble> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 2016-05-04 12:57:00, Josh Poimboeuf wrote: > 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. I have missed that the two commands are called with preemption disabled. So, I had the following crazy scenario in mind: CPU0 CPU1 klp_enable_patch() klp_target_state = KLP_PATCHED; for_each_task() set TIF_PENDING_PATCH # task 123 if (klp_patch_pending(current) klp_patch_task(current) clear TIF_PENDING_PATCH smp_rmb(); # switch to assembly of # klp_patch_task() mov klp_target_state, %r12 # interrupt and schedule # another task klp_reverse_transition(); klp_target_state = KLP_UNPATCHED; klt_try_to_complete_transition() task = 123; if (task->patch_state == klp_target_state; return 0; => task 123 is in target state and does not block conversion klp_complete_transition() # disable previous patch on the stack klp_disable_patch(); klp_target_state = KLP_UNPATCHED; # task 123 gets scheduled again lea %r12, task->patch_state => it happily stores an outdated state This is why the two functions should get called with preemption disabled. We should document it at least. I imagine that we will use them later also in another context and nobody will remember this crazy scenario. Well, even disabled preemption does not help. The process on CPU1 might be also interrupted by an NMI and do some long printk in it. IMHO, the only safe approach is to call klp_patch_task() only for "current" on a safe place. Then this race is harmless. The switch happen on a safe place, so that it does not matter into which state the process is switched. By other words, the task state might be updated only + by the task itself on a safe place + by other task when the updated on is sleeping on a safe place This should be well documented and the API should help to avoid a misuse. Best Regards, Petr