I somehow forgot about this patch set. The discussion about v1 was
inconclusive, so I'm sending it again with changes according to the
feedback I got. I still think it is useful, so please consider.
Changes since v1:
- timeout changed to 15 seconds (Josh)
- make it wait for 15 seconds, no more, no less (Josh)
- the message in klp_send_signals() is printed only once per transition
(Josh)
- the counter is zeroed in klp_start_transition(), so that even
klp_reverse_transition() starts correctly (Petr)
Miroslav Benes (2):
livepatch: Send a fake signal periodically
livepatch: Remove signal sysfs attribute
.../ABI/testing/sysfs-kernel-livepatch | 12 ---
Documentation/livepatch/livepatch.txt | 15 ++-
kernel/livepatch/core.c | 32 -------
kernel/livepatch/transition.c | 93 +++++++++++--------
kernel/livepatch/transition.h | 1 -
5 files changed, 59 insertions(+), 94 deletions(-)
--
2.20.1
An administrator may send a fake signal to all remaining blocking tasks
of a running transition by writing to
/sys/kernel/livepatch/<patch>/signal attribute. Let's do it
automatically after 15 seconds. The timeout is chosen deliberately. It
gives the tasks enough time to transition themselves.
Theoretically, sending it once should be more than enough. However,
every task must get outside of a patched function to be successfully
transitioned. It could prove not to be simple and resending could be
helpful in that case.
A new workqueue job could be a cleaner solution to achieve it, but it
could also introduce deadlocks and cause more headaches with
synchronization and cancelling.
Signed-off-by: Miroslav Benes <[email protected]>
---
Documentation/livepatch/livepatch.txt | 3 ++-
kernel/livepatch/transition.c | 17 ++++++++++++++---
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 71d7f286ec4d..407e0f03dc99 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -163,7 +163,8 @@ patched state. This may be harmful to the system though.
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.
+their patched state. Despite the sysfs attribute the fake signal is also sent
+every 15 seconds automatically.
Administrator can also affect a transition through
/sys/kernel/livepatch/<patch>/force attribute. Writing 1 there clears
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 300273819674..12a1a65531a0 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -29,10 +29,14 @@
#define MAX_STACK_ENTRIES 100
#define STACK_ERR_BUF_SIZE 128
+#define SIGNALS_TIMEOUT 15
+
struct klp_patch *klp_transition_patch;
static int klp_target_state = KLP_UNDEFINED;
+static unsigned int klp_signals_cnt;
+
/*
* This work can be performed periodically to finish patching or unpatching any
* "straggler" tasks which failed to transition in the first attempt.
@@ -393,6 +397,10 @@ void klp_try_complete_transition(void)
put_online_cpus();
if (!complete) {
+ if (klp_signals_cnt && !(klp_signals_cnt % SIGNALS_TIMEOUT))
+ klp_send_signals();
+ klp_signals_cnt++;
+
/*
* Some tasks weren't able to be switched over. Try again
* later and/or wait for other methods like kernel exit
@@ -400,6 +408,7 @@ void klp_try_complete_transition(void)
*/
schedule_delayed_work(&klp_transition_work,
round_jiffies_relative(HZ));
+
return;
}
@@ -454,6 +463,8 @@ void klp_start_transition(void)
if (task->patch_state != klp_target_state)
set_tsk_thread_flag(task, TIF_PATCH_PENDING);
}
+
+ klp_signals_cnt = 0;
}
/*
@@ -578,14 +589,14 @@ void klp_copy_process(struct task_struct *child)
/*
* 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.
+ * Kthreads with TIF_PATCH_PENDING set are woken up.
*/
void klp_send_signals(void)
{
struct task_struct *g, *task;
- pr_notice("signaling remaining tasks\n");
+ if (klp_signals_cnt == SIGNALS_TIMEOUT)
+ pr_notice("signaling remaining tasks\n");
read_lock(&tasklist_lock);
for_each_process_thread(g, task) {
--
2.20.1
The fake signal is send automatically now. We can rely on it completely
and remove the sysfs attribute.
Signed-off-by: Miroslav Benes <[email protected]>
---
.../ABI/testing/sysfs-kernel-livepatch | 12 ---
Documentation/livepatch/livepatch.txt | 16 ++--
kernel/livepatch/core.c | 32 --------
kernel/livepatch/transition.c | 82 +++++++++----------
kernel/livepatch/transition.h | 1 -
5 files changed, 48 insertions(+), 95 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index dac7e1e62a8b..85db352f68f9 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,18 +33,6 @@ Contact: [email protected]
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: [email protected]
-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>/force
Date: Nov 2017
KernelVersion: 4.15.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 407e0f03dc99..4627b41ff02e 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -158,13 +158,11 @@ 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. 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. Despite the sysfs attribute the fake signal is also sent
-every 15 seconds automatically.
+patched state. This may be harmful to the system though. Sending a fake signal
+to all remaining blocking tasks is a better alternative. 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. The fake
+signal is automatically sent every 15 seconds.
Administrator can also affect a transition through
/sys/kernel/livepatch/<patch>/force attribute. Writing 1 there clears
@@ -412,8 +410,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 and /sys/kernel/livepatch/<patch>/force
-attributes allow administrator to affect a patching operation.
+/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 adca5cf07f7e..fe1993399823 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -313,7 +313,6 @@ static int klp_write_object_relocations(struct module *pmod,
* /sys/kernel/livepatch/<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>
@@ -382,35 +381,6 @@ 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;
-
- ret = kstrtobool(buf, &val);
- if (ret)
- return ret;
-
- if (!val)
- return count;
-
- mutex_lock(&klp_mutex);
-
- patch = container_of(kobj, struct klp_patch, kobj);
- if (patch != klp_transition_patch) {
- mutex_unlock(&klp_mutex);
- return -EINVAL;
- }
-
- klp_send_signals();
-
- mutex_unlock(&klp_mutex);
-
- return count;
-}
-
static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
const char *buf, size_t count)
{
@@ -442,12 +412,10 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
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 12a1a65531a0..79876744d8da 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -347,6 +347,47 @@ static bool klp_try_switch_task(struct task_struct *task)
}
+/*
+ * Sends a fake signal to all non-kthread tasks with TIF_PATCH_PENDING set.
+ * Kthreads with TIF_PATCH_PENDING set are woken up.
+ */
+static void klp_send_signals(void)
+{
+ struct task_struct *g, *task;
+
+ if (klp_signals_cnt == SIGNALS_TIMEOUT)
+ 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);
+}
+
/*
* Try to switch all remaining tasks to the target patch state by walking the
* stacks of sleeping tasks and looking for any to-be-patched or
@@ -587,47 +628,6 @@ 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.
- */
-void klp_send_signals(void)
-{
- struct task_struct *g, *task;
-
- if (klp_signals_cnt == SIGNALS_TIMEOUT)
- 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);
-}
-
/*
* Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
* existing transition to finish.
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index f9d0bc016067..322db16233de 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -11,7 +11,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);
void klp_force_transition(void);
#endif /* _LIVEPATCH_TRANSITION_H */
--
2.20.1
On Tue, 15 Jan 2019, Miroslav Benes wrote:
> An administrator may send a fake signal to all remaining blocking tasks
> of a running transition by writing to
> /sys/kernel/livepatch/<patch>/signal attribute. Let's do it
> automatically after 15 seconds. The timeout is chosen deliberately. It
> gives the tasks enough time to transition themselves.
>
> Theoretically, sending it once should be more than enough. However,
> every task must get outside of a patched function to be successfully
> transitioned. It could prove not to be simple and resending could be
> helpful in that case.
>
> A new workqueue job could be a cleaner solution to achieve it, but it
> could also introduce deadlocks and cause more headaches with
> synchronization and cancelling.
>
> Signed-off-by: Miroslav Benes <[email protected]>
[ ... snip ... ]
> @@ -400,6 +408,7 @@ void klp_try_complete_transition(void)
> */
> schedule_delayed_work(&klp_transition_work,
> round_jiffies_relative(HZ));
> +
> return;
> }
I somehow didn't like the newly added newline here :) so I've removed it,
and applied both patches to for-5.1/fake-signal.
Thanks,
--
Jiri Kosina
SUSE Labs
On Tue, Jan 15, 2019 at 05:45:05PM +0100, Miroslav Benes wrote:
> I somehow forgot about this patch set. The discussion about v1 was
> inconclusive, so I'm sending it again with changes according to the
> feedback I got. I still think it is useful, so please consider.
>
> Changes since v1:
> - timeout changed to 15 seconds (Josh)
> - make it wait for 15 seconds, no more, no less (Josh)
> - the message in klp_send_signals() is printed only once per transition
> (Josh)
> - the counter is zeroed in klp_start_transition(), so that even
> klp_reverse_transition() starts correctly (Petr)
>
> Miroslav Benes (2):
> livepatch: Send a fake signal periodically
> livepatch: Remove signal sysfs attribute
>
> .../ABI/testing/sysfs-kernel-livepatch | 12 ---
> Documentation/livepatch/livepatch.txt | 15 ++-
> kernel/livepatch/core.c | 32 -------
> kernel/livepatch/transition.c | 93 +++++++++++--------
> kernel/livepatch/transition.h | 1 -
> 5 files changed, 59 insertions(+), 94 deletions(-)
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Tue 2019-01-15 17:45:05, Miroslav Benes wrote:
> I somehow forgot about this patch set. The discussion about v1 was
> inconclusive, so I'm sending it again with changes according to the
> feedback I got. I still think it is useful, so please consider.
>
> Changes since v1:
> - timeout changed to 15 seconds (Josh)
> - make it wait for 15 seconds, no more, no less (Josh)
> - the message in klp_send_signals() is printed only once per transition
> (Josh)
> - the counter is zeroed in klp_start_transition(), so that even
> klp_reverse_transition() starts correctly (Petr)
>
> Miroslav Benes (2):
> livepatch: Send a fake signal periodically
> livepatch: Remove signal sysfs attribute
Both patches look fine to me:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr