[PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD

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

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

(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

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

(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

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

On 3/25/21 3:30 AM, Miroslav Benes wrote:

quoted

(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

On 3/25/21 5:26 AM, Miroslav Benes wrote:

On Thu, 25 Mar 2021, Dong Kai wrote:

quoted

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

On 2021/3/25 17:26, Miroslav Benes wrote:

On Thu, 25 Mar 2021, Dong Kai wrote:

quoted

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

On Thu, 25 Mar 2021, Jens Axboe wrote:

On 3/25/21 3:30 AM, Miroslav Benes wrote:

quoted

quoted

(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