[PATCH 0/3] livepatch: Introduce force sysfs attribute

[PATCH 0/3] livepatch: Introduce force sysfs attribute

[Miroslav Benes]

Currently, livepatch gradually migrate the system from an unpatched to a
patched state (or vice versa). Each task drops its TIF_PATCH_PENDING
itself when crossing the kernel/user space boundary or it is cleared
using the stack checking approach. If there is a task which sleeps on a
patched function, the whole transition can get stuck indefinitely.

Livepatch has means which can be used in these cases. The transition can
be cancelled and/or immediate flag may be used for the live patch. On
the other hand it might be useful to poke the system a little bit and
help the transition to finish by doing so.

That is what the fake signal can be used for. A task sleeping/waiting in
the kernel gets TIF_SIGPENDING set, it handles it and during that its
TIF_PATCH_PENDING is cleared. Kthreads are only woken up, they do not
handle signals suitably.

Still, there are cases which neither fake signal can solve. A task can
sleep uninterruptibly without reacting to signals at all. Even then, it
may be safe to clear the task's TIF_PATCH_PENDING. As a last resort,
admin may force such clearing for all tasks in the system with this
patch set.

We use the fake signal in SLES for a long time. Moreover, we don't have
a stack checking there, so we rely on the fake signal a lot. We send it
automatically and periodically.

The first patch is only preparatory. It introduces the sysfs attribute
through which both actions are performed. The second patch adds the fake
signal and the third one forced clearing of the flag.

Miroslav Benes (3):
  livepatch: Add force sysfs attribute
  livepatch: send a fake signal to all blocking tasks
  livepatch: force transition process to finish

 Documentation/ABI/testing/sysfs-kernel-livepatch |  9 ++++
 include/linux/livepatch.h                        |  4 ++
 kernel/livepatch/core.c                          | 51 +++++++++++++++++++++
 kernel/livepatch/transition.c                    | 56 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  2 +
 kernel/signal.c                                  |  4 +-
 6 files changed, 125 insertions(+), 1 deletion(-)

-- 
2.12.2

[PATCH 1/3] livepatch: Add force sysfs attribute

[Miroslav Benes]

Add write-only force attribute to livepatch sysfs infrastructure. We can
use it later to force couple of events during a live patching process.
Be it a sending of a fake signal or forcing of the tasks' successful
conversion.

It does not make sense to use the force facility when there is no
transaction running (although there is no harm doing that). Therefore we
limit only to situations when klp_transition_patch variable is set.
Normally, klp_mutex lock should be grabbed, because the variable is
shared. However that would hold the action back unnecessarily because of
waiting for the lock, so we omit the lock here. The resulting race
window is harmless (using force when there is no transaction running).

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  9 +++++
 kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index d5d39748382f..26e9f58cea9e 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -8,6 +8,15 @@ Contact:	live-patching@vger.kernel.org
 		The /sys/kernel/livepatch directory contains subdirectories for
 		each loaded live patch module.
 
+What:		/sys/kernel/livepatch/force
+Date:		May 2017
+KernelVersion:	4.13.0
+Contact:	live-patching@vger.kernel.org
+Description:
+		A write-only attribute that allows administrator to affect the
+		course of an existing transition. A fake signal can be send or
+		tasks TIF can be cleared.
+
 What:		/sys/kernel/livepatch/<patch>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..84f8944704ad 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -437,6 +437,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * Sysfs Interface
  *
  * /sys/kernel/livepatch
+ * /sys/kernel/livepatch/force
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
@@ -444,6 +445,43 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * klp_mutex lock is not grabbed here intentionally. It is not really
+	 * needed. The race window is harmless and grabbing the lock would only
+	 * hold the action back.
+	 */
+	if (!klp_transition_patch) {
+		pr_info("no patching in progress. Force not allowed\n");
+		return -EINVAL;
+	}
+
+	switch (val) {
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
+static struct attribute *klp_attrs[] = {
+	&force_kobj_attr.attr,
+	NULL
+};
+static struct attribute_group klp_sysfs_group = {
+	.attrs = klp_attrs,
+};
+
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 			     const char *buf, size_t count)
 {
@@ -954,6 +992,13 @@ static int __init klp_init(void)
 	if (!klp_root_kobj)
 		return -ENOMEM;
 
+	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
+	if (ret) {
+		pr_err("cannot create livepatch attributes in sysfs\n");
+		kobject_put(klp_root_kobj);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.12.2

Re: [PATCH 1/3] livepatch: Add force sysfs attribute

[Libor Pechacek]

On Thu 18-05-17 14:00:41, Miroslav Benes wrote:
> Add write-only force attribute to livepatch sysfs infrastructure. We can
> use it later to force couple of events during a live patching process.
> Be it a sending of a fake signal or forcing of the tasks' successful
> conversion.
> 
> It does not make sense to use the force facility when there is no
> transaction running (although there is no harm doing that). Therefore we
> limit only to situations when klp_transition_patch variable is set.
> Normally, klp_mutex lock should be grabbed, because the variable is
> shared. However that would hold the action back unnecessarily because of
> waiting for the lock, so we omit the lock here. The resulting race
> window is harmless (using force when there is no transaction running).
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  9 +++++
>  kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index d5d39748382f..26e9f58cea9e 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -8,6 +8,15 @@ Contact:	live-patching@vger.kernel.org
>  		The /sys/kernel/livepatch directory contains subdirectories for
>  		each loaded live patch module.
>  
> +What:		/sys/kernel/livepatch/force
> +Date:		May 2017
> +KernelVersion:	4.13.0
> +Contact:	live-patching@vger.kernel.org
> +Description:
> +		A write-only attribute that allows administrator to affect the
> +		course of an existing transition. A fake signal can be send or
								       ^^^^
								      "sent"

> +		tasks TIF can be cleared.
> +
>  What:		/sys/kernel/livepatch/<patch>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..84f8944704ad 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -437,6 +437,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * Sysfs Interface
>   *
>   * /sys/kernel/livepatch
> + * /sys/kernel/livepatch/force
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/transition
> @@ -444,6 +445,43 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
>   */
>  
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * klp_mutex lock is not grabbed here intentionally. It is not really
> +	 * needed. The race window is harmless and grabbing the lock would only
> +	 * hold the action back.
> +	 */
> +	if (!klp_transition_patch) {
> +		pr_info("no patching in progress. Force not allowed\n");

proposing smoother wording and information sharing
pr_info("no patching in progress, forced action (%d) ineffective", val);

> +		return -EINVAL;
> +	}
> +
> +	switch (val) {

I felt strong confusion for a while looking at a function what does nothing. A
comment that this is intentionally an empty shell, at this stage, would be
welcome.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
> +static struct attribute *klp_attrs[] = {
> +	&force_kobj_attr.attr,
> +	NULL
> +};
> +static struct attribute_group klp_sysfs_group = {
> +	.attrs = klp_attrs,
> +};
> +
>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			     const char *buf, size_t count)
>  {
> @@ -954,6 +992,13 @@ static int __init klp_init(void)
>  	if (!klp_root_kobj)
>  		return -ENOMEM;
>  
> +	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
> +	if (ret) {
> +		pr_err("cannot create livepatch attributes in sysfs\n");
> +		kobject_put(klp_root_kobj);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  

Libor

> -- 
> 2.12.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Libor Pechacek
SUSE Labs

Re: [PATCH 1/3] livepatch: Add force sysfs attribute

[Miroslav Benes]

On Thu, 18 May 2017, Libor Pechacek wrote:

> On Thu 18-05-17 14:00:41, Miroslav Benes wrote:
> >
> > +		pr_info("no patching in progress. Force not allowed\n");
> 
> proposing smoother wording and information sharing
> pr_info("no patching in progress, forced action (%d) ineffective", val);

That is better. Thanks.

> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (val) {
> 
> I felt strong confusion for a while looking at a function what does nothing. A
> comment that this is intentionally an empty shell, at this stage, would be
> welcome.

Yes, I wanted to keep that sysfs glue separate from both implementations 
to make the review easier. Despite the ugly outcome. And it is confusing.

Comment sounds good.

Thanks,
Miroslav

> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return count;
> > +}

[PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

Live patching consistency model is of LEAVE_PATCHED_SET and
SWITCH_THREAD. This means that all tasks in the system have to be marked
one by one as safe to call a new patched function. Safe means when a
task is not (sleeping) in a set of patched functions. That is, no
patched function is on the task's stack. Another clearly safe place is
the boundary between kernel and userspace. The patching waits for all
tasks to get outside of the patched set or to cross the boundary. The
transition is completed afterwards.

The problem is that a task can block the transition for quite a long
time, if not forever. It could sleep in a set of patched functions, for
example.  Luckily we can force the task to leave the set by sending it a
fake signal, that is a signal with no data in signal pending structures
(no handler, no sign of proper signal delivered). Suspend/freezer use
this to freeze the tasks as well. The task gets TIF_SIGPENDING set and
is woken up (if it has been sleeping in the kernel before) or kicked by
rescheduling IPI (if it was running on other CPU). This causes the task
to go to kernel/userspace boundary where the signal would be handled and
the task would be marked as safe in terms of live patching.

There are tasks which are not affected by this technique though. The
fake signal is not sent to kthreads. They should be handled in a
different way. They can be woken up so they leave the patched set and
their TIF_PATCH_PENDING can be cleared thanks to stack checking.

For the sake of completeness, if the task is in TASK_RUNNING state but
not currently running on some CPU it doesn't get the IPI, but it would
eventually handle the signal anyway. Second, if the task runs in the
kernel (in TASK_RUNNING state) it gets the IPI, but the signal is not
handled on return from the interrupt. It would be handled on return to
the userspace in the future when the fake signal is sent again. Stack
checking deals with these cases in a better way.

If the task was sleeping in a syscall it would be woken by our fake
signal, it would check if TIF_SIGPENDING is set (by calling
signal_pending() predicate) and return ERESTART* or EINTR. Syscalls with
ERESTART* return values are restarted in case of the fake signal (see
do_signal()). EINTR is propagated back to the userspace program. This
could disturb the program, but...

* each process dealing with signals should react accordingly to EINTR
  return values.
* syscalls returning EINTR happen to be quite common situation in the
  system even if no fake signal is sent.
* freezer sends the fake signal and does not deal with EINTR anyhow.
  Thus EINTR values are returned when the system is resumed.

The very safe marking is done in entry.S on syscall and
interrupt/exception exit paths, and in a stack checking functions of
livepatch.  TIF_PATCH_PENDING is cleared and the next
recalc_sigpending() drops TIF_SIGPENDING.

Note that the fake signal is not sent to stopped/traced tasks. Such task
prevents the patching to finish till it continues again (is not traced
anymore).

Last, sending the fake signal is not automatic. It is done only when
admin requests it by writing 1 to force sysfs attribute in livepatch
sysfs directory.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h     |  3 +++
 kernel/livepatch/core.c       |  3 +++
 kernel/livepatch/transition.c | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/livepatch/transition.h |  1 +
 kernel/signal.c               |  4 +++-
 5 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..43cfeebeb42b 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -29,6 +29,9 @@
 
 #include <asm/livepatch.h>
 
+/* values for sysfs force attribute */
+#define KLP_FORCE_FAKE		1
+
 /* task patch states */
 #define KLP_UNDEFINED	-1
 #define KLP_UNPATCHED	 0
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 84f8944704ad..bb3b78fa7d2b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -466,6 +466,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 	}
 
 	switch (val) {
+	case KLP_FORCE_FAKE:
+		klp_send_fake_signal();
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index adc0cc64aa4b..bb61aaa196d3 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -551,3 +551,43 @@ void klp_copy_process(struct task_struct *child)
 
 	/* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
 }
+
+/*
+ * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.
+ * Kthreads with TIF_PATCH_PENDING set are woken up. Only admin can request this
+ * action currently.
+ */
+void klp_send_fake_signal(void)
+{
+	struct task_struct *g, *task;
+
+	pr_info("sending a fake signal and waking sleeping kthreads up\n");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		if (!klp_patch_pending(task))
+			continue;
+
+		/*
+		 * There is a small race here. We could see TIF_PATCH_PENDING
+		 * set and decide to wake up a kthread or send a fake signal.
+		 * Meanwhile the task could migrate itself and the action
+		 * would be meaningless. It is not serious though.
+		 */
+		if (task->flags & PF_KTHREAD) {
+			/*
+			 * Wake up a kthread which still has not been migrated.
+			 */
+			wake_up_process(task);
+		} else {
+			/*
+			 * Send fake signal to all non-kthread tasks which are
+			 * still not migrated.
+			 */
+			spin_lock_irq(&task->sighand->siglock);
+			signal_wake_up(task, 0);
+			spin_unlock_irq(&task->sighand->siglock);
+		}
+	}
+	read_unlock(&tasklist_lock);
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index ce09b326546c..1c7ede6eaa77 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -10,5 +10,6 @@ void klp_cancel_transition(void);
 void klp_start_transition(void);
 void klp_try_complete_transition(void);
 void klp_reverse_transition(void);
+void klp_send_fake_signal(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index 7e59ebc2c25e..3a25cc06231d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -39,6 +39,7 @@
 #include <linux/compat.h>
 #include <linux/cn_proc.h>
 #include <linux/compiler.h>
+#include <linux/livepatch.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>
@@ -162,7 +163,8 @@ void recalc_sigpending_and_wake(struct task_struct *t)
 
 void recalc_sigpending(void)
 {
-	if (!recalc_sigpending_tsk(current) && !freezing(current))
+	if (!recalc_sigpending_tsk(current) && !freezing(current) &&
+	    !klp_patch_pending(current))
 		clear_thread_flag(TIF_SIGPENDING);
 
 }
-- 
2.12.2

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Libor Pechacek]

On Thu 18-05-17 14:00:42, Miroslav Benes wrote:
[...]
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -29,6 +29,9 @@
>  
>  #include <asm/livepatch.h>
>  
> +/* values for sysfs force attribute */
> +#define KLP_FORCE_FAKE		1
> +

Should be documented in Documentation/ABI/testing/sysfs-kernel-livepatch for
end users.

Libor
-- 
Libor Pechacek
SUSE Labs

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Thu, 18 May 2017, Libor Pechacek wrote:

> On Thu 18-05-17 14:00:42, Miroslav Benes wrote:
> [...]
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -29,6 +29,9 @@
> >  
> >  #include <asm/livepatch.h>
> >  
> > +/* values for sysfs force attribute */
> > +#define KLP_FORCE_FAKE		1
> > +
> 
> Should be documented in Documentation/ABI/testing/sysfs-kernel-livepatch for
> end users.

Ugh, I forgot. Thanks for reminding me.

Miroslav

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Oleg Nesterov]

I didn't see other patches in series, not sure I understand...

On 05/18, Miroslav Benes wrote:
>
> The very safe marking is done in entry.S on syscall and
> interrupt/exception exit paths, and in a stack checking functions of
> livepatch.  TIF_PATCH_PENDING is cleared and the next
> recalc_sigpending() drops TIF_SIGPENDING.

Confused. The task can't return from do_signal() is signal_pending() is
true, thus it will spin forever if klp_patch_pending(current)) is true.
"forever" means until something else clears TIF_PATCH_PENDING, of course.

exit_to_usermode_loop() calls do_signal(), then klp_update_patch_state().
So it won't be cleared here.

Even if you change the order, this won't help unless I missed something,
TIF_PATCH_PENDING can be set when this task has already entered do_signal().

> Last, sending the fake signal is not automatic. It is done only when
> admin requests it by writing 1 to force sysfs attribute in livepatch
> sysfs directory.

OK, but see above, even if klp_send_fake_signal() is never called, the
a task will get this fake signal when it calls recalc_sigpending().

Oleg.

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Thu, 18 May 2017, Oleg Nesterov wrote:

> I didn't see other patches in series, not sure I understand...

There is nothing relevant to this patch, I think. I did not want to bother
you with it.

> On 05/18, Miroslav Benes wrote:
> >
> > The very safe marking is done in entry.S on syscall and
> > interrupt/exception exit paths, and in a stack checking functions of
> > livepatch.  TIF_PATCH_PENDING is cleared and the next
> > recalc_sigpending() drops TIF_SIGPENDING.
> 
> Confused. The task can't return from do_signal() is signal_pending() is
> true, thus it will spin forever if klp_patch_pending(current)) is true.
> "forever" means until something else clears TIF_PATCH_PENDING, of course.
>
> exit_to_usermode_loop() calls do_signal(), then klp_update_patch_state().
> So it won't be cleared here.

Ok, so maybe I misunderstand the code. I see the loop in
exit_to_usermode_loop() for processing ALLWORK_MASK. There we call
do_signal(). We go to get_signal(). The infinite loop there is relevant
for us. We call dequeue_signal(). There, if I am not mistaken
__dequeue_signal() would return 0 in our case, because there is no real
signal pending and thus nothing in the signal data structures.
recalc_sigpending() is called and TIF_SIGPENDING is preserved there (I
presume TIF_PATCH_PENDING is set). signr is zero, dequeue_signal() returns
0. Back in get_signal() the loop is broken and zero is return. Then
do_signal() may or may not restart the syscall.
  
If not, we get back to exit_to_usermode_loop() and TIF_PATCH_PENDING is 
cleared. Yes, it is true that TIF_SIGPENDING is still set and we get to
do_signal() once more. But for the last time.

If the syscall is restarted, it may be different. I have to think about
this one. But...

> Even if you change the order, this won't help unless I missed something,
> TIF_PATCH_PENDING can be set when this task has already entered do_signal().

...I think it could be solved with this anyway. And of course it should 
solve the double call to do_signal() I described above.

Damn, I fixed exactly this in SLES a year or so ago and there is a note I 
did the same in proposed version for upstream. It must have fallen through
the cracks.


So, am I wrong somewhere? It could be anywhere, because it is quite 
confusing.

Regards,
Miroslav

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Oleg Nesterov]

On 05/18, Miroslav Benes wrote:
>
> On Thu, 18 May 2017, Oleg Nesterov wrote:
>
> >
> > exit_to_usermode_loop() calls do_signal(), then klp_update_patch_state().
> > So it won't be cleared here.
>
> Ok, so maybe I misunderstand the code. I see the loop in
> exit_to_usermode_loop() for processing ALLWORK_MASK. There we call
> do_signal(). We go to get_signal(). The infinite loop there is relevant
> for us. We call dequeue_signal(). There, if I am not mistaken
> __dequeue_signal() would return 0

Yes, sorry, I didn't bother to read the code when I looked at your patch
and my memory fooled me.

> If not, we get back to exit_to_usermode_loop() and TIF_PATCH_PENDING is 
> cleared. Yes, it is true that TIF_SIGPENDING is still set and we get to
> do_signal() once more. But for the last time.

Yes, slightly sub-optimal but not really wrong and you can swap
do_signal() and klp_update_patch_state().

> If the syscall is restarted, it may be different. I have to think about
> this one. But...

Afaics, there are no problems.


In short. Thanks for correcting me and sorry for noise!

Oleg.

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

> > If not, we get back to exit_to_usermode_loop() and TIF_PATCH_PENDING is 
> > cleared. Yes, it is true that TIF_SIGPENDING is still set and we get to
> > do_signal() once more. But for the last time.
> 
> Yes, slightly sub-optimal but not really wrong and you can swap
> do_signal() and klp_update_patch_state().

Ok. I'll add it to v2.
 
> > If the syscall is restarted, it may be different. I have to think about
> > this one. But...
> 
> Afaics, there are no problems.
> 
> 
> In short. Thanks for correcting me and sorry for noise!

Thanks for the review, Oleg. Much appreciated.

Miroslav

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Josh Poimboeuf]

