Messages in this thread Patch in this message | | | Subject | Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD | From | "dongkai (H)" <> | Date | Fri, 26 Mar 2021 14:31:24 +0800 |
| |
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@huawei.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. >
| |