| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 26 Mar 2021 14:31:24 +0800
From: "dongkai (H)" <dongkai11@...wei.com>
To: Miroslav Benes <mbenes@...e.cz>
CC: <jpoimboe@...hat.com>, <jikos@...nel.org>, <pmladek@...e.com>,
<joe.lawrence@...hat.com>, <axboe@...nel.dk>,
<live-patching@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like
PF_KTHREAD
On 2021/3/25 17:26, Miroslav Benes wrote:
> On Thu, 25 Mar 2021, Dong Kai wrote:
>
>> commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
>> PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
>> by making the freezer not send them fake signals.
>>
>> Here live patching consistency model call klp_send_signals to wake up
>> all tasks by send fake signal to all non-kthread which only check the
>> PF_KTHREAD flag, so it still send signal to io threads which may lead to
>> freezeing issue of io threads.
>
> I suppose this could happen, but it will also affect the live patching
> transition if the io threads do not react to signals.
>
> Are you able to reproduce it easily? I mean, is there a testcase I could
> use to take a closer look?
>
Um... I tried but failed to reproduce this on real environment as i'm
not familiar with the io uring usage.
So i use a tricky way to verify this possibility by the following patch
which create a fake io thread in module and patch the func which is
always within thread running stack. Then the stack check will failed
when transition and trigger the klp_send_signal flow.
This example may not suitable, but you can get my point
Kai
Note: this patch export some symbols just for test via module because if
i create io thread via sysinit, it will receive SIGKILL signal[set by
zap_other_threads] when run init process and exit the loop, weird...
diff --git a/fs/exec.c b/fs/exec.c
index 18594f11c31f..a64af6cac43b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1229,6 +1229,7 @@ void __set_task_comm(struct task_struct *tsk,
const char *buf, bool exec)
task_unlock(tsk);
perf_event_comm(tsk, exec);
}
+EXPORT_SYMBOL_GPL(__set_task_comm);
/*
* Calling this is the point of no return. None of the failures will be
diff --git a/kernel/fork.c b/kernel/fork.c
index 54cc905e5fe0..03064fef7bb1 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2447,6 +2447,7 @@ struct task_struct *create_io_thread(int
(*fn)(void *), void *arg, int node)
}
return tsk;
}
+EXPORT_SYMBOL(create_io_thread);
/*
* Ok, this is the main fork-routine.
index 98191218d891..8151d17149a0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3856,6 +3856,7 @@ void wake_up_new_task(struct task_struct *p)
#endif
task_rq_unlock(rq, p, &rf);
}
+EXPORT_SYMBOL_GPL(wake_up_new_task);
#ifdef CONFIG_PREEMPT_NOTIFIERS
diff --git a/samples/test/Makefile b/samples/test/Makefile
new file mode 100644
index 000000000000..efbf01c6477e
--- /dev/null
+++ b/samples/test/Makefile
@@ -0,0 +1 @@
+obj-m += io_thread.o livepatch-sample.o
diff --git a/samples/test/io_thread.c b/samples/test/io_thread.c
new file mode 100644
index 000000000000..e7bdc51a4582
--- /dev/null
+++ b/samples/test/io_thread.c
@@ -0,0 +1,49 @@
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/sched/task.h>
+#include <linux/sched/signal.h>
+
+static __used noinline void func(void)
+{
+ printk("func\n");
+ schedule_timeout(HZ * 5);
+}
+
+static int io_worker(void *data)
+{
+ set_task_comm(current, "io_worker");
+ while (1) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ func();
+
+ if (fatal_signal_pending(current))
+ break;
+ }
+
+ return 0;
+}
+
+static int __init io_thread_init(void)
+{
+ struct task_struct *task = NULL;
+
+ task = create_io_thread(io_worker, NULL, 0);
+ if (task == NULL)
+ return -EINVAL;
+ wake_up_new_task(task);
+
+ /* when insmod exit, io thread got SIGKILL and exit, so... */
+ while (1)
+ schedule_timeout(HZ);
+ return 0;
+}
+
+static void __exit io_thread_exit(void)
+{
+ return;
+}
+
+module_init(io_thread_init);
+module_exit(io_thread_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/samples/test/livepatch-sample.c
b/samples/test/livepatch-sample.c
new file mode 100644
index 000000000000..c35b494f5c5a
--- /dev/null
+++ b/samples/test/livepatch-sample.c
@@ -0,0 +1,43 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+static void new_func(void)
+{
+ schedule_timeout(HZ * 5);
+ printk("new_func\n");
+}
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "func",
+ .new_func = new_func,
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ .name = "io_thread",
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int livepatch_init(void)
+{
+ return klp_enable_patch(&patch);
+}
+
+static void livepatch_exit(void)
+{
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_INFO(livepatch, "Y");
+
+MODULE_LICENSE("GPL");
--
>> Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
>> within klp_send_signal function.
>
> Yes, this sounds reasonable.
>
> Miroslav
>
>> Signed-off-by: Dong Kai <dongkai11@...wei.com>
>> ---
>> note:
>> the io threads freeze issue links:
>> [1] https://lore.kernel.org/io-uring/YEgnIp43%2F6kFn8GL@kevinlocke.name/
>> [2] https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb539b@kernel.dk/
>>
>> kernel/livepatch/transition.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
>> index f6310f848f34..0e1c35c8f4b4 100644
>> --- a/kernel/livepatch/transition.c
>> +++ b/kernel/livepatch/transition.c
>> @@ -358,7 +358,7 @@ static void klp_send_signals(void)
>> * Meanwhile the task could migrate itself and the action
>> * would be meaningless. It is not serious though.
>> */
>> - if (task->flags & PF_KTHREAD) {
>> + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) {
>> /*
>> * Wake up a kthread which sleeps interruptedly and
>> * still has not been migrated.
>
Powered by blists - more mailing lists