On Thu, May 18, 2017 at 02:00:42PM +0200, Miroslav Benes wrote:
> @@ -551,3 +551,43 @@ void klp_copy_process(struct task_struct *child)
>  
>  	/* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
>  }
> +
> +/*
> + * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.
> + * Kthreads with TIF_PATCH_PENDING set are woken up. Only admin can request this
> + * action currently.
> + */
> +void klp_send_fake_signal(void)
> +{
> +	struct task_struct *g, *task;
> +
> +	pr_info("sending a fake signal and waking sleeping kthreads up\n");

Maybe this should be pr_notice(), for consistency with our other
printks.

Also I wonder if the message can be made more meaningful to the user.
The "fake" part of the signal and the "waking sleeping kthreads" bit
could be too much information for the user, IMO.  How about "signaling
remaining tasks"?  Just an idea.

-- 
Josh

Re: [PATCH 2/3] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Tue, 23 May 2017, Josh Poimboeuf wrote:

> On Thu, May 18, 2017 at 02:00:42PM +0200, Miroslav Benes wrote:
> > @@ -551,3 +551,43 @@ void klp_copy_process(struct task_struct *child)
> >  
> >  	/* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
> >  }
> > +
> > +/*
> > + * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.
> > + * Kthreads with TIF_PATCH_PENDING set are woken up. Only admin can request this
> > + * action currently.
> > + */
> > +void klp_send_fake_signal(void)
> > +{
> > +	struct task_struct *g, *task;
> > +
> > +	pr_info("sending a fake signal and waking sleeping kthreads up\n");
> 
> Maybe this should be pr_notice(), for consistency with our other
> printks.
> 
> Also I wonder if the message can be made more meaningful to the user.
> The "fake" part of the signal and the "waking sleeping kthreads" bit
> could be too much information for the user, IMO.  How about "signaling
> remaining tasks"?  Just an idea.

Good one. I'll change it in v2.

Thanks,
Miroslav

[PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

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@suse.cz>
---
 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;
 	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");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_patch_state(task);
+	read_unlock(&tasklist_lock);
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 1c7ede6eaa77..c129397b3985 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -11,5 +11,6 @@ void klp_start_transition(void);
 void klp_try_complete_transition(void);
 void klp_reverse_transition(void);
 void klp_send_fake_signal(void);
+void klp_unmark_tasks(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.12.2

Re: [PATCH 3/3] livepatch: force transition process to finish

[Libor Pechacek]

On Thu 18-05-17 14:00:43, Miroslav Benes wrote:
[...]
> 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.
[...]
> --- 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

Again, needs documentation. I would consider using symbolic names instead of
numbers, like /sys/power/state does, even more friendly. Possibility to read a
list of available actions would help user space tools identify what is
available in case the list grows in the future.

Libor
-- 
Libor Pechacek
SUSE Labs

Re: [PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Thu, 18 May 2017, Libor Pechacek wrote:

> On Thu 18-05-17 14:00:43, Miroslav Benes wrote:
> [...]
> > 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.
> [...]
> > --- 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
> 
> Again, needs documentation. I would consider using symbolic names instead of
> numbers, like /sys/power/state does, even more friendly. Possibility to read a
> list of available actions would help user space tools identify what is
> available in case the list grows in the future.

I like that! -> v2

Thanks,
Miroslav

Re: [PATCH 3/3] livepatch: force transition process to finish

[Josh Poimboeuf]

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@suse.cz>

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

Re: [PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Tue, 23 May 2017, Josh Poimboeuf wrote:

> 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@suse.cz>
> 
> 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()

Yes, that is better.
 
> >  	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?

Ok.

> > +
> > +	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?

Correct. I'll add a comment to klp_unmark_tasks()/klp_force_transitions().

> On a related note, I think the new sysfs entry should also be documented
> in Documentation/livepatch/livepatch.txt somewhere.

No problem.

Thanks,
Miroslav

Re: [PATCH 3/3] livepatch: force transition process to finish

[Petr Mladek]

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.

Re: [PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Wed, 24 May 2017, Petr Mladek wrote:

> 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.

This should not happen. klp_update_patch_state() use 
test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING) and only if true, 
task->patch_state is set.

And all TIF_PATCH_PENDING are cleared when you get 
klp_complete_transition().
 
> 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.

I still think it is better not do it. klp_unmark_tasks() does nothing else 
than tasks already do. They call klp_update_patch_state() by themselves 
and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
only forces this action.

On the other hand, I do not see a problem in doing that. We already have a 
relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
is safe. It would only serialize things needlessly.

Thanks,
Miroslav

Re: [PATCH 3/3] livepatch: force transition process to finish

[Petr Mladek]

On Wed 2017-05-24 16:15:49, Miroslav Benes wrote:
> On Wed, 24 May 2017, Petr Mladek wrote:
> 
> > 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.
> 
> This should not happen. klp_update_patch_state() use 
> test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING) and only if true, 
> task->patch_state is set.
> 
> And all TIF_PATCH_PENDING are cleared when you get 
> klp_complete_transition().

You are right. I missed that klp_update_patch_state() checked
the TIF flag before setting the state.


> > In fact, I would suggest to take klp_mutex in force_store()
> > and do all actions synchronously, including the check
> > of klp_transition_patch.
> 
> I still think it is better not do it. klp_unmark_tasks() does nothing else 
> than tasks already do. They call klp_update_patch_state() by themselves 
> and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> only forces this action.

You have a point. But I am not convinced ;-) klp_update_patch_state()
was called very carefully only when it was safe. The forcing
intentionally breaks the consistency model. User should really know
what they are doing when they use this feature.

I think that we should actually taint the kernel. Developers should
know when users were pulling their legs.


> On the other hand, I do not see a problem in doing that. We already have a 
> relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> is safe.

Yup.

> It would only serialize things needlessly.

I do not agree. The speed is not important here. Also look
into klp_reverse_transition(). We explicitly clear all
TIF_PATCH_PENDING flags and call synchronize_rcu() just
to make the situation easier and reduce space for potential
mistakes.

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

> > > In fact, I would suggest to take klp_mutex in force_store()
> > > and do all actions synchronously, including the check
> > > of klp_transition_patch.
> > 
> > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > than tasks already do. They call klp_update_patch_state() by themselves 
> > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > only forces this action.
> 
> You have a point. But I am not convinced ;-) klp_update_patch_state()
> was called very carefully only when it was safe. The forcing
> intentionally breaks the consistency model. User should really know
> what they are doing when they use this feature.
> 
> I think that we should actually taint the kernel. Developers should
> know when users were pulling their legs.

We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
course taint the kernel.
 
> > On the other hand, I do not see a problem in doing that. We already have a 
> > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > is safe.
> 
> Yup.
> 
> > It would only serialize things needlessly.
> 
> I do not agree. The speed is not important here. Also look
> into klp_reverse_transition(). We explicitly clear all
> TIF_PATCH_PENDING flags and call synchronize_rcu() just
> to make the situation easier and reduce space for potential
> mistakes.

Yes, because we had to do that. We ran into problems otherwise. We do not 
have to do it here. It does not help anything in my opinion.

Miroslav

Re: [PATCH 3/3] livepatch: force transition process to finish

[Petr Mladek]

On Thu 2017-05-25 14:59:55, Miroslav Benes wrote:
> 
> > > > In fact, I would suggest to take klp_mutex in force_store()
> > > > and do all actions synchronously, including the check
> > > > of klp_transition_patch.
> > > 
> > > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > > than tasks already do. They call klp_update_patch_state() by themselves 
> > > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > > only forces this action.
> > 
> > You have a point. But I am not convinced ;-) klp_update_patch_state()
> > was called very carefully only when it was safe. The forcing
> > intentionally breaks the consistency model. User should really know
> > what they are doing when they use this feature.
> > 
> > I think that we should actually taint the kernel. Developers should
> > know when users were pulling their legs.
> 
> We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
> course taint the kernel.

Sounds good to me.


> > > On the other hand, I do not see a problem in doing that. We already have a 
> > > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > > is safe.
> > 
> > Yup.
> > 
> > > It would only serialize things needlessly.
> > 
> > I do not agree. The speed is not important here. Also look
> > into klp_reverse_transition(). We explicitly clear all
> > TIF_PATCH_PENDING flags and call synchronize_rcu() just
> > to make the situation easier and reduce space for potential
> > mistakes.
> 
> Yes, because we had to do that. We ran into problems otherwise. We do not 
> have to do it here. It does not help anything in my opinion.

AFAIK, we did not have to do it, see
https://lkml.kernel.org/r/20161222143452.GK25166@pathway.suse.cz
and the comment starting with "It would still leave a small".

Just for record, the idea of disabling the TIF flags came from Josh
in another mail. I have just repeated it.

I think that the problem already is complex enough and the
serialization would reduce the space of potential races.
But it is possible that I see it just too complex here.

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: force transition process to finish

[Josh Poimboeuf]

On Thu, May 25, 2017 at 06:03:07PM +0200, Petr Mladek wrote:
> On Thu 2017-05-25 14:59:55, Miroslav Benes wrote:
> > 
> > > > > In fact, I would suggest to take klp_mutex in force_store()
> > > > > and do all actions synchronously, including the check
> > > > > of klp_transition_patch.
> > > > 
> > > > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > > > than tasks already do. They call klp_update_patch_state() by themselves 
> > > > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > > > only forces this action.
> > > 
> > > You have a point. But I am not convinced ;-) klp_update_patch_state()
> > > was called very carefully only when it was safe. The forcing
> > > intentionally breaks the consistency model. User should really know
> > > what they are doing when they use this feature.
> > > 
> > > I think that we should actually taint the kernel. Developers should
> > > know when users were pulling their legs.
> > 
> > We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
> > course taint the kernel.
> 
> Sounds good to me.

I'm thinking that WARN_ON_ONCE() seems too severe.  If the patch didn't
need a consistency model in the first place then it wouldn't be worth
warning about.

We have to trust that the user knows what they're doing.  And that's
true for the entire live patching process, including patch analysis and
patch creation.  And anyway we already have a taint flag for that:
TAINT_LIVEPATCH.

> > > > On the other hand, I do not see a problem in doing that. We already have a 
> > > > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > > > is safe.
> > > 
> > > Yup.
> > > 
> > > > It would only serialize things needlessly.
> > > 
> > > I do not agree. The speed is not important here. Also look
> > > into klp_reverse_transition(). We explicitly clear all
> > > TIF_PATCH_PENDING flags and call synchronize_rcu() just
> > > to make the situation easier and reduce space for potential
> > > mistakes.
> > 
> > Yes, because we had to do that. We ran into problems otherwise. We do not 
> > have to do it here. It does not help anything in my opinion.
> 
> AFAIK, we did not have to do it, see
> https://lkml.kernel.org/r/20161222143452.GK25166@pathway.suse.cz
> and the comment starting with "It would still leave a small".
> 
> Just for record, the idea of disabling the TIF flags came from Josh
> in another mail. I have just repeated it.
> 
> I think that the problem already is complex enough and the
> serialization would reduce the space of potential races.
> But it is possible that I see it just too complex here.

IMO we can skip the mutex.  The consistency model will be broken anyway,
so all bets are off.

-- 
Josh

Re: [PATCH 3/3] livepatch: force transition process to finish

[Petr Mladek]

On Fri 2017-05-26 12:37:56, Josh Poimboeuf wrote:
> On Thu, May 25, 2017 at 06:03:07PM +0200, Petr Mladek wrote:
> > On Thu 2017-05-25 14:59:55, Miroslav Benes wrote:
> > > 
> > > > > > In fact, I would suggest to take klp_mutex in force_store()
> > > > > > and do all actions synchronously, including the check
> > > > > > of klp_transition_patch.
> > > > > 
> > > > > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > > > > than tasks already do. They call klp_update_patch_state() by themselves 
> > > > > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > > > > only forces this action.
> > > > 
> > > > You have a point. But I am not convinced ;-) klp_update_patch_state()
> > > > was called very carefully only when it was safe. The forcing
> > > > intentionally breaks the consistency model. User should really know
> > > > what they are doing when they use this feature.
> > > > 
> > > > I think that we should actually taint the kernel. Developers should
> > > > know when users were pulling their legs.
> > > 
> > > We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
> > > course taint the kernel.
> > 
> > Sounds good to me.
> 
> I'm thinking that WARN_ON_ONCE() seems too severe.  If the patch didn't
> need a consistency model in the first place then it wouldn't be worth
> warning about.
> 
> We have to trust that the user knows what they're doing.  And that's
> true for the entire live patching process, including patch analysis and
> patch creation.  And anyway we already have a taint flag for that:
> TAINT_LIVEPATCH.

