On Thu, May 18, 2017 at 02:00:43PM +0200, 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.
>
> Signed-off-by: Miroslav Benes <mbenes@xxxxxxx>
These patches look good to me. Just some minor comments.
> ---
> include/linux/livepatch.h | 1 +
> kernel/livepatch/core.c | 3 +++
> kernel/livepatch/transition.c | 16 ++++++++++++++++
> kernel/livepatch/transition.h | 1 +
> 4 files changed, 21 insertions(+)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 43cfeebeb42b..b567208a1c6e 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -31,6 +31,7 @@
>
> /* values for sysfs force attribute */
> #define KLP_FORCE_FAKE 1
> +#define KLP_FORCE_UNMARK 2
>
> /* task patch states */
> #define KLP_UNDEFINED -1
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bb3b78fa7d2b..9bc1103348c9 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> case KLP_FORCE_FAKE:
> klp_send_fake_signal();
> break;
> + case KLP_FORCE_UNMARK:
> + klp_unmark_tasks();
> + break;
I think the naming could be a little clearer, and more consistent. What
do you think about:
KLP_FORCE_SIGNALS -> klp_force_signals()
KLP_FORCE_TRANSITIONS -> klp_force_transitions()
> default:
> return -EINVAL;
> }
> 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");
The user might not know what migrated means. How about "forcing
remaining tasks to the patched state" or something similar?
> +
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task)
> + klp_update_patch_state(task);
> + read_unlock(&tasklist_lock);
So klp_update_patch_state() has the following comment:
* NOTE: If task is not 'current', the caller must ensure the task is inactive.
* Otherwise klp_ftrace_handler() might read the wrong 'patch_state' value.
This code doesn't ensure the task is inactive. But I think that's ok as
long as we document the fact that this could break the consistency
model, right?
On a related note, I think the new sysfs entry should also be documented
in Documentation/livepatch/livepatch.txt somewhere.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
