On Thu 2017-05-18 14:00:43, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptibly, it could
> block the whole transition process indefinitely. Thus it may be useful
> to clear its TIF_PATCH_PENDING to allow the process to finish.
>
> Admin can do that now by writing 2 to force sysfs attribute in livepatch
> sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> transition can finish successfully.
>
> Important note! Use wisely. Admin must be sure that it is safe to
> execute such action. This means that it must be checked that by doing so
> the consistency model guarantees are not violated.
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index bb61aaa196d3..d057a34510e6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
> }
> read_unlock(&tasklist_lock);
> }
> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + */
> +void klp_unmark_tasks(void)
> +{
> + struct task_struct *g, *task;
> +
> + pr_warn("all tasks marked as migrated on admin's request\n");
> +
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task)
> + klp_update_patch_state(task);
> + read_unlock(&tasklist_lock);
This should get called under klp_mutex. The following race comes to my mind:
CPU0: CPU1:
klp_transition_work_fn()
klp_try_complete_transition()
for_each_process()
if (!klp_try_switch_task(task))
# success
klp_complete_transition()
for_each_process()
task->patch_state = KLP_UNDEFINED;
klp_unmark_tasks()
for_each_process()
klp_update_patch_state()
task->patch_state =
klp_target_state;
klp_target_state = KLP_UNDEFINED;
=> CPU1 might happily set an obsolete state and create a mess.
It would be possible to solve this by reodering, barriers.
But much better solution seems to serialize both actions
using klp_mutex.
In fact, I would suggest to take klp_mutex in force_store()
and do all actions synchronously, including the check
of klp_transition_patch.
Best Regards,
Petr
PS: I know that I talked about this with Mirek and suggested
doing the check for klp_transition_patch without the lock.
It made perfect sense. But I have changed my mind when
seeing the final code.