But the force is done on the user side. Let's say that the authors of
the livepatch code and of the patches know what they are doing.
Could we expect the same from the admins that apply the patches?

TAINT_LIVEPATCH is set because the system behaves differently
than with the original code. But it still should be consistent.
Using the force migration might move the system to a wonder land.


> > > > > On the other hand, I do not see a problem in doing that. We already have a 
> > > > > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > > > > is safe.
> > > > 
> > > > Yup.
> > > > 
> > > > > It would only serialize things needlessly.
> > > > 
> > > > I do not agree. The speed is not important here. Also look
> > > > into klp_reverse_transition(). We explicitly clear all
> > > > TIF_PATCH_PENDING flags and call synchronize_rcu() just
> > > > to make the situation easier and reduce space for potential
> > > > mistakes.
> > > 
> > > Yes, because we had to do that. We ran into problems otherwise. We do not 
> > > have to do it here. It does not help anything in my opinion.
> > 
> > AFAIK, we did not have to do it, see
> > https://lkml.kernel.org/r/20161222143452.GK25166@pathway.suse.cz
> > and the comment starting with "It would still leave a small".
> > 
> > Just for record, the idea of disabling the TIF flags came from Josh
> > in another mail. I have just repeated it.
> > 
> > I think that the problem already is complex enough and the
> > serialization would reduce the space of potential races.
> > But it is possible that I see it just too complex here.
> 
> IMO we can skip the mutex.  The consistency model will be broken anyway,
> so all bets are off.

