Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755148AbbBJLRK (ORCPT ); Tue, 10 Feb 2015 06:17:10 -0500 Received: from mail7.hitachi.co.jp ([133.145.228.42]:50301 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750704AbbBJLRH (ORCPT ); Tue, 10 Feb 2015 06:17:07 -0500 Message-ID: <54D9E8AB.3070800@hitachi.com> Date: Tue, 10 Feb 2015 20:16:59 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Josh Poimboeuf Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 0/9] livepatch: consistency model References: In-Reply-To: Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6432 Lines: 152 (2015/02/10 2:31), Josh Poimboeuf wrote: > This patch set implements a livepatch consistency model, targeted for 3.21. > Now that we have a solid livepatch code base, this is the biggest remaining > missing piece. > > This code stems from the design proposal made by Vojtech [1] in November. It > makes live patching safer in general. Specifically, it allows you to apply > patches which change function prototypes. It also lays the groundwork for > future code changes which will enable data and data semantic changes. Interesting, How would you do that? > It's basically a hybrid of kpatch and kGraft, combining kpatch's backtrace > checking with kGraft's per-task consistency. When patching, tasks are > carefully transitioned from the old universe to the new universe. A task can > only be switched to the new universe if it's not using a function that is to be > patched or unpatched. After all tasks have moved to the new universe, the > patching process is complete. > > How it transitions various tasks to the new universe: > > - The stacks of all sleeping tasks are checked. Each task that is not sleeping > on a to-be-patched function is switched. > > - Other user tasks are handled by do_notify_resume() (see patch 9/9). If a > task is I/O bound, it switches universes when returning from a system call. > If it's CPU bound, it switches when returning from an interrupt. If it's > sleeping on a patched function, the user can send SIGSTOP and SIGCONT to > force it to switch upon return from the signal handler. Ah, OK. So you can handle those without hooking switch_to :) > > - Idle "swapper" tasks which are sleeping on a to-be-patched function can be > switched from within the outer idle loop. > > - An interrupt handler will inherit the universe of the task it interrupts. > > - kthreads which are sleeping on to-be-patched functions are not yet handled > (more on this below). > > > I think this approach provides the best benefits of both kpatch and kGraft: > > advantages vs kpatch: > - no stop machine latency Good! :) > - higher patch success rate (can patch in-use functions) > - patching failures are more predictable (primary failure mode is attempting to > patch a kthread which is sleeping forever on a patched function, more on this > below) > > advantages vs kGraft: > - less code complexity (don't have to hack up the code of all the different > kthreads) > - less impact to processes (don't have to signal all sleeping tasks) > > disadvantages vs kpatch: > - no system-wide switch point (not really a functional limitation, just forces > the patch author to be more careful. but that's probably a good thing anyway) OK, we must check carefully that the old function and new function can be co-exist. > My biggest concerns and questions related to this patch set are: > > 1) To safely examine the task stacks, the transition code locks each task's rq > struct, which requires using the scheduler's internal rq locking functions. > It seems to work well, but I'm not sure if there's a cleaner way to safely > do stack checking without stop_machine(). We'd better ask scheduler people. > > 2) As mentioned above, kthreads which are always sleeping on a patched function > will never transition to the new universe. This is really a minor issue > (less than 1% of patches). It's not necessarily something that needs to be > resolved with this patch set, but it would be good to have some discussion > about it regardless. > > To overcome this issue, I have 1/2 an idea: we could add some stack checking > code to the ftrace handler itself to transition the kthread to the new > universe after it re-enters the function it was originally sleeping on, if > the stack doesn't already have have any other to-be-patched functions. > Combined with the klp_transition_work_fn()'s periodic stack checking of > sleeping tasks, that would handle most of the cases (except when trying to > patch the high-level thread_fn itself). It makes sense to me. (I just did similar thing) > > But then how do you make the kthread wake up? As far as I can tell, > wake_up_process() doesn't seem to work on a kthread (unless I messed up my > testing somehow). What does kGraft do in this case? Hmm, at a glance, the code itself can work on kthread too... Maybe you can also send you testing patch too. Thank you, > > > [1] https://lkml.org/lkml/2014/11/7/354 > > > Josh Poimboeuf (9): > livepatch: simplify disable error path > livepatch: separate enabled and patched states > livepatch: move patching functions into patch.c > livepatch: get function sizes > sched: move task rq locking functions to sched.h > livepatch: create per-task consistency model > proc: add /proc//universe to show livepatch status > livepatch: allow patch modules to be removed > livepatch: update task universe when exiting kernel > > arch/x86/include/asm/thread_info.h | 4 +- > arch/x86/kernel/signal.c | 4 + > fs/proc/base.c | 11 ++ > include/linux/livepatch.h | 38 ++-- > include/linux/sched.h | 3 + > kernel/fork.c | 2 + > kernel/livepatch/Makefile | 2 +- > kernel/livepatch/core.c | 360 ++++++++++--------------------------- > kernel/livepatch/patch.c | 206 +++++++++++++++++++++ > kernel/livepatch/patch.h | 26 +++ > kernel/livepatch/transition.c | 318 ++++++++++++++++++++++++++++++++ > kernel/livepatch/transition.h | 16 ++ > kernel/sched/core.c | 34 +--- > kernel/sched/idle.c | 4 + > kernel/sched/sched.h | 33 ++++ > 15 files changed, 747 insertions(+), 314 deletions(-) > create mode 100644 kernel/livepatch/patch.c > create mode 100644 kernel/livepatch/patch.h > create mode 100644 kernel/livepatch/transition.c > create mode 100644 kernel/livepatch/transition.h > -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/