[PATCH v4 0/2] livepatch: Introduce signal and force sysfs attributes

[PATCH v4 0/2] livepatch: Introduce signal and force sysfs attributes

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

Petr, I did not add you Reviewed-by tags intentionally because of the changes.

Changes from v3:
- only TASK_INTERRUPTIBLE kthreads are woken up - Petr
- documentation - Pavel
- function naming and sysfs fix - Josh

Changes from v2:
- two sysfs attributes instead of one - Petr, Josh
- better documentation about force usage - Pavel
- small changes here and there

Changes from v1:
- better wording, typos, comments, documentation - Libor, Josh
- symbolic names in sysfs instead of numbers - Libor
- exit_to_usermode_loop(), call klp_update_patch_state() before do_signal() - Oleg
- better names - Josh
- mutex and WARN_ON_ONCE not added to klp_force_transitions() - Petr, Josh
- handle idle tasks in klp_force_transitions() too - Josh

Miroslav Benes (2):
  livepatch: send a fake signal to all blocking tasks
  livepatch: force transition to finish

 Documentation/ABI/testing/sysfs-kernel-livepatch | 25 +++++++++
 Documentation/livepatch/livepatch.txt            | 22 +++++++-
 arch/powerpc/kernel/signal.c                     |  6 +--
 arch/x86/entry/common.c                          |  6 +--
 kernel/livepatch/core.c                          | 60 +++++++++++++++++++++
 kernel/livepatch/transition.c                    | 66 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  2 +
 kernel/signal.c                                  |  4 +-
 8 files changed, 182 insertions(+), 9 deletions(-)

-- 
2.15.0

[PATCH v4 1/2] 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 differently.
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 architectures' "entry" 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. In connection with this, also
call klp_update_patch_state() before do_signal(), so that
recalc_sigpending() in dequeue_signal() can clear TIF_PATCH_PENDING
immediately and thus prevent a double call of do_signal().

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 signal sysfs attribute in livepatch
sysfs directory.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: x86@kernel.org
---
 Documentation/ABI/testing/sysfs-kernel-livepatch | 12 +++++++
 Documentation/livepatch/livepatch.txt            | 11 +++++--
 arch/powerpc/kernel/signal.c                     |  6 ++--
 arch/x86/entry/common.c                          |  6 ++--
 kernel/livepatch/core.c                          | 30 +++++++++++++++++
 kernel/livepatch/transition.c                    | 41 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 kernel/signal.c                                  |  4 ++-
 8 files changed, 102 insertions(+), 9 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index d5d39748382f..3bb9d5bc1ce3 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,6 +33,18 @@ Contact:	live-patching@vger.kernel.org
 		An attribute which indicates whether the patch is currently in
 		transition.
 
+What:		/sys/kernel/livepatch/<patch>/signal
+Date:		Nov 2017
+KernelVersion:	4.15.0
+Contact:	live-patching@vger.kernel.org
+Description:
+		A writable attribute that allows administrator to affect the
+		course of an existing transition. Writing 1 sends a fake
+		signal to all remaining blocking tasks. The fake signal
+		means that no proper signal is delivered (there is no data in
+		signal pending structures). Tasks are interrupted or woken up,
+		and forced to change their patched state.
+
 What:		/sys/kernel/livepatch/<patch>/<object>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index ecdb18104ab0..9bcdef277a36 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -176,8 +176,12 @@ If a patch is in transition, this file shows 0 to indicate the task is
 unpatched and 1 to indicate it's patched.  Otherwise, if no patch is in
 transition, it shows -1.  Any tasks which are blocking the transition
 can be signaled with SIGSTOP and SIGCONT to force them to change their
-patched state.
-
+patched state. This may be harmful to the system though.
+/sys/kernel/livepatch/<patch>/signal attribute provides a better alternative.
+Writing 1 to the attribute sends a fake signal to all remaining blocking
+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.
 
 3.1 Adding consistency model support to new architectures
 ---------------------------------------------------------
@@ -435,6 +439,9 @@ 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/<patch>/signal attribute allows administrator to affect a
+patching operation.
+
 See Documentation/ABI/testing/sysfs-kernel-livepatch for more details.
 
 
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e9436c5e1e09..bf9c4e7792d1 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -153,6 +153,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
 