I just hope that I will never be forced to debug a system crash
after this operation.

Imagine a situation when we send a livepatch using the hybrid
consistency model that should be safe also in the immediate mode.
Some processes would get stacked. We suggest forcing because
it should be safe. And it will break. Then we will want to know
why this has happened. If the forcing is not serialized, we will
need to consider/check much more parallel operations.

But if I am the only one who think this way, it might mean
that I am over-pessimistic in this context. I will buy
some head bandage to be prepared and could live without
the serialization.

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: force transition process to finish

[Josh Poimboeuf]

On Mon, May 29, 2017 at 02:28:13PM +0200, Petr Mladek wrote:
> On Fri 2017-05-26 12:37:56, Josh Poimboeuf wrote:
> > On Thu, May 25, 2017 at 06:03:07PM +0200, Petr Mladek wrote:
> > > On Thu 2017-05-25 14:59:55, Miroslav Benes wrote:
> > > > 
> > > > > > > In fact, I would suggest to take klp_mutex in force_store()
> > > > > > > and do all actions synchronously, including the check
> > > > > > > of klp_transition_patch.
> > > > > > 
> > > > > > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > > > > > than tasks already do. They call klp_update_patch_state() by themselves 
> > > > > > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > > > > > only forces this action.
> > > > > 
> > > > > You have a point. But I am not convinced ;-) klp_update_patch_state()
> > > > > was called very carefully only when it was safe. The forcing
> > > > > intentionally breaks the consistency model. User should really know
> > > > > what they are doing when they use this feature.
> > > > > 
> > > > > I think that we should actually taint the kernel. Developers should
> > > > > know when users were pulling their legs.
> > > > 
> > > > We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
> > > > course taint the kernel.
> > > 
> > > Sounds good to me.
> > 
> > I'm thinking that WARN_ON_ONCE() seems too severe.  If the patch didn't
> > need a consistency model in the first place then it wouldn't be worth
> > warning about.
> > 
> > We have to trust that the user knows what they're doing.  And that's
> > true for the entire live patching process, including patch analysis and
> > patch creation.  And anyway we already have a taint flag for that:
> > TAINT_LIVEPATCH.
> 
> But the force is done on the user side. Let's say that the authors of
> the livepatch code and of the patches know what they are doing.
> Could we expect the same from the admins that apply the patches?
> 
> TAINT_LIVEPATCH is set because the system behaves differently
> than with the original code. But it still should be consistent.
> Using the force migration might move the system to a wonder land.

