Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1390507 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2016-04-28 22:50 +0200 |
| Last post | 2016-05-10 13:50 +0200 |
| Articles | 12 on this page of 72 — 11 participants |
Back to article view | Back to linux.kernel
[RFC PATCH v2 00/18] livepatch: hybrid consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
[RFC PATCH v2 14/18] livepatch: remove unnecessary object loaded check Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
[RFC PATCH v2 10/18] livepatch/powerpc: add TIF_PATCH_PENDING thread flag Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
Re: [RFC PATCH v2 10/18] livepatch/powerpc: add TIF_PATCH_PENDING thread flag Petr Mladek <pmladek@suse.com> - 2016-05-03 11:10 +0200
Re: [RFC PATCH v2 10/18] livepatch/powerpc: add TIF_PATCH_PENDING thread flag Miroslav Benes <mbenes@suse.cz> - 2016-05-03 14:10 +0200
[RFC PATCH v2 16/18] livepatch: store function sizes Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
[RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-29 20:10 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-29 22:20 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 22:30 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-29 22:40 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 23:30 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-29 23:40 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Jiri Kosina <jikos@kernel.org> - 2016-04-30 00:20 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-30 01:00 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-30 02:20 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-30 00:50 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-04-30 02:10 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-02 16:00 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-05-02 18:00 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-02 19:40 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-05-02 20:20 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Ingo Molnar <mingo@kernel.org> - 2016-05-02 20:40 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-02 21:50 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Jiri Kosina <jikos@kernel.org> - 2016-05-02 22:00 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Jiri Kosina <jikos@kernel.org> - 2016-05-02 22:10 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Andy Lutomirski <luto@amacapital.net> - 2016-05-03 02:50 +0200
RE: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking David Laight <David.Laight@ACULAB.COM> - 2016-05-04 17:20 +0200
Re: [RFC PATCH v2 05/18] sched: add task flag for preempt IRQ tracking Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 22:20 +0200
[RFC PATCH v2 09/18] livepatch/x86: add TIF_PATCH_PENDING thread flag Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
Re: [RFC PATCH v2 09/18] livepatch/x86: add TIF_PATCH_PENDING thread flag Andy Lutomirski <luto@amacapital.net> - 2016-04-29 20:10 +0200
Re: [RFC PATCH v2 09/18] livepatch/x86: add TIF_PATCH_PENDING thread flag Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 22:20 +0200
[RFC PATCH v2 02/18] x86/asm/head: use a common function for starting CPUs Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
[RFC PATCH v2 13/18] livepatch: separate enabled and patched states Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
Re: [RFC PATCH v2 13/18] livepatch: separate enabled and patched states Petr Mladek <pmladek@suse.com> - 2016-05-03 11:40 +0200
Re: [RFC PATCH v2 13/18] livepatch: separate enabled and patched states Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-03 15:50 +0200
[RFC PATCH v2 06/18] x86: dump_trace() error handling Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 22:50 +0200
Re: [RFC PATCH v2 06/18] x86: dump_trace() error handling Minfei Huang <mnghuan@gmail.com> - 2016-04-29 15:50 +0200
Re: [RFC PATCH v2 06/18] x86: dump_trace() error handling Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 16:10 +0200
[RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 23:00 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Brian Gerst <brgerst@gmail.com> - 2016-04-29 20:50 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 22:30 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Andy Lutomirski <luto@kernel.org> - 2016-04-29 21:40 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-29 23:00 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Andy Lutomirski <luto@amacapital.net> - 2016-04-29 23:40 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-30 01:30 +0200
Re: [RFC PATCH v2 03/18] x86/asm/head: standardize the bottom of the stack for idle tasks Andy Lutomirski <luto@amacapital.net> - 2016-04-30 02:20 +0200
[RFC PATCH v2 04/18] x86: move _stext marker before head code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 23:00 +0200
[RFC PATCH v2 01/18] x86/asm/head: clean up initial stack variable Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 23:00 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-04 10:50 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-04 18:00 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Miroslav Benes <mbenes@suse.cz> - 2016-05-05 11:50 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-05 15:10 +0200
barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-04 14:40 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Peter Zijlstra <peterz@infradead.org> - 2016-05-04 16:00 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-04 19:00 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-04 16:20 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-04 19:30 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-05 13:30 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Miroslav Benes <mbenes@suse.cz> - 2016-05-09 17:50 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-04 19:10 +0200
Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-05 12:30 +0200
klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-04 16:50 +0200
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Jiri Kosina <jikos@kernel.org> - 2016-05-04 17:00 +0200
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-04 20:00 +0200
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-05 14:00 +0200
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-06 14:40 +0200
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-09 14:30 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Petr Mladek <pmladek@suse.com> - 2016-05-06 13:40 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-06 14:50 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Miroslav Benes <mbenes@suse.cz> - 2016-05-09 11:50 +0200
Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model Miroslav Benes <mbenes@suse.cz> - 2016-05-10 13:50 +0200
Page 4 of 4 — ← Prev page 1 2 3 [4]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-05-04 19:10 +0200 |
| Subject | Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rv8tc-86C-25@gated-at.bofh.it> |
| In reply to | #1394246 |
On Wed, May 04, 2016 at 02:39:40PM +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. > > I spent a lot of time with checking the memory barriers. It seems that > they are basically correct. Let me use my own words to show how > I understand it. I hope that it will help others with review. [...snip a ton of useful comments...] Thanks, this will help a lot! I'll try to incorporate your barrier comments into the code. I also agree that kpatch_patch_task() is poorly named. I was trying to make it clear to external callers that "hey, the task is getting patched now!", but it's internally inconsistent with livepatch code because we make a distinction between patching and unpatching. Maybe I'll do: klp_update_task_patch_state() -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-05-05 12:30 +0200 |
| Subject | Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rvoHE-6D6-11@gated-at.bofh.it> |
| In reply to | #1394554 |
On Wed 2016-05-04 12:02:36, Josh Poimboeuf wrote: > On Wed, May 04, 2016 at 02:39:40PM +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. > > > > I spent a lot of time with checking the memory barriers. It seems that > > they are basically correct. Let me use my own words to show how > > I understand it. I hope that it will help others with review. > > [...snip a ton of useful comments...] > > Thanks, this will help a lot! I'll try to incorporate your barrier > comments into the code. Thanks a lot. > I also agree that kpatch_patch_task() is poorly named. I was trying to > make it clear to external callers that "hey, the task is getting patched > now!", but it's internally inconsistent with livepatch code because we > make a distinction between patching and unpatching. > > Maybe I'll do: > > klp_update_task_patch_state() I like it. It is long but it well describes the purpose. Livepatch is using many state variables: + global: klp_transition_patch, klp_target_state + per task specific: TIF_PENDING_PATCH, patch_state + per each new function: transition, patched + per old function: func_stack + per object: patched, loaded + per patch: enabled The dependency between them and the workflow is important to create a mental picture about the Livepatching. Good names help with it. Best Regards, Petr
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-05-04 16:50 +0200 |
| Subject | klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rv6hJ-5S0-59@gated-at.bofh.it> |
| In reply to | #1390507 |
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 <linux/mm.h>
> #include <linux/stackprotector.h>
> #include <linux/suspend.h>
> +#include <linux/livepatch.h>
>
> #include <asm/tlb.h>
>
> @@ -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.
Best Regards,
Petr
[toc] | [prev] | [next] | [standalone]
| From | Jiri Kosina <jikos@kernel.org> |
|---|---|
| Date | 2016-05-04 17:00 +0200 |
| Subject | Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rv6rn-5XA-3@gated-at.bofh.it> |
| In reply to | #1394405 |
On Wed, 4 May 2016, Petr Mladek wrote: > > + > > + 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 The code in question is running with preemption disabled. > or inside the klp_patch_task() function. We must make sure that this function doesn't go to sleep. It's only used to clear the task_struct flag anyway. -- Jiri Kosina SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-05-04 20:00 +0200 |
| Subject | Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rv9fz-85-3@gated-at.bofh.it> |
| In reply to | #1394405 |
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 <linux/mm.h>
> > #include <linux/stackprotector.h>
> > #include <linux/suspend.h>
> > +#include <linux/livepatch.h>
> >
> > #include <asm/tlb.h>
> >
> > @@ -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
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-05-05 14:00 +0200 |
| Subject | Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rvq6J-7IA-5@gated-at.bofh.it> |
| In reply to | #1394581 |
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 <linux/mm.h>
> > > #include <linux/stackprotector.h>
> > > #include <linux/suspend.h>
> > > +#include <linux/livepatch.h>
> > >
> > > #include <asm/tlb.h>
> > >
> > > @@ -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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-05-06 14:40 +0200 |
| Subject | Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rvNcZ-57y-3@gated-at.bofh.it> |
| In reply to | #1395058 |
On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> 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
>
Thanks for the clear explanation, this helps a lot.
> 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.
I'm not sure about this solution. When klp_complete_transition() is
called, we need all tasks to be patched, for good. We don't want any of
them to randomly switch to the wrong state at some later time in the
middle of a future patch operation. How would changing klp_patch_task()
to only use "current" prevent that?
> 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.
I think we could fix it to be safe for future callers who might not have
preemption disabled with a couple of changes to klp_patch_task():
disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
before changing the patch state:
void klp_patch_task(struct task_struct *task)
{
preempt_disable();
if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
task->patch_state = READ_ONCE(klp_target_state);
preempt_enable();
}
We would also need a synchronize_sched() after the patching is complete,
either at the end of klp_try_complete_transition() or in
klp_complete_transition(). That would make sure that all existing calls
to klp_patch_task() are done.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-05-09 14:30 +0200 |
| Subject | Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rwStY-5zM-15@gated-at.bofh.it> |
| In reply to | #1395831 |
On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > 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
> >
>
> Thanks for the clear explanation, this helps a lot.
>
> > 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.
>
> I'm not sure about this solution. When klp_complete_transition() is
> called, we need all tasks to be patched, for good. We don't want any of
> them to randomly switch to the wrong state at some later time in the
> middle of a future patch operation. How would changing klp_patch_task()
> to only use "current" prevent that?
You are right that it is pity but it really should be safe because
it is not entirely random.
If the race happens and assign an outdated value, there are two
situations:
1. It is assigned when there is not transition in the progress.
Then it is OK because it will be ignored by the ftrace handler.
The right state will be set before the next transition starts.
2. It is assigned when some other transition is in progress.
Then it is OK as long as the function is called from "current".
The "wrong" state will be used consistently. It will switch
to the right state on another safe state.
> > 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.
>
> I think we could fix it to be safe for future callers who might not have
> preemption disabled with a couple of changes to klp_patch_task():
> disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> before changing the patch state:
>
> void klp_patch_task(struct task_struct *task)
> {
> preempt_disable();
>
> if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> task->patch_state = READ_ONCE(klp_target_state);
>
> preempt_enable();
> }
It reduces the race window a bit but it is still there. For example,
NMI still might add a huge delay between reading klp_target_state
and assigning task->patch state.
What about the following?
/*
* This function might assign an outdated value if the transaction
`* is reverted and finalized in parallel. But it is safe. If the value
* is assigned outside of a transaction, it is ignored and the next
* transaction will set the right one. Or if it gets assigned
* inside another transaction, it will repeat the cycle and
* set the right state.
*/
void klp_update_current_patch_state()
{
while (test_and_clear_tsk_thread_flag(current, TIF_PATCH_PENDING))
current->patch_state = READ_ONCE(klp_target_state);
}
Note that the disabled preemption helped only partially,
so I think that it was not really needed.
Hmm, it means that the task->patch_state might be either
KLP_PATCHED or KLP_UNPATCHED outside a transition. I wonder
if the tristate really brings some advantages.
Alternatively, we might synchronize the operation with klp_mutex.
The function is called in a slow path and in a safe context.
Well, it might cause contention on the lock when many CPUs are
trying to update their tasks.
Best Regards,
Petr
[toc] | [prev] | [next] | [standalone]
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Date | 2016-05-06 13:40 +0200 |
| Subject | Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rvMgX-466-25@gated-at.bofh.it> |
| In reply to | #1390507 |
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 <linux/bug.h>
> #include <linux/printk.h>
> #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.
> + 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.
Best Regards,
Petr
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-05-06 14:50 +0200 |
| Subject | Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rvNmG-5ee-19@gated-at.bofh.it> |
| In reply to | #1395796 |
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 <linux/bug.h>
> > #include <linux/printk.h>
> > #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
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-05-09 11:50 +0200 |
| Subject | Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rwPZ8-2Mv-3@gated-at.bofh.it> |
| In reply to | #1390507 |
[...]
> +static int klp_target_state;
[...]
> +void klp_init_transition(struct klp_patch *patch, int state)
> +{
> + struct task_struct *g, *task;
> + unsigned int cpu;
> + struct klp_object *obj;
> + struct klp_func *func;
> + int initial_state = !state;
> +
> + klp_transition_patch = patch;
> +
> + /*
> + * If the patch can be applied or reverted immediately, skip the
> + * per-task transitions.
> + */
> + if (patch->immediate)
> + return;
> +
> + /*
> + * Initialize all tasks to the initial patch state to prepare them for
> + * switching to the target state.
> + */
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task)
> + task->patch_state = initial_state;
> + read_unlock(&tasklist_lock);
> +
> + /*
> + * Ditto for the idle "swapper" tasks.
> + */
> + get_online_cpus();
> + for_each_online_cpu(cpu)
> + idle_task(cpu)->patch_state = initial_state;
> + put_online_cpus();
> +
> + /*
> + * Ensure klp_ftrace_handler() sees the task->patch_state updates
> + * before the func->transition updates. Otherwise it could read an
> + * out-of-date task state and pick the wrong function.
> + */
> + smp_wmb();
> +
> + /*
> + * Set the func transition states so klp_ftrace_handler() will know to
> + * switch to the transition logic.
> + *
> + * When patching, the funcs aren't yet in the func_stack and will be
> + * made visible to the ftrace handler shortly by the calls to
> + * klp_patch_object().
> + *
> + * When unpatching, the funcs are already in the func_stack and so are
> + * already visible to the ftrace handler.
> + */
> + klp_for_each_object(patch, obj)
> + klp_for_each_func(obj, func)
> + func->transition = true;
> +
> + /*
> + * Set the global target patch state which tasks will switch to. This
> + * has no effect until the TIF_PATCH_PENDING flags get set later.
> + */
> + klp_target_state = state;
I am afraid there is a problem for (patch->immediate == true) patches.
klp_target_state is not set for those and the comment is not entirely
true, because klp_target_state has an effect in several places.
[...]
> +void klp_start_transition(void)
> +{
> + struct task_struct *g, *task;
> + unsigned int cpu;
> +
> + pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
> + klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
Here...
> +
> + /*
> + * If the patch can be applied or reverted immediately, skip the
> + * per-task transitions.
> + */
> + if (klp_transition_patch->immediate)
> + return;
> +
[...]
> +bool klp_try_complete_transition(void)
> +{
> + unsigned int cpu;
> + struct task_struct *g, *task;
> + bool complete = true;
> +
> + /*
> + * If the patch can be applied or reverted immediately, skip the
> + * per-task transitions.
> + */
> + if (klp_transition_patch->immediate)
> + goto success;
> +
> + /*
> + * Try to switch the tasks to the target patch state by walking their
> + * stacks and looking for any to-be-patched or to-be-unpatched
> + * functions. If such functions are found on a stack, or if the stack
> + * is deemed unreliable, the task can't be switched yet.
> + *
> + * Usually this will transition most (or all) of the tasks on a system
> + * unless the patch includes changes to a very common function.
> + */
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task)
> + if (!klp_try_switch_task(task))
> + complete = false;
> + read_unlock(&tasklist_lock);
> +
> + /*
> + * Ditto for the idle "swapper" tasks.
> + */
> + get_online_cpus();
> + for_each_online_cpu(cpu)
> + if (!klp_try_switch_task(idle_task(cpu)))
> + complete = false;
> + put_online_cpus();
> +
> + /*
> + * Some tasks weren't able to be switched over. Try again later and/or
> + * wait for other methods like syscall barrier switching.
> + */
> + if (!complete)
> + return false;
> +
> +success:
> + /*
> + * When unpatching, all tasks have transitioned to KLP_UNPATCHED so we
> + * can now remove the new functions from the func_stack.
> + */
> + if (klp_target_state == KLP_UNPATCHED) {
Here (this is the most important one I think).
> + klp_unpatch_objects(klp_transition_patch);
> +
> + /*
> + * Don't allow any existing instances of ftrace handlers to
> + * access any obsolete funcs before we reset the func
> + * transition states to false. Otherwise the handler may see
> + * the deleted "new" func, see that it's not in transition, and
> + * wrongly pick the new version of the function.
> + */
> + synchronize_rcu();
> + }
> +
> + pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> + klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
Here
> +
> + /* we're done, now cleanup the data structures */
> + klp_complete_transition();
> +
> + return true;
> +}
> +
> +/*
> + * This function can be called in the middle of an existing transition to
> + * reverse the direction of the target patch state. This can be done to
> + * effectively cancel an existing enable or disable operation if there are any
> + * tasks which are stuck in the initial patch state.
> + */
> +void klp_reverse_transition(void)
> +{
> + struct klp_patch *patch = klp_transition_patch;
> +
> + klp_target_state = !klp_target_state;
And probably here.
All other references look safe.
I guess we need to set klp_target_state even for immediate patches. Should
we also initialize it with KLP_UNDEFINED and set it to KLP_UNDEFINED in
klp_complete_transition()?
Miroslav
[toc] | [prev] | [next] | [standalone]
| From | Miroslav Benes <mbenes@suse.cz> |
|---|---|
| Date | 2016-05-10 13:50 +0200 |
| Subject | Re: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model |
| Message-ID | <rxekP-1Bh-29@gated-at.bofh.it> |
| In reply to | #1390507 |
On Thu, 28 Apr 2016, 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. > > This code stems from the design proposal made by Vojtech [1] in November > 2014. It's a hybrid of kGraft and kpatch: it uses kGraft's per-task > consistency and syscall barrier switching combined with kpatch's stack > trace switching. There are also a number of fallback options which make > it quite flexible. > > Patches are applied on a per-task basis, when the task is deemed safe to > switch over. When a patch is enabled, livepatch enters into a > transition state where tasks are converging to the patched state. > Usually this transition state can complete in a few seconds. The same > sequence occurs when a patch is disabled, except the tasks converge from > the patched state to the unpatched state. > > An interrupt handler inherits the patched state of the task it > interrupts. The same is true for forked tasks: the child inherits the > patched state of the parent. > > Livepatch uses several complementary approaches to determine when it's > safe to patch tasks: > > 1. The first and most effective approach is stack checking of sleeping > tasks. If no affected functions are on the stack of a given task, > the task is patched. In most cases this will patch most or all of > the tasks on the first try. Otherwise it'll keep trying > periodically. This option is only available if the architecture has > reliable stacks (CONFIG_RELIABLE_STACKTRACE and > CONFIG_STACK_VALIDATION). > > 2. The second approach, if needed, is kernel exit switching. A > task is switched when it returns to user space from a system call, a > user space IRQ, or a signal. It's useful in the following cases: > > a) Patching I/O-bound user tasks which are sleeping on an affected > function. In this case you have to send SIGSTOP and SIGCONT to > force it to exit the kernel and be patched. > b) Patching CPU-bound user tasks. If the task is highly CPU-bound > then it will get patched the next time it gets interrupted by an > IRQ. > c) Applying patches for architectures which don't yet have > CONFIG_RELIABLE_STACKTRACE. In this case you'll have to signal > most of the tasks on the system. However this isn't a complete > solution, because there's currently no way to patch kthreads > without CONFIG_RELIABLE_STACKTRACE. > > Note: since idle "swapper" tasks don't ever exit the kernel, they > instead have a kpatch_patch_task() call in the idle loop which allows s/kpatch_patch_task()/klp_patch_task()/ [...] > --- a/Documentation/livepatch/livepatch.txt > +++ b/Documentation/livepatch/livepatch.txt > @@ -72,7 +72,8 @@ example, they add a NULL pointer or a boundary check, fix a race by adding > a missing memory barrier, or add some locking around a critical section. > Most of these changes are self contained and the function presents itself > the same way to the rest of the system. In this case, the functions might > -be updated independently one by one. > +be updated independently one by one. (This can be done by setting the > +'immediate' flag in the klp_patch struct.) > > But there are more complex fixes. For example, a patch might change > ordering of locking in multiple functions at the same time. Or a patch > @@ -86,20 +87,103 @@ or no data are stored in the modified structures at the moment. > The theory about how to apply functions a safe way is rather complex. > The aim is to define a so-called consistency model. It attempts to define > conditions when the new implementation could be used so that the system > -stays consistent. The theory is not yet finished. See the discussion at > -http://thread.gmane.org/gmane.linux.kernel/1823033/focus=1828189 > - > -The current consistency model is very simple. It guarantees that either > -the old or the new function is called. But various functions get redirected > -one by one without any synchronization. > - > -In other words, the current implementation _never_ modifies the behavior > -in the middle of the call. It is because it does _not_ rewrite the entire > -function in the memory. Instead, the function gets redirected at the > -very beginning. But this redirection is used immediately even when > -some other functions from the same patch have not been redirected yet. > - > -See also the section "Limitations" below. > +stays consistent. > + > +Livepatch has a consistency model which is a hybrid of kGraft and > +kpatch: it uses kGraft's per-task consistency and syscall barrier > +switching combined with kpatch's stack trace switching. There are also > +a number of fallback options which make it quite flexible. > + > +Patches are applied on a per-task basis, when the task is deemed safe to > +switch over. When a patch is enabled, livepatch enters into a > +transition state where tasks are converging to the patched state. > +Usually this transition state can complete in a few seconds. The same > +sequence occurs when a patch is disabled, except the tasks converge from > +the patched state to the unpatched state. > + > +An interrupt handler inherits the patched state of the task it > +interrupts. The same is true for forked tasks: the child inherits the > +patched state of the parent. > + > +Livepatch uses several complementary approaches to determine when it's > +safe to patch tasks: > + > +1. The first and most effective approach is stack checking of sleeping > + tasks. If no affected functions are on the stack of a given task, > + the task is patched. In most cases this will patch most or all of > + the tasks on the first try. Otherwise it'll keep trying > + periodically. This option is only available if the architecture has > + reliable stacks (CONFIG_RELIABLE_STACKTRACE and > + CONFIG_STACK_VALIDATION). > + > +2. The second approach, if needed, is kernel exit switching. A > + task is switched when it returns to user space from a system call, a > + user space IRQ, or a signal. It's useful in the following cases: > + > + a) Patching I/O-bound user tasks which are sleeping on an affected > + function. In this case you have to send SIGSTOP and SIGCONT to > + force it to exit the kernel and be patched. > + b) Patching CPU-bound user tasks. If the task is highly CPU-bound > + then it will get patched the next time it gets interrupted by an > + IRQ. > + c) Applying patches for architectures which don't yet have > + CONFIG_RELIABLE_STACKTRACE. In this case you'll have to signal > + most of the tasks on the system. However this isn't a complete > + solution, because there's currently no way to patch kthreads > + without CONFIG_RELIABLE_STACKTRACE. > + > + Note: since idle "swapper" tasks don't ever exit the kernel, they > + instead have a kpatch_patch_task() call in the idle loop which allows s/kpatch_patch_task()/klp_patch_task()/ Otherwise all the code that touches livepatch looks good to me. Apart from the things mentioned in another emails. Miroslav
[toc] | [prev] | [standalone]
Page 4 of 4 — ← Prev page 1 2 3 [4]
Back to top | Article view | linux.kernel
csiph-web