+	if (thread_info_flags & _TIF_PATCH_PENDING)
+		klp_update_patch_state(current);
+
 	if (thread_info_flags & _TIF_SIGPENDING) {
 		BUG_ON(regs != current->thread.regs);
 		do_signal(current);
@@ -163,9 +166,6 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 		tracehook_notify_resume(regs);
 	}
 
-	if (thread_info_flags & _TIF_PATCH_PENDING)
-		klp_update_patch_state(current);
-
 	user_enter();
 }
 
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 03505ffbe1b6..fb90bdb3602f 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -153,6 +153,9 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		if (cached_flags & _TIF_UPROBE)
 			uprobe_notify_resume(regs);
 
+		if (cached_flags & _TIF_PATCH_PENDING)
+			klp_update_patch_state(current);
+
 		/* deal with pending signal delivery */
 		if (cached_flags & _TIF_SIGPENDING)
 			do_signal(regs);
@@ -165,9 +168,6 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
 			fire_user_return_notifiers();
 
-		if (cached_flags & _TIF_PATCH_PENDING)
-			klp_update_patch_state(current);
-
 		/* Disable IRQs and retry */
 		local_irq_disable();
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bf8c8fd72589..e772be452462 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -440,6 +440,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
+ * /sys/kernel/livepatch/<patch>/signal
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
@@ -514,11 +515,40 @@ static ssize_t transition_show(struct kobject *kobj,
 			patch == klp_transition_patch);
 }
 
+static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct klp_patch *patch;
+	int ret;
+	bool val;
+
+	patch = container_of(kobj, struct klp_patch, kobj);
+
+	/*
+	 * 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 (patch != klp_transition_patch)
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		klp_send_signals();
+
+	return count;
+}
+
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
 static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
+static struct kobj_attribute signal_kobj_attr = __ATTR_WO(signal);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
+	&signal_kobj_attr.attr,
 	NULL
 };
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1fb6032..095aa8d864f5 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -577,3 +577,44 @@ 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_signals(void)
+{
+	struct task_struct *g, *task;
+
+	pr_notice("signaling remaining tasks\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 sleeps interruptedly and
+			 * still has not been migrated.
+			 */
+			wake_up_state(task, TASK_INTERRUPTIBLE);
+		} 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..47c335f7c04c 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_signals(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index 800a18f77732..9009b8e77bc3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -40,6 +40,7 @@
 #include <linux/cn_proc.h>
 #include <linux/compiler.h>
 #include <linux/posix-timers.h>
+#include <linux/livepatch.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>
@@ -163,7 +164,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.15.0

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

[Jiri Kosina]

On Wed, 15 Nov 2017, Miroslav Benes wrote:

> 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 differently.
> 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 architectures' "entry" 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. In connection with this, also
> call klp_update_patch_state() before do_signal(), so that
> recalc_sigpending() in dequeue_signal() can clear TIF_PATCH_PENDING
> immediately and thus prevent a double call of do_signal().
> 
> 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 signal sysfs attribute in livepatch
> sysfs directory.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: x86@kernel.org
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch | 12 +++++++
>  Documentation/livepatch/livepatch.txt            | 11 +++++--
>  arch/powerpc/kernel/signal.c                     |  6 ++--
>  arch/x86/entry/common.c                          |  6 ++--
>  kernel/livepatch/core.c                          | 30 +++++++++++++++++
>  kernel/livepatch/transition.c                    | 41 ++++++++++++++++++++++++
>  kernel/livepatch/transition.h                    |  1 +
>  kernel/signal.c                                  |  4 ++-

I'd like to be queuing this patchset for the next merge window, so if 
there are any objections for the out-of-kernel/livepatch/* changes, please 
speak up now.

Thanks.

-- 
Jiri Kosina
SUSE Labs

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

[Michael Ellerman]

Jiri Kosina <jikos@kernel.org> writes:
> On Wed, 15 Nov 2017, Miroslav Benes wrote:
>> 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.
...
>> 
>> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: x86@kernel.org
>> ---
>>  Documentation/ABI/testing/sysfs-kernel-livepatch | 12 +++++++
>>  Documentation/livepatch/livepatch.txt            | 11 +++++--
>>  arch/powerpc/kernel/signal.c                     |  6 ++--
>>  arch/x86/entry/common.c                          |  6 ++--
>>  kernel/livepatch/core.c                          | 30 +++++++++++++++++
>>  kernel/livepatch/transition.c                    | 41 ++++++++++++++++++++++++
>>  kernel/livepatch/transition.h                    |  1 +
>>  kernel/signal.c                                  |  4 ++-
>
> I'd like to be queuing this patchset for the next merge window, so if 
> there are any objections for the out-of-kernel/livepatch/* changes, please 
> speak up now.

The powerpc changes look innocuous and in-line with the change log, no
objection from me. Have an ack if you like:

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

[PATCH v4 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.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
Pavel, does this wording work for you?

 Documentation/ABI/testing/sysfs-kernel-livepatch | 13 ++++++++++
 Documentation/livepatch/livepatch.txt            | 15 ++++++++++--
 kernel/livepatch/core.c                          | 30 ++++++++++++++++++++++++
 kernel/livepatch/transition.c                    | 25 ++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 5 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 3bb9d5bc1ce3..567a8ebb795d 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -45,6 +45,19 @@ Contact:	live-patching@vger.kernel.org
 		signal pending structures). Tasks are interrupted or woken up,
 		and forced to change their patched state.
 
+What:		/sys/kernel/livepatch/<patch>/force
+Date:		Nov 2017
+KernelVersion:	4.15.0
+Contact:	live-patching@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. See Documentation/livepatch/livepatch.txt
+		for more information.
+
 What:		/sys/kernel/livepatch/<patch>/<object>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 9bcdef277a36..57d59274101d 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -183,6 +183,17 @@ 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/<patch>/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/<pid>/stack may help here).
+
 3.1 Adding consistency model support to new architectures
 ---------------------------------------------------------
 
@@ -439,8 +450,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/<patch>/signal attribute allows administrator to affect a
-patching operation.
+/sys/kernel/livepatch/<patch>/signal and /sys/kernel/livepatch/<patch>/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/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/signal
+ * /sys/kernel/livepatch/<patch>/force
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
@@ -542,13 +543,42 @@ static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct klp_patch *patch;
+	int ret;
+	bool val;
+
+	patch = container_of(kobj, struct klp_patch, kobj);
+
+	/*
+	 * 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 (patch != klp_transition_patch)
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		klp_force_transition();
+
+	return count;
+}
+
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
 static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
 static struct kobj_attribute signal_kobj_attr = __ATTR_WO(signal);
+static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
 	&signal_kobj_attr.attr,
+	&force_kobj_attr.attr,
 	NULL
 };
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 095aa8d864f5..566ab210853f 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -618,3 +618,28 @@ void klp_send_signals(void)
 	}
 	read_unlock(&tasklist_lock);
 }
+
+/*
+ * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
+ * existing transition to finish.
+ *
+ * NOTE: klp_update_patch_state(task) requires the task to be inactive or
+ * 'current'. This is not the case here and the consistency model could be
+ * broken. Administrator, who is the only one to execute the
+ * klp_force_transitions(), has to be aware of this.
+ */
+void klp_force_transition(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	pr_warn("forcing remaining tasks to the patched state\n");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_patch_state(task);
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu)
+		klp_update_patch_state(idle_task(cpu));
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 47c335f7c04c..837e51ee30fd 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_signals(void);
+void klp_force_transition(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.15.0

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

[Miroslav Benes]

On Wed, 15 Nov 2017, 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.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

While working on "immediate" removal, I realized we had the similar 
problem here with modules removal. There is no way out of the rabbit hole.

If a patch is forced, we obviously cannot say there is no task sleeping in 
the old code. This could be disastrous if such old module is then removed 
(either we disabled it and we want to rmmod it, or there is a new "atomic 
replace" patch and we want to remove the old one).

We need something like the following (at least as a starting point)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 566ab210853f..df4f2bbd9731 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -33,6 +33,8 @@ struct klp_patch *klp_transition_patch;
 
 static int klp_target_state = KLP_UNDEFINED;
 
+static bool klp_forced = false;
+
 /*
  * This work can be performed periodically to finish patching or unpatching any
  * "straggler" tasks which failed to transition in the first attempt.
@@ -109,7 +111,7 @@ static void klp_complete_transition(void)
                }
        }
 
-       if (klp_target_state == KLP_UNPATCHED && !immediate_func)
+       if (klp_target_state == KLP_UNPATCHED && !klp_forced && !immediate_func)
                module_put(klp_transition_patch->mod);
 
        /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
@@ -642,4 +644,6 @@ void klp_force_transition(void)
 
        for_each_possible_cpu(cpu)
                klp_update_patch_state(idle_task(cpu));
+
+       klp_forced = true;
 }


It is still better than immediate, because it is a "ex post" action. We 
can also try to improve later. We could remember all forced tasks and 
reenable rmmod once those tasks are really migrated ("shadow migration"). 

Thoughts are really welcome here.

Miroslav

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

[Josh Poimboeuf]

On Mon, Nov 20, 2017 at 04:57:19PM +0100, Miroslav Benes wrote:
> On Wed, 15 Nov 2017, 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.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> While working on "immediate" removal, I realized we had the similar 
> problem here with modules removal. There is no way out of the rabbit hole.
> 
> If a patch is forced, we obviously cannot say there is no task sleeping in 
> the old code. This could be disastrous if such old module is then removed 
> (either we disabled it and we want to rmmod it, or there is a new "atomic 
> replace" patch and we want to remove the old one).
> 
> We need something like the following (at least as a starting point)
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 566ab210853f..df4f2bbd9731 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -33,6 +33,8 @@ struct klp_patch *klp_transition_patch;
>  
>  static int klp_target_state = KLP_UNDEFINED;
>  
> +static bool klp_forced = false;
> +
>  /*
>   * This work can be performed periodically to finish patching or unpatching any
>   * "straggler" tasks which failed to transition in the first attempt.
> @@ -109,7 +111,7 @@ static void klp_complete_transition(void)
>                 }
>         }
>  
> -       if (klp_target_state == KLP_UNPATCHED && !immediate_func)
> +       if (klp_target_state == KLP_UNPATCHED && !klp_forced && !immediate_func)
>                 module_put(klp_transition_patch->mod);
>  
>         /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> @@ -642,4 +644,6 @@ void klp_force_transition(void)
>  
>         for_each_possible_cpu(cpu)
>                 klp_update_patch_state(idle_task(cpu));
> +
> +       klp_forced = true;
>  }
> 
> 
> It is still better than immediate, because it is a "ex post" action.

Looks good to me.

> We can also try to improve later. We could remember all forced tasks
> and reenable rmmod once those tasks are really migrated ("shadow
> migration"). 

NACK :-)  Forcing should hopefully be a rare event, not worth the
trouble to try to keep track of that IMO.

-- 
Josh

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

[Jiri Kosina]

On Mon, 20 Nov 2017, Miroslav Benes wrote:

> While working on "immediate" removal, I realized we had the similar 
> problem here with modules removal. There is no way out of the rabbit hole.
> 
> If a patch is forced, we obviously cannot say there is no task sleeping in 
> the old code. This could be disastrous if such old module is then removed 
> (either we disabled it and we want to rmmod it, or there is a new "atomic 
> replace" patch and we want to remove the old one).
> 
> We need something like the following (at least as a starting point)

I agree; the only thing I think really has to be done is putting a comment 
there, explaining why forcing implies infinite module reference (and also 
perhaps making it therefore even more obvious from documentation, that 
this really is a last-resort-"you-know-what-you-are-doing" kind of knob).


On Mon, 20 Nov 2017, Josh Poimboeuf wrote:

> > We can also try to improve later. We could remember all forced tasks
> > and reenable rmmod once those tasks are really migrated ("shadow
> > migration"). 
> 
> NACK :-)  Forcing should hopefully be a rare event, not worth the
> trouble to try to keep track of that IMO.

Well, that was my rather random idea when we were discussing this over 
lunch today. But I agree, it definitely is a total overkill, and I don't 
want it to be atributed to me any more :p

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Tue, 21 Nov 2017, Jiri Kosina wrote:

> I agree; the only thing I think really has to be done is putting a comment 
> there, explaining why forcing implies infinite module reference (and also 
> perhaps making it therefore even more obvious from documentation, that 
> this really is a last-resort-"you-know-what-you-are-doing" kind of knob).

Miroslav, could you please just send v2 of 2/2, so that I can apply it?

Josh, given your latest comment I assume that your Ack holds even for v2 
with the proposed ammendment (perhaps with an extra comment), unless you 
tell me otherwise.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Josh Poimboeuf]

On Tue, Nov 21, 2017 at 12:11:14AM +0100, Jiri Kosina wrote:
> On Tue, 21 Nov 2017, Jiri Kosina wrote:
> 
> > I agree; the only thing I think really has to be done is putting a comment 
> > there, explaining why forcing implies infinite module reference (and also 
> > perhaps making it therefore even more obvious from documentation, that 
> > this really is a last-resort-"you-know-what-you-are-doing" kind of knob).
> 
> Miroslav, could you please just send v2 of 2/2, so that I can apply it?
> 
> Josh, given your latest comment I assume that your Ack holds even for v2 
> with the proposed ammendment (perhaps with an extra comment), unless you 
> tell me otherwise.

Yeah, that's fine.

-- 
Josh

[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 <mbenes@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 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-patching@vger.kernel.org
 		signal pending structures). Tasks are interrupted or woken up,
 		and forced to change their patched state.
 
+What:		/sys/kernel/livepatch/<patch>/force
+Date:		Nov 2017
+KernelVersion:	4.15.0
+Contact:	live-patching@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/<patch>/<object>
 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/<patch>/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/<pid>/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/<patch>/signal attribute allows administrator to affect a
-patching operation.
+/sys/kernel/livepatch/<patch>/signal and /sys/kernel/livepatch/<patch>/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/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/signal
+ * /sys/kernel/livepatch/<patch>/force
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
@@ -542,13 +543,42 @@ static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct klp_patch *patch;
+	int ret;
+	bool val;
+
+	patch = container_of(kobj, struct klp_patch, kobj);
+
+	/*
+	 * 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 (patch != klp_transition_patch)
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		klp_force_transition();
+
+	return count;
+}
+
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
 static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
 static struct kobj_attribute signal_kobj_attr = __ATTR_WO(signal);
+static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
 	&signal_kobj_attr.attr,
+	&force_kobj_attr.attr,
 	NULL
 };
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 095aa8d864f5..abdb15e24b20 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -33,6 +33,8 @@ struct klp_patch *klp_transition_patch;
 
 static int klp_target_state = KLP_UNDEFINED;
 
+static bool klp_forced = false;
+
 /*
  * This work can be performed periodically to finish patching or unpatching any
  * "straggler" tasks which failed to transition in the first attempt.
@@ -109,7 +111,11 @@ static void klp_complete_transition(void)
 		}
 	}
 
-	if (klp_target_state == KLP_UNPATCHED && !immediate_func)
+	/*
+	 * klp_forced or immediate_func set implies unbounded increase of
+	 * module's ref count if the module is disabled/enabled in a loop.
+	 */
+	if (klp_target_state == KLP_UNPATCHED && !klp_forced && !immediate_func)
 		module_put(klp_transition_patch->mod);
 
 	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
@@ -618,3 +624,30 @@ void klp_send_signals(void)
 	}
 	read_unlock(&tasklist_lock);
 }
+
+/*
+ * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
+ * existing transition to finish.
+ *
+ * NOTE: klp_update_patch_state(task) requires the task to be inactive or
+ * 'current'. This is not the case here and the consistency model could be
+ * broken. Administrator, who is the only one to execute the
+ * klp_force_transitions(), has to be aware of this.
+ */
+void klp_force_transition(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	pr_warn("forcing remaining tasks to the patched state\n");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_patch_state(task);
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu)
+		klp_update_patch_state(idle_task(cpu));
+
+	klp_forced = true;
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 47c335f7c04c..837e51ee30fd 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_signals(void);
+void klp_force_transition(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.15.0

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 <mbenes@suse.cz>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
>  Documentation/livepatch/livepatch.txt            | 18 ++++++++++--
>  kernel/livepatch/core.c                          | 30 ++++++++++++++++++++
>  kernel/livepatch/transition.c                    | 35 +++++++++++++++++++++++-
>  kernel/livepatch/transition.h                    |  1 +
>  5 files changed, 95 insertions(+), 3 deletions(-)

....

> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + *
> + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> + * 'current'. This is not the case here and the consistency model could be
> + * broken. Administrator, who is the only one to execute the
> + * klp_force_transitions(), has to be aware of this.
> + */
> +void klp_force_transition(void)
> +{
> +	struct task_struct *g, *task;
> +	unsigned int cpu;
> +
> +	pr_warn("forcing remaining tasks to the patched state\n");
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task)
> +		klp_update_patch_state(task);
> +	read_unlock(&tasklist_lock);
> +
> +	for_each_possible_cpu(cpu)
> +		klp_update_patch_state(idle_task(cpu));
> +
> +	klp_forced = true;
> +}

I had a question on this bit. If say cpu 0 executes
klp_force_transition(void), right up until klp_forced is set to true,
and then cpu 1 does klp_complete_transition() (since all threads have
the correct state), wouldn't it be possible then for
klp_complete_transition() to not see klp_forced set to true, and thus
the module could be potentially removed even though it was forced?

If so, I think that the force path just needs to be set before the
threads are updated (as below). I don't think that the
klp_complete_transition() needs the corresponding rmb, b/c there is
sufficient ordering there already (although it would deserve a comment).

Thanks,

-Jason

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index be5bfa5..cca6a3a 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -671,6 +671,15 @@ void klp_force_transition(void)

        pr_warn("forcing remaining tasks to the patched state\n");

+       klp_forced = true;
+
+       /*
+        * ensure that if klp_complete_transition() sees that all
+        * the threads have been updated to desired task->patch_state
+        * that we also see klp_forced = true;
+        */
+       smp_wmb();
+
        read_lock(&tasklist_lock);
        for_each_process_thread(g, task)
                klp_update_patch_state(task);
@@ -678,6 +687,4 @@ void klp_force_transition(void)

        for_each_possible_cpu(cpu)
                klp_update_patch_state(idle_task(cpu));
-
-       klp_forced = true;
 }

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 <mbenes@suse.cz>
> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> >  Documentation/livepatch/livepatch.txt            | 18 ++++++++++--
> >  kernel/livepatch/core.c                          | 30 ++++++++++++++++++++
> >  kernel/livepatch/transition.c                    | 35 +++++++++++++++++++++++-
> >  kernel/livepatch/transition.h                    |  1 +
> >  5 files changed, 95 insertions(+), 3 deletions(-)
> 
> ....
> 
> > +
> > +/*
> > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > + * existing transition to finish.
> > + *
> > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > + * 'current'. This is not the case here and the consistency model could be
> > + * broken. Administrator, who is the only one to execute the
> > + * klp_force_transitions(), has to be aware of this.
> > + */
> > +void klp_force_transition(void)
> > +{
> > +	struct task_struct *g, *task;
> > +	unsigned int cpu;
> > +
> > +	pr_warn("forcing remaining tasks to the patched state\n");
> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process_thread(g, task)
> > +		klp_update_patch_state(task);
> > +	read_unlock(&tasklist_lock);
> > +
> > +	for_each_possible_cpu(cpu)
> > +		klp_update_patch_state(idle_task(cpu));
> > +
> > +	klp_forced = true;
> > +}
> 
> I had a question on this bit. If say cpu 0 executes
> klp_force_transition(void), right up until klp_forced is set to true,
> and then cpu 1 does klp_complete_transition() (since all threads have
> the correct state), wouldn't it be possible then for
> klp_complete_transition() to not see klp_forced set to true, and thus
> the module could be potentially removed even though it was forced?

Yes, you're right. That could happen.
 
> If so, I think that the force path just needs to be set before the
> threads are updated (as below). I don't think that the
> klp_complete_transition() needs the corresponding rmb, b/c there is
> sufficient ordering there already (although it would deserve a comment).

Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be 
done with it once and for all. The problem is exactly what Petr predicted 
and I refused to have klp_mutex here just because it may have fixed 
theoretical issue. 

Petr, Josh, what do you think?

Miroslav

> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index be5bfa5..cca6a3a 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -671,6 +671,15 @@ void klp_force_transition(void)
> 
>         pr_warn("forcing remaining tasks to the patched state\n");
> 
> +       klp_forced = true;
> +
> +       /*
> +        * ensure that if klp_complete_transition() sees that all
> +        * the threads have been updated to desired task->patch_state
> +        * that we also see klp_forced = true;
> +        */
> +       smp_wmb();
> +
>         read_lock(&tasklist_lock);
>         for_each_process_thread(g, task)
>                 klp_update_patch_state(task);
> @@ -678,6 +687,4 @@ void klp_force_transition(void)
> 
>         for_each_possible_cpu(cpu)
>                 klp_update_patch_state(idle_task(cpu));
> -
> -       klp_forced = true;
>  }
> 

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 <mbenes@suse.cz>
> > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> > >  Documentation/livepatch/livepatch.txt            | 18 ++++++++++--
> > >  kernel/livepatch/core.c                          | 30 ++++++++++++++++++++
> > >  kernel/livepatch/transition.c                    | 35 +++++++++++++++++++++++-
> > >  kernel/livepatch/transition.h                    |  1 +
> > >  5 files changed, 95 insertions(+), 3 deletions(-)
> > 
> > ....
> > 
> > > +
> > > +/*
> > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > > + * existing transition to finish.
> > > + *
> > > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > > + * 'current'. This is not the case here and the consistency model could be
> > > + * broken. Administrator, who is the only one to execute the
> > > + * klp_force_transitions(), has to be aware of this.
> > > + */
> > > +void klp_force_transition(void)
> > > +{
> > > +	struct task_struct *g, *task;
> > > +	unsigned int cpu;
> > > +
> > > +	pr_warn("forcing remaining tasks to the patched state\n");
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +	for_each_process_thread(g, task)
> > > +		klp_update_patch_state(task);
> > > +	read_unlock(&tasklist_lock);
> > > +
> > > +	for_each_possible_cpu(cpu)
> > > +		klp_update_patch_state(idle_task(cpu));
> > > +
> > > +	klp_forced = true;
> > > +}
> > 
> > I had a question on this bit. If say cpu 0 executes
> > klp_force_transition(void), right up until klp_forced is set to true,
> > and then cpu 1 does klp_complete_transition() (since all threads have
> > the correct state), wouldn't it be possible then for
> > klp_complete_transition() to not see klp_forced set to true, and thus
> > the module could be potentially removed even though it was forced?
> 
> Yes, you're right. That could happen.
>  
> > If so, I think that the force path just needs to be set before the
> > threads are updated (as below). I don't think that the
> > klp_complete_transition() needs the corresponding rmb, b/c there is
> > sufficient ordering there already (although it would deserve a comment).
> 
> Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be 
> done with it once and for all. The problem is exactly what Petr predicted 
> and I refused to have klp_mutex here just because it may have fixed 
> theoretical issue. 
> 
> Petr, Josh, what do you think?

Sounds good to me.

We should have known to listen to Petr in the first place :-)

-- 
Josh

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 <mbenes@suse.cz>
> > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-kernel-livepatch | 14 ++++++++++
> > >  Documentation/livepatch/livepatch.txt            | 18 ++++++++++--
> > >  kernel/livepatch/core.c                          | 30 ++++++++++++++++++++
> > >  kernel/livepatch/transition.c                    | 35 +++++++++++++++++++++++-
> > >  kernel/livepatch/transition.h                    |  1 +
> > >  5 files changed, 95 insertions(+), 3 deletions(-)
> > 
> > ....
> > 
> > > +
> > > +/*
> > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > > + * existing transition to finish.
> > > + *
> > > + * NOTE: klp_update_patch_state(task) requires the task to be inactive or
> > > + * 'current'. This is not the case here and the consistency model could be
> > > + * broken. Administrator, who is the only one to execute the
> > > + * klp_force_transitions(), has to be aware of this.
> > > + */
> > > +void klp_force_transition(void)
> > > +{
> > > +	struct task_struct *g, *task;
> > > +	unsigned int cpu;
> > > +
> > > +	pr_warn("forcing remaining tasks to the patched state\n");
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +	for_each_process_thread(g, task)
> > > +		klp_update_patch_state(task);
> > > +	read_unlock(&tasklist_lock);
> > > +
> > > +	for_each_possible_cpu(cpu)
> > > +		klp_update_patch_state(idle_task(cpu));
> > > +
> > > +	klp_forced = true;
> > > +}
> > 
> > I had a question on this bit. If say cpu 0 executes
> > klp_force_transition(void), right up until klp_forced is set to true,
> > and then cpu 1 does klp_complete_transition() (since all threads have
> > the correct state), wouldn't it be possible then for
> > klp_complete_transition() to not see klp_forced set to true, and thus
> > the module could be potentially removed even though it was forced?
> 
> Yes, you're right. That could happen.
>  
> > If so, I think that the force path just needs to be set before the
> > threads are updated (as below). I don't think that the
> > klp_complete_transition() needs the corresponding rmb, b/c there is
> > sufficient ordering there already (although it would deserve a comment).
> 
> Or we can take klp_mutex in force_store() (kernel/livepatch/core.c) and be 
> done with it once and for all. The problem is exactly what Petr predicted 
> and I refused to have klp_mutex here just because it may have fixed 
> theoretical issue. 
> 
> Petr, Josh, what do you think?

Sounds good to me. I would feel better if the lock is there.

Best Regards,
Petr

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Hi!

> > We can also try to improve later. We could remember all forced tasks
> > and reenable rmmod once those tasks are really migrated ("shadow
> > migration"). 
> 
> NACK :-)  Forcing should hopefully be a rare event, not worth the
> trouble to try to keep track of that IMO.

Just disable rmmod in case of forced removal. Noone should be doing
rmmod on important machines. And noone should be forcing patches.

Ouch and make sure to taint the kernel.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jiri Kosina]

On Tue, 21 Nov 2017, Pavel Machek wrote:

> Just disable rmmod in case of forced removal.

Yeah, well, that's basically what the patch does :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Petr Mladek]

On Mon 2017-11-20 16:57:19, Miroslav Benes wrote:
> On Wed, 15 Nov 2017, 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.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> While working on "immediate" removal, I realized we had the similar 
> problem here with modules removal. There is no way out of the rabbit hole.
> 
> If a patch is forced, we obviously cannot say there is no task sleeping in 
> the old code. This could be disastrous if such old module is then removed 
> (either we disabled it and we want to rmmod it, or there is a new "atomic 
> replace" patch and we want to remove the old one).

> We need something like the following (at least as a starting point)

Great catch! The proposed solution looks good to me. And as already
mentioned, it might be worth updating the documentation.

For the whole patchset, including this fixup:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v4 0/2] livepatch: Introduce signal and force sysfs attributes

[Josh Poimboeuf]

On Wed, Nov 15, 2017 at 02:50:12PM +0100, Miroslav Benes wrote:
> 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 uninterruptedly 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.
> 
> Petr, I did not add you Reviewed-by tags intentionally because of the changes.
> 
> Changes from v3:
> - only TASK_INTERRUPTIBLE kthreads are woken up - Petr
> - documentation - Pavel
> - function naming and sysfs fix - Josh

For the series:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v4 0/2] livepatch: Introduce signal and force sysfs attributes

[Jiri Kosina]

On Wed, 15 Nov 2017, Miroslav Benes wrote:

> 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 uninterruptedly 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.
> 
> Petr, I did not add you Reviewed-by tags intentionally because of the changes.
> 
> Changes from v3:
> - only TASK_INTERRUPTIBLE kthreads are woken up - Petr
> - documentation - Pavel
> - function naming and sysfs fix - Josh
> 
> Changes from v2:
> - two sysfs attributes instead of one - Petr, Josh
> - better documentation about force usage - Pavel
> - small changes here and there
> 
> Changes from v1:
> - better wording, typos, comments, documentation - Libor, Josh
> - symbolic names in sysfs instead of numbers - Libor
> - exit_to_usermode_loop(), call klp_update_patch_state() before do_signal() - Oleg
> - better names - Josh
> - mutex and WARN_ON_ONCE not added to klp_force_transitions() - Petr, Josh
> - handle idle tasks in klp_force_transitions() too - Josh
> 
> Miroslav Benes (2):
>   livepatch: send a fake signal to all blocking tasks
>   livepatch: force transition to finish
> 
>  Documentation/ABI/testing/sysfs-kernel-livepatch | 25 +++++++++
>  Documentation/livepatch/livepatch.txt            | 22 +++++++-
>  arch/powerpc/kernel/signal.c                     |  6 +--
>  arch/x86/entry/common.c                          |  6 +--
>  kernel/livepatch/core.c                          | 60 +++++++++++++++++++++
>  kernel/livepatch/transition.c                    | 66 ++++++++++++++++++++++++
>  kernel/livepatch/transition.h                    |  2 +
>  kernel/signal.c                                  |  4 +-

This is now queued in livepatching.git#for-4.16/signal-sysfs-force. 
Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 0/2] livepatch: Introduce signal and force sysfs attributes

[Jiri Kosina]

[ CC list trimmed ]

On Mon, 4 Dec 2017, Jiri Kosina wrote:

> This is now queued in livepatching.git#for-4.16/signal-sysfs-force. 

The branch above has been obsoleted by for-4.16/signal-sysfs-force-v2, 
which contains the proper (v4.1) version of the patch 2/2.

-- 
Jiri Kosina
SUSE Labs