True.  If the patch creators don't want the user to "use the force", the
WARNING would be appropriate.  Otherwise, if the patch creators *do*
want the user to force, the WARNING will be overkill and may alarm the
user.

When the user is root, we always have to trust them to a certain extent.
So I'd be more worried about dealing with the fallout of the false
WARNING.  But I don't feel strongly about it either way.

> > > > > > On the other hand, I do not see a problem in doing that. We already have a 
> > > > > > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > > > > > is safe.
> > > > > 
> > > > > Yup.
> > > > > 
> > > > > > It would only serialize things needlessly.
> > > > > 
> > > > > I do not agree. The speed is not important here. Also look
> > > > > into klp_reverse_transition(). We explicitly clear all
> > > > > TIF_PATCH_PENDING flags and call synchronize_rcu() just
> > > > > to make the situation easier and reduce space for potential
> > > > > mistakes.
> > > > 
> > > > Yes, because we had to do that. We ran into problems otherwise. We do not 
> > > > have to do it here. It does not help anything in my opinion.
> > > 
> > > AFAIK, we did not have to do it, see
> > > https://lkml.kernel.org/r/20161222143452.GK25166@pathway.suse.cz
> > > and the comment starting with "It would still leave a small".
> > > 
> > > Just for record, the idea of disabling the TIF flags came from Josh
> > > in another mail. I have just repeated it.
> > > 
> > > I think that the problem already is complex enough and the
> > > serialization would reduce the space of potential races.
> > > But it is possible that I see it just too complex here.
> > 
> > IMO we can skip the mutex.  The consistency model will be broken anyway,
> > so all bets are off.
> 
> I just hope that I will never be forced to debug a system crash
> after this operation.
> 
> Imagine a situation when we send a livepatch using the hybrid
> consistency model that should be safe also in the immediate mode.
> Some processes would get stacked. We suggest forcing because
> it should be safe. And it will break. Then we will want to know
> why this has happened. If the forcing is not serialized, we will
> need to consider/check much more parallel operations.
> 
> But if I am the only one who think this way, it might mean
> that I am over-pessimistic in this context. I will buy
> some head bandage to be prepared and could live without
> the serialization.

I don't feel strongly about this one either way either.  But we all
agree that it doesn't need the mutex now, and I can't think of a
scenario where the code would change such that it would need it.

-- 
Josh

Re: [PATCH 3/3] livepatch: force transition process to finish

[Josh Poimboeuf]

On Thu, May 18, 2017 at 02:00:43PM +0200, Miroslav Benes wrote:
> @@ -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);
> +}

Should this also force the idle tasks to transition?

-- 
Josh

Re: [PATCH 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Fri, 26 May 2017, Josh Poimboeuf wrote:

> On Thu, May 18, 2017 at 02:00:43PM +0200, Miroslav Benes wrote:
> > @@ -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);
> > +}
> 
> Should this also force the idle tasks to transition?

Yes. Thanks.

Miroslav