Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Petr Mladek]

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 
> > > Acked-by: Josh Poimboeuf 
> > > Reviewed-by: Petr Mladek 
> > > ---
> > >  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(_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(_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

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Petr Mladek]

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 
> > > Acked-by: Josh Poimboeuf 
> > > Reviewed-by: Petr Mladek 
> > > ---
> > >  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(_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(_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

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Josh Poimboeuf]

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 
> > > Acked-by: Josh Poimboeuf 
> > > Reviewed-by: Petr Mladek 
> > > ---
> > >  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(_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(_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

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Josh Poimboeuf]

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 
> > > Acked-by: Josh Poimboeuf 
> > > Reviewed-by: Petr Mladek 
> > > ---
> > >  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(_lock);
> > > + for_each_process_thread(g, task)
> > > + klp_update_patch_state(task);
> > > + read_unlock(_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

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Miroslav Benes]

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 
> > Acked-by: Josh Poimboeuf 
> > Reviewed-by: Petr Mladek 
> > ---
> >  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(_lock);
> > +   for_each_process_thread(g, task)
> > +   klp_update_patch_state(task);
> > +   read_unlock(_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(_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;
>  }
> 

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Miroslav Benes]

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 
> > Acked-by: Josh Poimboeuf 
> > Reviewed-by: Petr Mladek 
> > ---
> >  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(_lock);
> > +   for_each_process_thread(g, task)
> > +   klp_update_patch_state(task);
> > +   read_unlock(_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(_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;
>  }
> 

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Jason Baron]

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 
> Acked-by: Josh Poimboeuf 
> Reviewed-by: Petr Mladek 
> ---
>  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(_lock);
> + for_each_process_thread(g, task)
> + klp_update_patch_state(task);
> + read_unlock(_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(_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;
 }

Re: [PATCH v4.1 2/2] livepatch: force transition to finish

[Jason Baron]

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 
> Acked-by: Josh Poimboeuf 
> Reviewed-by: Petr Mladek 
> ---
>  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(_lock);
> + for_each_process_thread(g, task)
> + klp_update_patch_state(task);
> + read_unlock(_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(_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;
 }

[PATCH v4.1 2/2] livepatch: force transition to finish

[Miroslav Benes]

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 
Acked-by: Josh Poimboeuf 
Reviewed-by: Petr Mladek 
---
 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(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch 
b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 3bb9d5bc1ce3..dac7e1e62a8b 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -45,6 +45,20 @@ Contact: live-patch...@vger.kernel.org
signal pending structures). Tasks are interrupted or woken up,
and forced to change their patched state.
 
+What:  /sys/kernel/livepatch//force
+Date:  Nov 2017
+KernelVersion: 4.15.0
+Contact:   live-patch...@vger.kernel.org
+Description:
+   A writable attribute that allows administrator to affect the
+   course of an existing transition. Writing 1 clears
+   TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to
+   the patched or unpatched state. Administrator should not
+   use this feature without a clearance from a patch
+   distributor. Removal (rmmod) of patch modules is permanently
+   disabled when the feature is used. See
+   Documentation/livepatch/livepatch.txt for more information.
+
 What:  /sys/kernel/livepatch//
 Date:  Nov 2014
 KernelVersion: 3.19.0
diff --git a/Documentation/livepatch/livepatch.txt 
b/Documentation/livepatch/livepatch.txt
index 9bcdef277a36..896ba8941702 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -183,6 +183,20 @@ tasks. No proper signal is actually delivered (there is no 
data in signal
 pending structures). Tasks are interrupted or woken up, and forced to change
 their patched state.
 
+Administrator can also affect a transition through
+/sys/kernel/livepatch//force attribute. Writing 1 there clears
+TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to the patched
+state. Important note! The force attribute is intended for cases when the
+transition gets stuck for a long time because of a blocking task. Administrator
+is expected to collect all necessary data (namely stack traces of such blocking
+tasks) and request a clearance from a patch distributor to force the 
transition.
+Unauthorized usage may cause harm to the system. It depends on the nature of 
the
+patch, which functions are (un)patched, and which functions the blocking tasks
+are sleeping in (/proc//stack may help here). Removal (rmmod) of patch
+modules is permanently disabled when the force feature is used. It cannot be
+guaranteed there is no task sleeping in such module. It implies unbounded
+reference count if a patch module is disabled and enabled in a loop.
+
 3.1 Adding consistency model support to new architectures
 -
 
@@ -439,8 +453,8 @@ Information about the registered patches can be found under
 /sys/kernel/livepatch. The patches could be enabled and disabled
 by writing there.
 
-/sys/kernel/livepatch//signal attribute allows administrator to affect a
-patching operation.
+/sys/kernel/livepatch//signal and /sys/kernel/livepatch//force
+attributes allow administrator to affect a patching operation.
 
 See Documentation/ABI/testing/sysfs-kernel-livepatch for more details.
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e772be452462..c19c6c32c47e 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -441,6 +441,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch//enabled
  * /sys/kernel/livepatch//transition
  * /sys/kernel/livepatch//signal
+ * /sys/kernel/livepatch//force
  * /sys/kernel/livepatch//
  * /sys/kernel/livepatch///
  */
@@ -542,13 +543,42 @@ static ssize_t 

[PATCH v4.1 2/2] livepatch: force transition to finish

[Miroslav Benes]

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 
Acked-by: Josh Poimboeuf 
Reviewed-by: Petr Mladek 
---
 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(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch 
b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 3bb9d5bc1ce3..dac7e1e62a8b 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -45,6 +45,20 @@ Contact: live-patch...@vger.kernel.org
signal pending structures). Tasks are interrupted or woken up,
and forced to change their patched state.
 
+What:  /sys/kernel/livepatch//force
+Date:  Nov 2017
+KernelVersion: 4.15.0
+Contact:   live-patch...@vger.kernel.org
+Description:
+   A writable attribute that allows administrator to affect the
+   course of an existing transition. Writing 1 clears
+   TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to
+   the patched or unpatched state. Administrator should not
+   use this feature without a clearance from a patch
+   distributor. Removal (rmmod) of patch modules is permanently
+   disabled when the feature is used. See
+   Documentation/livepatch/livepatch.txt for more information.
+
 What:  /sys/kernel/livepatch//
 Date:  Nov 2014
 KernelVersion: 3.19.0
diff --git a/Documentation/livepatch/livepatch.txt 
b/Documentation/livepatch/livepatch.txt
index 9bcdef277a36..896ba8941702 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -183,6 +183,20 @@ tasks. No proper signal is actually delivered (there is no 
data in signal
 pending structures). Tasks are interrupted or woken up, and forced to change
 their patched state.
 
+Administrator can also affect a transition through
+/sys/kernel/livepatch//force attribute. Writing 1 there clears
+TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to the patched
+state. Important note! The force attribute is intended for cases when the
+transition gets stuck for a long time because of a blocking task. Administrator
+is expected to collect all necessary data (namely stack traces of such blocking
+tasks) and request a clearance from a patch distributor to force the 
transition.
+Unauthorized usage may cause harm to the system. It depends on the nature of 
the
+patch, which functions are (un)patched, and which functions the blocking tasks
+are sleeping in (/proc//stack may help here). Removal (rmmod) of patch
+modules is permanently disabled when the force feature is used. It cannot be
+guaranteed there is no task sleeping in such module. It implies unbounded
+reference count if a patch module is disabled and enabled in a loop.
+
 3.1 Adding consistency model support to new architectures
 -
 
@@ -439,8 +453,8 @@ Information about the registered patches can be found under
 /sys/kernel/livepatch. The patches could be enabled and disabled
 by writing there.
 
-/sys/kernel/livepatch//signal attribute allows administrator to affect a
-patching operation.
+/sys/kernel/livepatch//signal and /sys/kernel/livepatch//force
+attributes allow administrator to affect a patching operation.
 
 See Documentation/ABI/testing/sysfs-kernel-livepatch for more details.
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e772be452462..c19c6c32c47e 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -441,6 +441,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch//enabled
  * /sys/kernel/livepatch//transition
  * /sys/kernel/livepatch//signal
+ * /sys/kernel/livepatch//force
  * /sys/kernel/livepatch//
  * /sys/kernel/livepatch///
  */
@@ -542,13 +543,42 @@ static ssize_t signal_store(struct kobject *kobj, struct 
kobj_attribute *attr,