From: Miroslav Benes <mbenes@suse.cz>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jiri Kosina <jikos@kernel.org>, Jessica Yu <jeyu@redhat.com>,
linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
Vojtech Pavlik <vojtech@suse.com>
Subject: Re: [RFC PATCH v1.9 12/14] livepatch: create per-task consistency model
Date: Thu, 14 Apr 2016 11:25:18 +0200 (CEST) [thread overview]
Message-ID: <alpine.LNX.2.00.1604141102400.10605@pobox.suse.cz> (raw)
In-Reply-To: <74a2b37cea7a64a185e50876dba031137aa59a24.1458933243.git.jpoimboe@redhat.com>
On Fri, 25 Mar 2016, Josh Poimboeuf wrote:
> Add 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 prototypes and/or data semantics.
>
> When a patch is enabled, livepatch enters into a transition state where
> tasks are converging from the old universe to the new universe. If a
> given task isn't using any of the patched functions, it's switched to
> the new universe. Once all the tasks have been converged to the new
> universe, patching is complete.
>
> The same sequence occurs when a patch is disabled, except the tasks
> converge from the new universe to the old universe.
>
> The /sys/kernel/livepatch/<patch>/transition file shows whether a patch
> is in transition. Only a single patch (the topmost patch on the stack)
> can be in transition at a given time. A patch can remain in the
> transition state indefinitely, if any of the tasks are stuck in the
> previous universe.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch/<patch>/enabled file while
> the transition is in progress. Then all the tasks will attempt to
> converge back to the original universe.
So after spending some time with this patch I must say hats off to you. I
haven't managed to find anything serious yet and I doubt I would. Few
things below.
> +/*
> + * klp_update_task_universe() - change the patched state of a task
> + * @task: The task to change
> + *
> + * Converts the patched state of the task so that it will switch to the set of
> + * functions in the goal universe.
> + */
> +static inline void klp_update_task_universe(struct task_struct *task)
> +{
> + /*
> + * The corresponding write barriers are in klp_init_transition() and
> + * klp_start_transition(). See the comments there for an explanation.
> + */
> + smp_rmb();
> +
> + task->klp_universe = klp_universe_goal;
> +}
I wonder whether we should introduce a static_key for this function.
klp_update_task_universe() is called in some important code paths and it
is called everytime if CONFIG_LIVEPATCH is on. It does not matter much for
syscall paths, because we enter slowpath there only if TIF_KLP_NEED_UPDATE
is set and that is set iff patching is in progress. But we call the
function also elsewhere. So maybe it would be nice to introduce static_key
in the function which would be on if the patching is in progress and off
if not.
Maybe it is just an overoptimization though.
[...]
> +static bool klp_try_switch_task(struct task_struct *task)
> +{
> + struct rq *rq;
> + unsigned long flags;
> + int ret;
> + bool success = false;
> +
> + /* check if this task has already switched over */
> + if (task->klp_universe == klp_universe_goal)
> + return true;
[...]
> +/*
> + * This function can be called in the middle of an existing transition to
> + * reverse the direction of the universe goal. This can be done to effectively
> + * cancel an existing enable or disable operation if there are any tasks which
> + * are stuck in the starting universe.
> + */
> +void klp_reverse_transition(void)
> +{
> + struct klp_patch *patch = klp_transition_patch;
> +
> + klp_start_transition(!klp_universe_goal);
> + klp_try_complete_transition();
> +
> + patch->enabled = !patch->enabled;
> +}
This function could be called iff the patching is in progress, there are
some not-yet-migrated tasks and we wait for scheduled workqueue to run
klp_try_complete_transition() again, right? I have two questions.
1. Is the call to klp_try_complete_transition() here necessary? It would
be called via workqueue, wouldn't it? I suspect we don't gain much with
this and we introduce possible future problems because of "double
success" of klp_try_complete_transition() (nothing serious there as of
now though). But I might be wrong because I got really confused by this
piece of code and its context :)
2. klp_reverse_transition() works well because not-yet-migrated tasks are
still in KLP_UNIVERSE_OLD (without loss of generality) and since
klp_universe_goal is swapped they are in fact in their target universe.
klp_try_switch_task() returns immediately in such case. The rest is
migrated in an ordinary way.
What about TIF_KLP_NEED_UPDATE (I know it is introduced in later patch but
it is easier to discuss it here)? klp_start_transition() resets the flag
for all the task. Shouldn't the flag be cleared for the task in
klp_try_switch_task() if task->klp_universe == klp_universe_goal? In fact
it does not matter if it is set. The patching success is determined by
klp_func->klp_universe booleans. But those tasks would needlessly go
through syscall slowpaths (see note above about static_key) which could
hurt performance.
Does this make sense?
[...]
transition.h:
>
> +extern void klp_init_transition(struct klp_patch *patch, int universe);
> +extern void klp_start_transition(int universe);
> +extern void klp_reverse_transition(void);
> +extern bool klp_try_complete_transition(void);
> +extern void klp_complete_transition(void);
externs are superfluous here and we do not have them in other header
files.
Anyway, great job!
Miroslav
next prev parent reply other threads:[~2016-04-14 9:25 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-25 19:34 [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 01/14] x86/asm/head: cleanup initial stack variable Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 02/14] x86/asm/head: use a common function for starting CPUs Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 03/14] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 04/14] x86: move _stext marker before head code Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a task has been preempted Josh Poimboeuf
2016-04-06 13:06 ` Petr Mladek
2016-04-06 16:33 ` Josh Poimboeuf
2016-04-07 9:47 ` Petr Mladek
2016-04-07 14:34 ` Josh Poimboeuf
2016-04-08 8:07 ` Petr Mladek
2016-04-08 14:34 ` Josh Poimboeuf
2016-04-07 21:15 ` Jessica Yu
2016-04-07 21:37 ` Jiri Kosina
2016-04-07 22:35 ` Josh Poimboeuf
2016-04-07 22:53 ` Jiri Kosina
2016-04-07 23:15 ` Jessica Yu
2016-04-08 7:05 ` Jiri Kosina
2016-04-08 8:03 ` Petr Mladek
2016-04-08 14:31 ` Josh Poimboeuf
2016-04-11 8:38 ` Petr Mladek
2016-03-25 19:34 ` [RFC PATCH v1.9 06/14] x86: add error handling to dump_trace() Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces Josh Poimboeuf
2016-03-31 13:03 ` Miroslav Benes
2016-04-04 17:54 ` Josh Poimboeuf
2016-04-11 14:16 ` Jiri Slaby
2016-04-12 15:56 ` Josh Poimboeuf
2016-06-09 8:31 ` Jiri Slaby
2016-06-13 21:58 ` Josh Poimboeuf
2016-12-01 20:28 ` Jiri Slaby
2016-12-01 20:59 ` Josh Poimboeuf
2017-01-17 13:08 ` Jiri Slaby
2016-04-04 15:55 ` Petr Mladek
2016-04-04 17:58 ` Josh Poimboeuf
2016-04-07 11:55 ` Petr Mladek
2016-04-07 14:46 ` Josh Poimboeuf
2016-04-08 8:24 ` Petr Mladek
2016-04-11 3:29 ` Jessica Yu
2016-03-25 19:34 ` [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states Josh Poimboeuf
2016-04-11 3:31 ` Jessica Yu
2016-04-12 14:44 ` [RFC PATCH v1.9 08/14] " Chris J Arges
2016-04-12 17:16 ` Josh Poimboeuf
2016-04-12 17:35 ` Chris J Arges
2016-04-12 18:25 ` Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 09/14] livepatch: remove unnecessary object loaded check Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 10/14] livepatch: move patching functions into patch.c Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 11/14] livepatch: store function sizes Josh Poimboeuf
2016-03-25 19:34 ` [RFC PATCH v1.9 12/14] livepatch: create per-task consistency model Josh Poimboeuf
2016-03-31 13:12 ` Miroslav Benes
2016-04-04 18:21 ` Josh Poimboeuf
2016-04-04 18:27 ` Vojtech Pavlik
2016-04-04 18:33 ` Josh Poimboeuf
2016-04-05 11:36 ` Vojtech Pavlik
2016-04-05 13:53 ` Josh Poimboeuf
2016-04-05 17:32 ` Minfei Huang
2016-04-05 21:17 ` Josh Poimboeuf
2016-04-14 9:25 ` Miroslav Benes [this message]
2016-04-14 16:39 ` Josh Poimboeuf
2016-04-15 9:17 ` Miroslav Benes
2016-03-25 19:35 ` [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status Josh Poimboeuf
2016-03-31 9:33 ` Jiri Slaby
2016-03-31 9:40 ` Jiri Slaby
2016-04-04 16:56 ` Josh Poimboeuf
2016-03-25 19:35 ` [RFC PATCH v1.9 14/14] livepatch: update task universe when exiting kernel Josh Poimboeuf
2016-04-14 8:47 ` Miroslav Benes
2016-04-14 8:50 ` Miroslav Benes
2016-04-14 13:39 ` Josh Poimboeuf
2016-04-18 15:01 ` [RFC PATCH 0/2] s390/klp: s390 support Miroslav Benes
2016-04-18 15:01 ` [RFC PATCH 1/2] s390: livepatch, reorganize TIF bits Miroslav Benes
2016-04-18 15:01 ` [RFC PATCH 2/2] s390/klp: update task universe when exiting kernel Miroslav Benes
2016-04-18 15:17 ` [RFC PATCH 0/2] s390/klp: s390 support Josh Poimboeuf
2016-04-14 13:23 ` [RFC PATCH v1.9 14/14] livepatch: update task universe when exiting kernel Josh Poimboeuf
2016-03-31 12:54 ` [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Miroslav Benes
2016-04-04 17:03 ` Josh Poimboeuf
2016-04-05 14:24 ` Miroslav Benes
2016-04-05 14:34 ` Josh Poimboeuf
2016-04-05 14:53 ` Miroslav Benes
2016-04-01 13:34 ` Miroslav Benes
2016-04-01 13:39 ` Petr Mladek
2016-04-01 15:38 ` Petr Mladek
2016-04-05 13:44 ` Josh Poimboeuf
2016-04-06 8:15 ` Petr Mladek
2016-04-28 18:53 ` Josh Poimboeuf
2016-06-09 14:20 ` Petr Mladek
2016-04-07 12:10 ` Petr Mladek
2016-04-07 15:08 ` Josh Poimboeuf
2016-04-07 15:47 ` Jiri Kosina
2016-04-07 18:03 ` Josh Poimboeuf
2016-04-07 18:33 ` Jiri Kosina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LNX.2.00.1604141102400.10605@pobox.suse.cz \
--to=mbenes@suse.cz \
--cc=jeyu@redhat.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=vojtech@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).