[PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Dong Kai]

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.

Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
within klp_send_signal function.

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.
-- 
2.17.1

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Joe Lawrence]

On 3/24/21 9:48 PM, 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

nit: s/freezeing/freezing

> 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.
> 
> Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
> within klp_send_signal function.
> 
> 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.
> 

(PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this 
is a silly question, but...

If the livepatch code could use fake_signal_wake_up(), we could 
consolidate the pattern in klp_send_signals() with the one in 
freeze_task().  Then there would only one place for wake up / fake 
signal logic.

I don't fully understand the differences in the freeze_task() version, 
so I only pose this as a question and not v2 request.

As it is, this change seems logical to me, so:
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

Thanks,

-- Joe

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Miroslav Benes]

> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a
> silly question, but...
> 
> If the livepatch code could use fake_signal_wake_up(), we could consolidate
> the pattern in klp_send_signals() with the one in freeze_task().  Then there
> would only one place for wake up / fake signal logic.
> 
> I don't fully understand the differences in the freeze_task() version, so I
> only pose this as a question and not v2 request.

The plan was to remove our live patching fake signal completely and use 
the new infrastructure Jens proposed in the past.

Something like

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/tracehook.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
                         * 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);
+                       set_notify_signal(task);
                }
        }
        read_unlock(&tasklist_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index a15c584a0455..b7cf4eda8611 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
 
 void recalc_sigpending(void)
 {
-       if (!recalc_sigpending_tsk(current) && !freezing(current) &&
-           !klp_patch_pending(current))
+       if (!recalc_sigpending_tsk(current) && !freezing(current))
                clear_thread_flag(TIF_SIGPENDING);
 
 }


Let me verify it still works and there are all the needed pieces merged 
for all the architectures we support (x86_64, ppc64le and s390x). I'll 
send a proper patch then.

Miroslav

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Jens Axboe]

On 3/25/21 3:30 AM, Miroslav Benes wrote:
>> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a
>> silly question, but...
>>
>> If the livepatch code could use fake_signal_wake_up(), we could consolidate
>> the pattern in klp_send_signals() with the one in freeze_task().  Then there
>> would only one place for wake up / fake signal logic.
>>
>> I don't fully understand the differences in the freeze_task() version, so I
>> only pose this as a question and not v2 request.
> 
> The plan was to remove our live patching fake signal completely and use 
> the new infrastructure Jens proposed in the past.

That would be great, I've actually been waiting for that to show up!
I would greatly prefer this approach if you deem it suitable for 5.12,
if not we'll still need the temporary work-around for live patching.

-- 
Jens Axboe

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Miroslav Benes]

On Thu, 25 Mar 2021, Jens Axboe wrote:

> On 3/25/21 3:30 AM, Miroslav Benes wrote:
> >> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a
> >> silly question, but...
> >>
> >> If the livepatch code could use fake_signal_wake_up(), we could consolidate
> >> the pattern in klp_send_signals() with the one in freeze_task().  Then there
> >> would only one place for wake up / fake signal logic.
> >>
> >> I don't fully understand the differences in the freeze_task() version, so I
> >> only pose this as a question and not v2 request.
> > 
> > The plan was to remove our live patching fake signal completely and use 
> > the new infrastructure Jens proposed in the past.
> 
> That would be great, I've actually been waiting for that to show up!

Sorry about that. I failed to notice that the infrastructure was merged 
already. I'll send it soonish.

> I would greatly prefer this approach if you deem it suitable for 5.12,
> if not we'll still need the temporary work-around for live patching.

I noticed there is 20210326003928.978750-1-axboe@kernel.dk now, so I 
suppose we should wait for that to land in mainline and simply do nothing 
about PF_IO_WORKER for live patching. 

Miroslav

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Miroslav Benes]

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?
 
> 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.

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Joe Lawrence]

On 3/25/21 5:26 AM, 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?
>   

If repro is only hypothetical at this point, perhaps we can artificially 
create it in selftests?  And useful to verify the future change you 
mentioned in your other reply?

-- Joe

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[dongkai (H)]

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

Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

[Jens Axboe]

On 3/24/21 7:48 PM, 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.
> 
> Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
> within klp_send_signal function.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe