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

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

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

TODO:
Now there is a sysfs attribute called "force", which provides two
functionalities, "signal" and "force" (previously "unmark"). I haven't
managed to come up with better names. Proposals are welcome. On the
other hand I do not mind it much.

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 | 18 +++++++
 Documentation/livepatch/livepatch.txt            | 14 +++++
 arch/powerpc/kernel/signal.c                     |  6 +--
 arch/x86/entry/common.c                          |  6 +--
 kernel/livepatch/core.c                          | 50 ++++++++++++++++++
 kernel/livepatch/transition.c                    | 65 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  2 +
 kernel/signal.c                                  |  4 +-
 8 files changed, 158 insertions(+), 7 deletions(-)

-- 
2.13.3

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

[Miroslav Benes]

Add read-write 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 it only to situations when klp_transition_patch variable is set.
Normally, klp_mutex lock should be acquired, 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 | 12 +++++++
 Documentation/livepatch/livepatch.txt            |  7 ++++
 kernel/livepatch/core.c                          | 43 ++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index d5d39748382f..b7a487ca8852 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -8,6 +8,18 @@ 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:		Jul 2017
+KernelVersion:	4.14.0
+Contact:	live-patching@vger.kernel.org
+Description:
+		The attribute allows administrator to affect the course of an
+		existing transition.
+
+		Reading from the file returns all available operations.
+
+		Writing one of the strings to the file executes the operation.
+
 What:		/sys/kernel/livepatch/<patch>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index ecdb18104ab0..9c9966be328d 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -178,6 +178,10 @@ 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.
 
+Administrator can also affect a transition through /sys/kernel/livepatch/force
+attribute. Reading from the file returns all available operations. Writing one
+of the strings to the file executes the operation.
+
 
 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/force attribute allows 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 b9628e43c78f..79022b7eca2c 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,41 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 
+/*
+ * NOTE: The following function are currently empty. The intention is to keep
+ * sysfs glue separate and thus make the review easier.
+ */
+static ssize_t force_show(struct kobject *kobj,
+			  struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "No operation is currently permitted.\n");
+}
+
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	/*
+	 * 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, forced action ineffective\n");
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static struct kobj_attribute force_kobj_attr = __ATTR_RW(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 +990,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.13.3

[PATCH v2 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 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 force 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 |  4 ++-
 Documentation/livepatch/livepatch.txt            |  5 ++-
 arch/powerpc/kernel/signal.c                     |  6 ++--
 arch/x86/entry/common.c                          |  6 ++--
 kernel/livepatch/core.c                          |  9 ++++--
 kernel/livepatch/transition.c                    | 40 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 kernel/signal.c                                  |  4 ++-
 8 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index b7a487ca8852..45f4e3551d27 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -16,9 +16,11 @@ Contact:	live-patching@vger.kernel.org
 		The attribute allows administrator to affect the course of an
 		existing transition.
 
-		Reading from the file returns all available operations.
+		Reading from the file returns all available operations, which
+		may be "signal" (signalling remaining tasks).
 
 		Writing one of the strings to the file executes the operation.
+		"signal" sends a signal to all remaining blocking tasks.
 
 What:		/sys/kernel/livepatch/<patch>
 Date:		Nov 2014
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 9c9966be328d..343b0bfa1b9f 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -180,7 +180,10 @@ patched state.
 
 Administrator can also affect a transition through /sys/kernel/livepatch/force
 attribute. Reading from the file returns all available operations. Writing one
-of the strings to the file executes the operation.
+of the strings to the file executes the operation. "signal" is available for
+signalling all remaining blocking tasks. This is an alternative for
+SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
+less harmful to the system.
 
 
 3.1 Adding consistency model support to new architectures
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 cdefcfdd9e63..8e638dcd0822 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -152,6 +152,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);
@@ -164,9 +167,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 79022b7eca2c..a359340c924d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -452,7 +452,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
 static ssize_t force_show(struct kobject *kobj,
 			  struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "No operation is currently permitted.\n");
+	return sprintf(buf, "signal\n");
 }
 
 static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 		return -EINVAL;
 	}
 
-	return -EINVAL;
+	if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
+		klp_force_signals();
+	else
+		return -EINVAL;
+
+	return count;
 }
 
 static struct kobj_attribute force_kobj_attr = __ATTR_RW(force);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1fb6032..0d8be69be8b1 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -577,3 +577,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_force_signals(void)
+{
+	struct task_struct *g, *task;
+
+	pr_notice("signalling 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 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..6c480057539a 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_force_signals(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index ca92bcfeb322..8a961dd943f4 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.13.3

[PATCH v2 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 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>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  6 +++++-
 Documentation/livepatch/livepatch.txt            |  6 +++++-
 kernel/livepatch/core.c                          |  4 +++-
 kernel/livepatch/transition.c                    | 25 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 5 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 45f4e3551d27..c1604a45bf84 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -17,11 +17,15 @@ Contact:	live-patching@vger.kernel.org
 		existing transition.
 
 		Reading from the file returns all available operations, which
-		may be "signal" (signalling remaining tasks).
+		may be "signal" (signalling remaining tasks) and "force" (force
+		transition process to finish).
 
 		Writing one of the strings to the file executes the operation.
 		"signal" sends a signal to all remaining blocking tasks.
 
+		"force" clears TIF_PATCH_PENDING flag of all tasks and thus
+		forces the tasks to the patched state.
+
 What:		/sys/kernel/livepatch/<patch>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 343b0bfa1b9f..7626d1b947c2 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -183,7 +183,11 @@ attribute. Reading from the file returns all available operations. Writing one
 of the strings to the file executes the operation. "signal" is available for
 signalling all remaining blocking tasks. This is an alternative for
 SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
-less harmful to the system.
+less harmful to the system. "force" clears TIF_PATCH_PENDING flag of all tasks
+and thus forces the tasks to the patched state. Important note! Use "force"
+wisely. Administrator 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.
 
 
 3.1 Adding consistency model support to new architectures
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a359340c924d..e759af6e4393 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -452,7 +452,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
 static ssize_t force_show(struct kobject *kobj,
 			  struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "signal\n");
+	return sprintf(buf, "signal force\n");
 }
 
 static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -470,6 +470,8 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
 		klp_force_signals();
+	else if (!memcmp("force", buf, min(sizeof("force")-1, count)))
+		klp_force_transitions();
 	else
 		return -EINVAL;
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 0d8be69be8b1..d69ec3be7b8e 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -617,3 +617,28 @@ void klp_force_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_transitions(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 6c480057539a..da3be48d36bb 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_force_signals(void);
+void klp_force_transitions(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.13.3

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

[Josh Poimboeuf]

On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> Now there is a sysfs attribute called "force", which provides two
> functionalities, "signal" and "force" (previously "unmark"). I haven't
> managed to come up with better names. Proposals are welcome. On the
> other hand I do not mind it much.

Now "force" has two meanings, which is a little confusing.  What do you
think about just having two separate write-only sysfs flags?

  echo 1 > /sys/kernel/livepatch/signal
  echo 1 > /sys/kernel/livepatch/force

That way there's no ambiguity.

-- 
Josh

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

[Josh Poimboeuf]

On Thu, Aug 10, 2017 at 12:48:13PM +0200, Miroslav Benes wrote:
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	/*
> +	 * 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, forced action ineffective\n");
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}

I think this is really a minor issue, and the -EINVAL is enough.  As the
comment says, the race window is harmless.  So I'd say there's no need
to scare the user with a printk.

-- 
Josh

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

[Josh Poimboeuf]

On Thu, Aug 10, 2017 at 12:48:14PM +0200, Miroslav Benes wrote:
> 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.

'writing 1' -> 'writing "signal"'

(unless you take my suggestion to change to two separate sysfs files)

> @@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		return -EINVAL;
>  	}
>  
> -	return -EINVAL;
> +	if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
> +		klp_force_signals();

Any reason why you can't just do a strcmp()?

> +++ b/kernel/livepatch/transition.c
> @@ -577,3 +577,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_force_signals(void)
> +{
> +	struct task_struct *g, *task;
> +
> +	pr_notice("signalling remaining tasks\n");

As a native US speaker with possible OCD spelling tendencies, it bothers
me to see "signalling" with two l's instead of one.  According to
Google, the UK spelling of the word has two l's, so maybe it's not a
typo.  I'll forgive you if you don't fix it :-)

> +
> +	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);

I can't remember if we talked about this before, is it possible to also
signal/wake the idle tasks?

-- 
Josh

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

[Jiri Kosina]

On Fri, 11 Aug 2017, Josh Poimboeuf wrote:

> > +	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);
> 
> I can't remember if we talked about this before, is it possible to also
> signal/wake the idle tasks?

Scheduler won't select idle task in case there is *anything* else runnable 
in any other sched class anyway. And if that is the case, there is no need 
for explicit wakeup, as idle task would get scheduled anyway implicitly.

So idle task is a little bit more difficult than that, unfortunately.

-- 
Jiri Kosina
SUSE Labs

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

[Miroslav Benes]

On Fri, 11 Aug 2017, Josh Poimboeuf wrote:

> On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> > Now there is a sysfs attribute called "force", which provides two
> > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > managed to come up with better names. Proposals are welcome. On the
> > other hand I do not mind it much.
> 
> Now "force" has two meanings, which is a little confusing.  What do you
> think about just having two separate write-only sysfs flags?
> 
>   echo 1 > /sys/kernel/livepatch/signal
>   echo 1 > /sys/kernel/livepatch/force
> 
> That way there's no ambiguity.

Sure, why not. Moreover it would simplify things (no need for dummy 1/3 
patch and its code).

Thanks,
Miroslav

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

[Miroslav Benes]

On Fri, 11 Aug 2017, Josh Poimboeuf wrote:

> On Thu, Aug 10, 2017 at 12:48:13PM +0200, Miroslav Benes wrote:
> > +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	/*
> > +	 * 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, forced action ineffective\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> 
> I think this is really a minor issue, and the -EINVAL is enough.  As the
> comment says, the race window is harmless.  So I'd say there's no need
> to scare the user with a printk.

Ok, I'll remove it in v3.

Miroslav

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

[Miroslav Benes]

On Fri, 11 Aug 2017, Josh Poimboeuf wrote:

> On Thu, Aug 10, 2017 at 12:48:14PM +0200, Miroslav Benes wrote:
> > 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.
> 
> 'writing 1' -> 'writing "signal"'
> 
> (unless you take my suggestion to change to two separate sysfs files)

I'll take two separate sysfs files instead.
 
> > @@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  		return -EINVAL;
> >  	}
> >  
> > -	return -EINVAL;
> > +	if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
> > +		klp_force_signals();
> 
> Any reason why you can't just do a strcmp()?

Not really IIRC. I just borrowed the code from 
mm/huge_memory.c:enabled_store().

> > +++ b/kernel/livepatch/transition.c
> > @@ -577,3 +577,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_force_signals(void)
> > +{
> > +	struct task_struct *g, *task;
> > +
> > +	pr_notice("signalling remaining tasks\n");
> 
> As a native US speaker with possible OCD spelling tendencies, it bothers
> me to see "signalling" with two l's instead of one.  According to
> Google, the UK spelling of the word has two l's, so maybe it's not a
> typo.  I'll forgive you if you don't fix it :-)

If it bothers you, I'll fix it. As a non-native speaker, I can live with 
both.

> > +
> > +	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);
> 
> I can't remember if we talked about this before, is it possible to also
> signal/wake the idle tasks?

Jiri mentioned that in his email. It is not that easy. Take a look at 
pick_next_task() in kernel/sched/core.c. idle_sched_class is always the 
last one to be checked. Of course we could do something like this 
there...

if (klp_patch_pending(rq->idle)) {
	p = idle_sched_class.pick_next_task(rq, prev);
	
	return p;
}

... but people may be watching, so I didn't say anything.

Thanks,
Miroslav

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

[Petr Mladek]

On Thu 2017-08-10 12:48:13, Miroslav Benes wrote:
> Add read-write 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 it only to situations when klp_transition_patch variable is set.
> Normally, klp_mutex lock should be acquired, 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).
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..79022b7eca2c 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -954,6 +990,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);

We need to set klp_root_kobj = NULL here. Or we need to set the global
klp_root_kobj only when the attributes are created. Otherwise,
klp_initialized() would return true and registering a patch would
push the system out of a safe road.

Note that this actually opens a small race window when the livepatching
core pretends to be initialized even when the initialization still
might fail. It is rather theoretical but it would be nice to avoid
it if it can be done an easy way, e.g. by setting klp_root_kobj later.

Best Regards,
Petr

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

[Petr Mladek]

On Thu 2017-08-10 12:48:12, 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.
> 
> TODO:
> Now there is a sysfs attribute called "force", which provides two
> functionalities, "signal" and "force" (previously "unmark"). I haven't
> managed to come up with better names. Proposals are welcome. On the
> other hand I do not mind it much.

What about calling the attribute?

     transition-speedup
     transition-urge

In each case, I would make it more clear that the attribute
is related to the transition attribute of each patch.

Best Regards,
Petr

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

[Petr Mladek]

On Thu 2017-08-10 12:48:14, 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.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 79022b7eca2c..a359340c924d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -452,7 +452,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>  static ssize_t force_show(struct kobject *kobj,
>  			  struct kobj_attribute *attr, char *buf)
>  {
> -	return sprintf(buf, "No operation is currently permitted.\n");
> +	return sprintf(buf, "signal\n");

This makes invalid the "NOTE:" above this function ;-)

Best Regards,
Petr

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

[Petr Mladek]

On Fri 2017-08-11 16:11:31, Josh Poimboeuf wrote:
> On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> > Now there is a sysfs attribute called "force", which provides two
> > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > managed to come up with better names. Proposals are welcome. On the
> > other hand I do not mind it much.
> 
> Now "force" has two meanings, which is a little confusing.  What do you
> think about just having two separate write-only sysfs flags?
> 
>   echo 1 > /sys/kernel/livepatch/signal
>   echo 1 > /sys/kernel/livepatch/force

I like the simplicity but I wonder if there might be more actions
that need to be forced in the future. Then this might cause
confusion.

For example, we have force_module_load attribute in kGraft.
It allows to load a module even when it is refused by a livepatch.
It is handy when there is a harmless bug in the patch.

Best Regards,
Petr

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

[Josh Poimboeuf]

On Wed, Aug 16, 2017 at 04:50:07PM +0200, Petr Mladek wrote:
> On Fri 2017-08-11 16:11:31, Josh Poimboeuf wrote:
> > On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> > > Now there is a sysfs attribute called "force", which provides two
> > > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > > managed to come up with better names. Proposals are welcome. On the
> > > other hand I do not mind it much.
> > 
> > Now "force" has two meanings, which is a little confusing.  What do you
> > think about just having two separate write-only sysfs flags?
> > 
> >   echo 1 > /sys/kernel/livepatch/signal
> >   echo 1 > /sys/kernel/livepatch/force
> 
> I like the simplicity but I wonder if there might be more actions
> that need to be forced in the future. Then this might cause
> confusion.
> 
> For example, we have force_module_load attribute in kGraft.
> It allows to load a module even when it is refused by a livepatch.
> It is handy when there is a harmless bug in the patch.

What if we put the flags in the per-patch dir?

  /sys/kernel/livepatch/<patch>/signal
  /sys/kernel/livepatch/<patch>/force

That seems pretty unambiguous.  The "force" is specific to the patch, it
clearly means we are forcing the patch.

-- 
Josh

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

[Miroslav Benes]

On Wed, 16 Aug 2017, Petr Mladek wrote:

> On Thu 2017-08-10 12:48:13, Miroslav Benes wrote:
> > Add read-write 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 it only to situations when klp_transition_patch variable is set.
> > Normally, klp_mutex lock should be acquired, 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).
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..79022b7eca2c 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -954,6 +990,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);
> 
> We need to set klp_root_kobj = NULL here. Or we need to set the global
> klp_root_kobj only when the attributes are created. Otherwise,
> klp_initialized() would return true and registering a patch would
> push the system out of a safe road.

Oh, right!
 
> Note that this actually opens a small race window when the livepatching
> core pretends to be initialized even when the initialization still
> might fail. It is rather theoretical but it would be nice to avoid
> it if it can be done an easy way, e.g. by setting klp_root_kobj later.

Hm, klp_initialized() uses klp_root_kobj because it was a simple way of 
detection. Maybe it is time to introduce proper klp_initialized 
global variable. I'd like it more than setting klp_root_kobj later just 
for the sake of correctness.

What do you think?

Miroslav

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

[Pavel Machek]

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

On Thu 2017-08-10 12:48:15, 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 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.

Yes, that's what admins are good for. Magically determining what state
their machine is in, and deciding if all the processes are in the sane
state and what the consequences are. They have all the tools they need
to do that, like JTAG connection to the CPU and about 10 years of
time... do they?

This should taint the kernel at the very least.

It should also require capabilities beyond "normal root", because it
allows malicious admin to do "bad things (tm)" to the kernel.

								Pavel

> diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
> index 343b0bfa1b9f..7626d1b947c2 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -183,7 +183,11 @@ attribute. Reading from the file returns all available operations. Writing one
>  of the strings to the file executes the operation. "signal" is available for
>  signalling all remaining blocking tasks. This is an alternative for
>  SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
> -less harmful to the system.
> +less harmful to the system. "force" clears TIF_PATCH_PENDING flag of all tasks
> +and thus forces the tasks to the patched state. Important note! Use "force"
> +wisely. Administrator 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.

You may want to elaborate here a lot. If you want to pretend
administrator can decide this, you need to describe what the model is,
and how administrator get enough information about the system state.

-- 
(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 v2 3/3] livepatch: force transition process to finish

[Miroslav Benes]

On Wed, 30 Aug 2017, Pavel Machek wrote:

> On Thu 2017-08-10 12:48:15, 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 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.
> 
> Yes, that's what admins are good for. Magically determining what state
> their machine is in, and deciding if all the processes are in the sane
> state and what the consequences are. They have all the tools they need
> to do that, like JTAG connection to the CPU and about 10 years of
> time... do they?

It is not that bad in this case fortunately. The decision depends on

1. which processes are not yet migrated and why (which functions they're 
sleeping on)

AND

2. what functions are patched and what is the nature of a fix.

Admin knows 1. and he needs input from a livepatch author about 2. There 
is no magic there, but cooperation between those two is definitely 
required.
 
> This should taint the kernel at the very least.

That's what we discussed in v1. In cases where the patch does not need the 
consistency model at all, the taint would be too much. But I have no 
problem to transform pr_warn to WARN_ON_ONCE.

> It should also require capabilities beyond "normal root", because it
> allows malicious admin to do "bad things (tm)" to the kernel.

I think that malicious admin can do pretty bad (and more serious) things 
even without this. That is not an excuse, but it is general problem not 
specific to this patch.

> > diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
> > index 343b0bfa1b9f..7626d1b947c2 100644
> > --- a/Documentation/livepatch/livepatch.txt
> > +++ b/Documentation/livepatch/livepatch.txt
> > @@ -183,7 +183,11 @@ attribute. Reading from the file returns all available operations. Writing one
> >  of the strings to the file executes the operation. "signal" is available for
> >  signalling all remaining blocking tasks. This is an alternative for
> >  SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
> > -less harmful to the system.
> > +less harmful to the system. "force" clears TIF_PATCH_PENDING flag of all tasks
> > +and thus forces the tasks to the patched state. Important note! Use "force"
> > +wisely. Administrator 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.
> 
> You may want to elaborate here a lot. If you want to pretend
> administrator can decide this, you need to describe what the model is,
> and how administrator get enough information about the system state.

Ok, np. I can add something along the lines I drafted above.

Thanks,
Miroslav

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

[Miroslav Benes]

On Wed, 16 Aug 2017, Josh Poimboeuf wrote:

> On Wed, Aug 16, 2017 at 04:50:07PM +0200, Petr Mladek wrote:
> > On Fri 2017-08-11 16:11:31, Josh Poimboeuf wrote:
> > > On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> > > > Now there is a sysfs attribute called "force", which provides two
> > > > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > > > managed to come up with better names. Proposals are welcome. On the
> > > > other hand I do not mind it much.
> > > 
> > > Now "force" has two meanings, which is a little confusing.  What do you
> > > think about just having two separate write-only sysfs flags?
> > > 
> > >   echo 1 > /sys/kernel/livepatch/signal
> > >   echo 1 > /sys/kernel/livepatch/force
> > 
> > I like the simplicity but I wonder if there might be more actions
> > that need to be forced in the future. Then this might cause
> > confusion.
> > 
> > For example, we have force_module_load attribute in kGraft.
> > It allows to load a module even when it is refused by a livepatch.
> > It is handy when there is a harmless bug in the patch.

We can add force_module_load attribute too in that case. But I see your 
point, I just don't think it would be that serious as far as confusion is 
concerned.

> What if we put the flags in the per-patch dir?
> 
>   /sys/kernel/livepatch/<patch>/signal
>   /sys/kernel/livepatch/<patch>/force
> 
> That seems pretty unambiguous.  The "force" is specific to the patch, it
> clearly means we are forcing the patch.

Petr, would this solve your worries?

Thanks,
Miroslav

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

[Miroslav Benes]

On Wed, 16 Aug 2017, Petr Mladek wrote:

> On Thu 2017-08-10 12:48:12, 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.
> > 
> > TODO:
> > Now there is a sysfs attribute called "force", which provides two
> > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > managed to come up with better names. Proposals are welcome. On the
> > other hand I do not mind it much.
> 
> What about calling the attribute?
> 
>      transition-speedup
>      transition-urge
> 
> In each case, I would make it more clear that the attribute
> is related to the transition attribute of each patch.

Umm... I don't like that much and those names would definitely confuse me. 
But I'm biased already.

Miroslav

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

[Josh Poimboeuf]

On Wed, Aug 30, 2017 at 02:48:24PM +0200, Miroslav Benes wrote:
> On Wed, 30 Aug 2017, Pavel Machek wrote:
> 
> > On Thu 2017-08-10 12:48:15, 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 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.
> > 
> > Yes, that's what admins are good for. Magically determining what state
> > their machine is in, and deciding if all the processes are in the sane
> > state and what the consequences are. They have all the tools they need
> > to do that, like JTAG connection to the CPU and about 10 years of
> > time... do they?
> 
> It is not that bad in this case fortunately. The decision depends on
> 
> 1. which processes are not yet migrated and why (which functions they're 
> sleeping on)
> 
> AND
> 
> 2. what functions are patched and what is the nature of a fix.
> 
> Admin knows 1. and he needs input from a livepatch author about 2. There 
> is no magic there, but cooperation between those two is definitely 
> required.

As we said before, we have to trust the admin knows what they're doing.
If the vendor didn't tell them to force the patch, they shouldn't be
forcing it.  Just like they shouldn't be doing "sudo rm -rf /".

And the last time I checked, there's no taint for logging in as root.

> > This should taint the kernel at the very least.
> 
> That's what we discussed in v1. In cases where the patch does not need the 
> consistency model at all, the taint would be too much. But I have no 
> problem to transform pr_warn to WARN_ON_ONCE.

As I mentioned before, false warnings can have unintended negative
consequences as well.

But anyway, if you do decide to make it a warning, I think the ONCE
wouldn't be helpful.  It should just be WARN_ON.  But I still vote
against it.

> > It should also require capabilities beyond "normal root", because it
> > allows malicious admin to do "bad things (tm)" to the kernel.

What exactly do you consider to be beyond "normal root"???

> I think that malicious admin can do pretty bad (and more serious) things 
> even without this. That is not an excuse, but it is general problem not 
> specific to this patch.

Right.  A malicious admin with root access can do a *lot* worse than
force loading a patch.  A hacker who just obtained root access isn't
going to say "Finally! I can force load that stalled CVE fix!"

-- 
Josh

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

[Petr Mladek]

On Mon 2017-08-28 16:58:40, Miroslav Benes wrote:
> On Wed, 16 Aug 2017, Petr Mladek wrote:
> 
> > On Thu 2017-08-10 12:48:13, Miroslav Benes wrote:
> > > Add read-write 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 it only to situations when klp_transition_patch variable is set.
> > > Normally, klp_mutex lock should be acquired, 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).
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..79022b7eca2c 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -954,6 +990,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);
> > 
> > We need to set klp_root_kobj = NULL here. Or we need to set the global
> > klp_root_kobj only when the attributes are created. Otherwise,
> > klp_initialized() would return true and registering a patch would
> > push the system out of a safe road.
> 
> Oh, right!
>  
> > Note that this actually opens a small race window when the livepatching
> > core pretends to be initialized even when the initialization still
> > might fail. It is rather theoretical but it would be nice to avoid
> > it if it can be done an easy way, e.g. by setting klp_root_kobj later.
> 
> Hm, klp_initialized() uses klp_root_kobj because it was a simple way of 
> detection. Maybe it is time to introduce proper klp_initialized 
> global variable. I'd like it more than setting klp_root_kobj later just 
> for the sake of correctness.
> 
> What do you think?

The global klp_initialized variable sounds fine to me. The current use
of klp_root_kobj is error prone as we see in this patch.

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2017-08-30 14:51:16, Miroslav Benes wrote:
> On Wed, 16 Aug 2017, Josh Poimboeuf wrote:
> 
> > On Wed, Aug 16, 2017 at 04:50:07PM +0200, Petr Mladek wrote:
> > > On Fri 2017-08-11 16:11:31, Josh Poimboeuf wrote:
> > > > On Thu, Aug 10, 2017 at 12:48:12PM +0200, Miroslav Benes wrote:
> > > > > Now there is a sysfs attribute called "force", which provides two
> > > > > functionalities, "signal" and "force" (previously "unmark"). I haven't
> > > > > managed to come up with better names. Proposals are welcome. On the
> > > > > other hand I do not mind it much.
> > > > 
> > > > Now "force" has two meanings, which is a little confusing.  What do you
> > > > think about just having two separate write-only sysfs flags?
> > > > 
> > > >   echo 1 > /sys/kernel/livepatch/signal
> > > >   echo 1 > /sys/kernel/livepatch/force
> > > 
> > > I like the simplicity but I wonder if there might be more actions
> > > that need to be forced in the future. Then this might cause
> > > confusion.
> > > 
> > > For example, we have force_module_load attribute in kGraft.
> > > It allows to load a module even when it is refused by a livepatch.
> > > It is handy when there is a harmless bug in the patch.
> 
> We can add force_module_load attribute too in that case. But I see your 
> point, I just don't think it would be that serious as far as confusion is 
> concerned.
> 
> > What if we put the flags in the per-patch dir?
> > 
> >   /sys/kernel/livepatch/<patch>/signal
> >   /sys/kernel/livepatch/<patch>/force
> > 
> > That seems pretty unambiguous.  The "force" is specific to the patch, it
> > clearly means we are forcing the patch.
> 
> Petr, would this solve your worries?

I like it.

Best Regards,
Petr