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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
