On 11/22/2017 05:29 AM, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptedly, it could
> block the whole transition 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 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! Administrator should not use this feature without a
> clearance from a patch distributor. It must be checked that by doing so
> the consistency model guarantees are not violated. Removal (rmmod) of
> patch modules is permanently disabled when the feature is used. It
> cannot be guaranteed there is no task sleeping in such module.
>
> Signed-off-by: Miroslav Benes <[email protected]>
> Acked-by: Josh Poimboeuf <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> Documentation/livepatch/livepatch.txt | 18 ++++++++++--
> kernel/livepatch/core.c | 30 ++++++++++++++++++++
> kernel/livepatch/transition.c | 35 +++++++++++++++++++++++-
> kernel/livepatch/transition.h | 1 +
> 5 files changed, 95 insertions(+), 3 deletions(-)
....
> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + *
> + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> + * 'current'. This is not the case here and the consistency model could be
> + * broken. Administrator, who is the only one to execute the
> + * klp_force_transitions(), has to be aware of this.
> + */
> +void klp_force_transition(void)
> +{
> + struct task_struct *g, *task;
> + unsigned int cpu;
> +
> + pr_warn("forcing remaining tasks to the patched state\n");
> +
> + read_lock(&tasklist_lock);
> + for_each_process_thread(g, task)
> + klp_update_patch_state(task);
> + read_unlock(&tasklist_lock);
> +
> + for_each_possible_cpu(cpu)
> + klp_update_patch_state(idle_task(cpu));
> +
> + klp_forced = true;
> +}
I had a question on this bit. If say cpu 0 executes
klp_force_transition(void), right up until klp_forced is set to true,
and then cpu 1 does klp_complete_transition() (since all threads have
the correct state), wouldn't it be possible then for
klp_complete_transition() to not see klp_forced set to true, and thus
the module could be potentially removed even though it was forced?
If so, I think that the force path just needs to be set before the
threads are updated (as below). I don't think that the
klp_complete_transition() needs the corresponding rmb, b/c there is
sufficient ordering there already (although it would deserve a comment).
Thanks,
-Jason
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index be5bfa5..cca6a3a 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -671,6 +671,15 @@ void klp_force_transition(void)
pr_warn("forcing remaining tasks to the patched state\n");
+ klp_forced = true;
+
+ /*
+ * ensure that if klp_complete_transition() sees that all
+ * the threads have been updated to desired task->patch_state
+ * that we also see klp_forced = true;
+ */
+ smp_wmb();
+
read_lock(&tasklist_lock);
for_each_process_thread(g, task)
klp_update_patch_state(task);
@@ -678,6 +687,4 @@ void klp_force_transition(void)
for_each_possible_cpu(cpu)
klp_update_patch_state(idle_task(cpu));
-
- klp_forced = true;
}
On Fri, 15 Dec 2017, Jason Baron wrote:
> On 11/22/2017 05:29 AM, Miroslav Benes wrote:
> > If a task sleeps in a set of patched functions uninterruptedly, it could
> > block the whole transition 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 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! Administrator should not use this feature without a
> > clearance from a patch distributor. It must be checked that by doing so
> > the consistency model guarantees are not violated. Removal (rmmod) of
> > patch modules is permanently disabled when the feature is used. It
> > cannot be guaranteed there is no task sleeping in such module.
> >
> > Signed-off-by: Miroslav Benes <[email protected]>
> > Acked-by: Josh Poimboeuf <[email protected]>
> > Reviewed-by: Petr Mladek <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> > Documentation/livepatch/livepatch.txt | 18 ++++++++++--
> > kernel/livepatch/core.c | 30 ++++++++++++++++++++
> > kernel/livepatch/transition.c | 35 +++++++++++++++++++++++-
> > kernel/livepatch/transition.h | 1 +
> > 5 files changed, 95 insertions(+), 3 deletions(-)
>
> ....
>
> > +
> > +/*
> > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > + * existing transition to finish.
> > + *
> > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > + * 'current'. This is not the case here and the consistency model could be
> > + * broken. Administrator, who is the only one to execute the
> > + * klp_force_transitions(), has to be aware of this.
> > + */
> > +void klp_force_transition(void)
> > +{
> > + struct task_struct *g, *task;
> > + unsigned int cpu;
> > +
> > + pr_warn("forcing remaining tasks to the patched state\n");
> > +
> > + read_lock(&tasklist_lock);
> > + for_each_process_thread(g, task)
> > + klp_update_patch_state(task);
> > + read_unlock(&tasklist_lock);
> > +
> > + for_each_possible_cpu(cpu)
> > + klp_update_patch_state(idle_task(cpu));
> > +
> > + klp_forced = true;
> > +}
>
> I had a question on this bit. If say cpu 0 executes
> klp_force_transition(void), right up until klp_forced is set to true,
> and then cpu 1 does klp_complete_transition() (since all threads have
> the correct state), wouldn't it be possible then for
> klp_complete_transition() to not see klp_forced set to true, and thus
> the module could be potentially removed even though it was forced?
Yes, you're right. That could happen.
> If so, I think that the force path just needs to be set before the
> threads are updated (as below). I don't think that the
> klp_complete_transition() needs the corresponding rmb, b/c there is
> sufficient ordering there already (although it would deserve a comment).
Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be
done with it once and for all. The problem is exactly what Petr predicted
and I refused to have klp_mutex here just because it may have fixed
theoretical issue.
Petr, Josh, what do you think?
Miroslav
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index be5bfa5..cca6a3a 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -671,6 +671,15 @@ void klp_force_transition(void)
>
> pr_warn("forcing remaining tasks to the patched state\n");
>
> + klp_forced = true;
> +
> + /*
> + * ensure that if klp_complete_transition() sees that all
> + * the threads have been updated to desired task->patch_state
> + * that we also see klp_forced = true;
> + */
> + smp_wmb();
> +
> read_lock(&tasklist_lock);
> for_each_process_thread(g, task)
> klp_update_patch_state(task);
> @@ -678,6 +687,4 @@ void klp_force_transition(void)
>
> for_each_possible_cpu(cpu)
> klp_update_patch_state(idle_task(cpu));
> -
> - klp_forced = true;
> }
>
On Mon, Dec 18, 2017 at 02:23:40PM +0100, Miroslav Benes wrote:
> On Fri, 15 Dec 2017, Jason Baron wrote:
>
> > On 11/22/2017 05:29 AM, Miroslav Benes wrote:
> > > If a task sleeps in a set of patched functions uninterruptedly, it could
> > > block the whole transition 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 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! Administrator should not use this feature without a
> > > clearance from a patch distributor. It must be checked that by doing so
> > > the consistency model guarantees are not violated. Removal (rmmod) of
> > > patch modules is permanently disabled when the feature is used. It
> > > cannot be guaranteed there is no task sleeping in such module.
> > >
> > > Signed-off-by: Miroslav Benes <[email protected]>
> > > Acked-by: Josh Poimboeuf <[email protected]>
> > > Reviewed-by: Petr Mladek <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> > > Documentation/livepatch/livepatch.txt | 18 ++++++++++--
> > > kernel/livepatch/core.c | 30 ++++++++++++++++++++
> > > kernel/livepatch/transition.c | 35 +++++++++++++++++++++++-
> > > kernel/livepatch/transition.h | 1 +
> > > 5 files changed, 95 insertions(+), 3 deletions(-)
> >
> > ....
> >
> > > +
> > > +/*
> > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > > + * existing transition to finish.
> > > + *
> > > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > > + * 'current'. This is not the case here and the consistency model could be
> > > + * broken. Administrator, who is the only one to execute the
> > > + * klp_force_transitions(), has to be aware of this.
> > > + */
> > > +void klp_force_transition(void)
> > > +{
> > > + struct task_struct *g, *task;
> > > + unsigned int cpu;
> > > +
> > > + pr_warn("forcing remaining tasks to the patched state\n");
> > > +
> > > + read_lock(&tasklist_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(&tasklist_lock);
> > > +
> > > + for_each_possible_cpu(cpu)
> > > + klp_update_patch_state(idle_task(cpu));
> > > +
> > > + klp_forced = true;
> > > +}
> >
> > I had a question on this bit. If say cpu 0 executes
> > klp_force_transition(void), right up until klp_forced is set to true,
> > and then cpu 1 does klp_complete_transition() (since all threads have
> > the correct state), wouldn't it be possible then for
> > klp_complete_transition() to not see klp_forced set to true, and thus
> > the module could be potentially removed even though it was forced?
>
> Yes, you're right. That could happen.
>
> > If so, I think that the force path just needs to be set before the
> > threads are updated (as below). I don't think that the
> > klp_complete_transition() needs the corresponding rmb, b/c there is
> > sufficient ordering there already (although it would deserve a comment).
>
> Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be
> done with it once and for all. The problem is exactly what Petr predicted
> and I refused to have klp_mutex here just because it may have fixed
> theoretical issue.
>
> Petr, Josh, what do you think?
Sounds good to me.
We should have known to listen to Petr in the first place :-)
--
Josh
On Mon 2017-12-18 14:23:40, Miroslav Benes wrote:
> On Fri, 15 Dec 2017, Jason Baron wrote:
>
> > On 11/22/2017 05:29 AM, Miroslav Benes wrote:
> > > If a task sleeps in a set of patched functions uninterruptedly, it could
> > > block the whole transition 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 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! Administrator should not use this feature without a
> > > clearance from a patch distributor. It must be checked that by doing so
> > > the consistency model guarantees are not violated. Removal (rmmod) of
> > > patch modules is permanently disabled when the feature is used. It
> > > cannot be guaranteed there is no task sleeping in such module.
> > >
> > > Signed-off-by: Miroslav Benes <[email protected]>
> > > Acked-by: Josh Poimboeuf <[email protected]>
> > > Reviewed-by: Petr Mladek <[email protected]>
> > > ---
> > > Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> > > Documentation/livepatch/livepatch.txt | 18 ++++++++++--
> > > kernel/livepatch/core.c | 30 ++++++++++++++++++++
> > > kernel/livepatch/transition.c | 35 +++++++++++++++++++++++-
> > > kernel/livepatch/transition.h | 1 +
> > > 5 files changed, 95 insertions(+), 3 deletions(-)
> >
> > ....
> >
> > > +
> > > +/*
> > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > > + * existing transition to finish.
> > > + *
> > > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > > + * 'current'. This is not the case here and the consistency model could be
> > > + * broken. Administrator, who is the only one to execute the
> > > + * klp_force_transitions(), has to be aware of this.
> > > + */
> > > +void klp_force_transition(void)
> > > +{
> > > + struct task_struct *g, *task;
> > > + unsigned int cpu;
> > > +
> > > + pr_warn("forcing remaining tasks to the patched state\n");
> > > +
> > > + read_lock(&tasklist_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(&tasklist_lock);
> > > +
> > > + for_each_possible_cpu(cpu)
> > > + klp_update_patch_state(idle_task(cpu));
> > > +
> > > + klp_forced = true;
> > > +}
> >
> > I had a question on this bit. If say cpu 0 executes
> > klp_force_transition(void), right up until klp_forced is set to true,
> > and then cpu 1 does klp_complete_transition() (since all threads have
> > the correct state), wouldn't it be possible then for
> > klp_complete_transition() to not see klp_forced set to true, and thus
> > the module could be potentially removed even though it was forced?
>
> Yes, you're right. That could happen.
>
> > If so, I think that the force path just needs to be set before the
> > threads are updated (as below). I don't think that the
> > klp_complete_transition() needs the corresponding rmb, b/c there is
> > sufficient ordering there already (although it would deserve a comment).
>
> Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be
> done with it once and for all. The problem is exactly what Petr predicted
> and I refused to have klp_mutex here just because it may have fixed
> theoretical issue.
>
> Petr, Josh, what do you think?
Sounds good to me. I would feel better if the lock is there.
Best Regards,
Petr