[PATCH 0/2] Periodic fake signal

[PATCH 0/2] Periodic fake signal

[Miroslav Benes]

This is what we have in SLES and I wondered if upstream would be
interested as well.

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                 | 89 ++++++++++---------
 kernel/livepatch/transition.h                 |  1 -
 5 files changed, 55 insertions(+), 94 deletions(-)

-- 
2.17.0

[PATCH 1/2] livepatch: Send a fake signal periodically

[Miroslav Benes]

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 10 seconds. The timeout is chosen deliberately. It
gives the tasks enough time to transition themselves.

Theoretically, sending it once should be more than enough. Better be safe
than sorry, so send it periodically. 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 <mbenes@suse.cz>
---
 Documentation/livepatch/livepatch.txt |  3 ++-
 kernel/livepatch/transition.c         | 11 +++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 1ae2de758c08..011897e2392f 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 10 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 7c6631e693bc..c33d29c74ac6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -29,6 +29,8 @@
 #define MAX_STACK_ENTRIES  100
 #define STACK_ERR_BUF_SIZE 128
 
+#define SIGNALS_TIMEOUT 10
+
 struct klp_patch *klp_transition_patch;
 
 static int klp_target_state = KLP_UNDEFINED;
@@ -367,6 +369,7 @@ void klp_try_complete_transition(void)
 	unsigned int cpu;
 	struct task_struct *g, *task;
 	bool complete = true;
+	static unsigned int signals_cnt = 0;
 
 	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
 
@@ -403,6 +406,9 @@ void klp_try_complete_transition(void)
 	put_online_cpus();
 
 	if (!complete) {
+		if (!(++signals_cnt % SIGNALS_TIMEOUT))
+			klp_send_signals();
+
 		/*
 		 * Some tasks weren't able to be switched over.  Try again
 		 * later and/or wait for other methods like kernel exit
@@ -410,10 +416,12 @@ void klp_try_complete_transition(void)
 		 */
 		schedule_delayed_work(&klp_transition_work,
 				      round_jiffies_relative(HZ));
+
 		return;
 	}
 
 	/* we're done, now cleanup the data structures */
+	signals_cnt = 0;
 	klp_complete_transition();
 }
 
@@ -577,8 +585,7 @@ 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)
 {
-- 
2.17.0

Re: [PATCH 1/2] livepatch: Send a fake signal periodically

[Josh Poimboeuf]

On Mon, Jun 04, 2018 at 04:16:35PM +0200, 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 10 seconds. The timeout is chosen deliberately. It
> gives the tasks enough time to transition themselves.
> 
> Theoretically, sending it once should be more than enough. Better be safe
> than sorry, so send it periodically.

This is the part I don't understand.  Why do it periodically?

Instead, might it make sense to just send the signals once, and if that
doesn't work, reverse the transition?  Then we could make patching a
synchronous operation.  But then, it might be remotely possible that the
reverse operation also stalls (e.g., on a kthread).  So, maybe it's best
to just leave all these controls in the hands of the user.

All that said, a few code review comments:

- AFAICT, it does an 8 second delay instead of a 10 second delay,
  because
  
  a) try_complete_transition() is first called before there's any delay;

  b) the preincrement operator used on signals_cnt.

- I think 15 seconds might be a better default.  I've seen longer
  patching delays on a system with 100+ CPUs.

- If a kthread or idle task is sleeping on a patched function, the
  pr_notice("signaling remaining tasks\n") will be repeated continously.

- It might be cleaner to do it from the delayed work function
  (klp_transition_work_fn).

-- 
Josh

Re: [PATCH 1/2] livepatch: Send a fake signal periodically

[Miroslav Benes]

On Mon, 4 Jun 2018, Josh Poimboeuf wrote:

> On Mon, Jun 04, 2018 at 04:16:35PM +0200, 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 10 seconds. The timeout is chosen deliberately. It
> > gives the tasks enough time to transition themselves.
> > 
> > Theoretically, sending it once should be more than enough. Better be safe
> > than sorry, so send it periodically.
> 
> This is the part I don't understand.  Why do it periodically?

I met (rare!) cases when doing it once was not enough due to a race and 
the signal was missed. However involved testcases were really artificial.
 
> Instead, might it make sense to just send the signals once, and if that
> doesn't work, reverse the transition?  Then we could make patching a
> synchronous operation.  But then, it might be remotely possible that the
> reverse operation also stalls (e.g., on a kthread).  So, maybe it's best
> to just leave all these controls in the hands of the user.

And there is 'force' option...

So given all this, I'd call klp_send_signals() once and then leave it up 
to the user. Would that work for you?

> All that said, a few code review comments:
> 
> - AFAICT, it does an 8 second delay instead of a 10 second delay,
>   because

Not that it matters, because it is still wrong, but it is a 9 second 
delay (or I miscounted again).

>   a) try_complete_transition() is first called before there's any delay;
> 
>   b) the preincrement operator used on signals_cnt.
> 
> - I think 15 seconds might be a better default.  I've seen longer
>   patching delays on a system with 100+ CPUs.

Ok, why not. 

> - If a kthread or idle task is sleeping on a patched function, the
>   pr_notice("signaling remaining tasks\n") will be repeated continously.

True.

> - It might be cleaner to do it from the delayed work function
>   (klp_transition_work_fn).

I considered it but then decided to do it in klp_try_complete_transition() 
under 'if (!complete)'. It belongs right there in my opinion.

Thanks,
Miroslav

Re: [PATCH 1/2] livepatch: Send a fake signal periodically

[Josh Poimboeuf]

On Tue, Jun 05, 2018 at 09:17:52AM +0200, Miroslav Benes wrote:
> On Mon, 4 Jun 2018, Josh Poimboeuf wrote:
> 
> > On Mon, Jun 04, 2018 at 04:16:35PM +0200, 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 10 seconds. The timeout is chosen deliberately. It
> > > gives the tasks enough time to transition themselves.
> > > 
> > > Theoretically, sending it once should be more than enough. Better be safe
> > > than sorry, so send it periodically.
> > 
> > This is the part I don't understand.  Why do it periodically?
> 
> I met (rare!) cases when doing it once was not enough due to a race and 
> the signal was missed. However involved testcases were really artificial.
>  
> > Instead, might it make sense to just send the signals once, and if that
> > doesn't work, reverse the transition?  Then we could make patching a
> > synchronous operation.  But then, it might be remotely possible that the
> > reverse operation also stalls (e.g., on a kthread).  So, maybe it's best
> > to just leave all these controls in the hands of the user.
> 
> And there is 'force' option...
> 
> So given all this, I'd call klp_send_signals() once and then leave it up 
> to the user. Would that work for you?

Well, I don't know.  Since the patching process will already need to be
managed by user space, what's the benefit of having the kernel doing
only this part of it?

-- 
Josh

Re: [PATCH 1/2] livepatch: Send a fake signal periodically

[Miroslav Benes]

On Tue, 5 Jun 2018, Josh Poimboeuf wrote:

> On Tue, Jun 05, 2018 at 09:17:52AM +0200, Miroslav Benes wrote:
> > On Mon, 4 Jun 2018, Josh Poimboeuf wrote:
> > 
> > > On Mon, Jun 04, 2018 at 04:16:35PM +0200, 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 10 seconds. The timeout is chosen deliberately. It
> > > > gives the tasks enough time to transition themselves.
> > > > 
> > > > Theoretically, sending it once should be more than enough. Better be safe
> > > > than sorry, so send it periodically.
> > > 
> > > This is the part I don't understand.  Why do it periodically?
> > 
> > I met (rare!) cases when doing it once was not enough due to a race and 
> > the signal was missed. However involved testcases were really artificial.
> >  
> > > Instead, might it make sense to just send the signals once, and if that
> > > doesn't work, reverse the transition?  Then we could make patching a
> > > synchronous operation.  But then, it might be remotely possible that the
> > > reverse operation also stalls (e.g., on a kthread).  So, maybe it's best
> > > to just leave all these controls in the hands of the user.
> > 
> > And there is 'force' option...
> > 
> > So given all this, I'd call klp_send_signals() once and then leave it up 
> > to the user. Would that work for you?
> 
> Well, I don't know.  Since the patching process will already need to be
> managed by user space, what's the benefit of having the kernel doing
> only this part of it?

I'm not sure about 'will already need to be managed by user space' part. 
Sending the fake signal would unblock transition in certain cases "for 
free". No user interaction is really needed and we can do it 
automatically. That's why I find it beneficial.

If it does not help, then the user must decide what to do next. Reversion 
or force. That's up to them.

Miroslav

Re: [PATCH 1/2] livepatch: Send a fake signal periodically

[Petr Mladek]

On Wed 2018-06-06 09:49:50, Miroslav Benes wrote:
> On Tue, 5 Jun 2018, Josh Poimboeuf wrote:
> 
> > On Tue, Jun 05, 2018 at 09:17:52AM +0200, Miroslav Benes wrote:
> > > On Mon, 4 Jun 2018, Josh Poimboeuf wrote:
> > > 
> > > > On Mon, Jun 04, 2018 at 04:16:35PM +0200, 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 10 seconds. The timeout is chosen deliberately. It
> > > > > gives the tasks enough time to transition themselves.
> > > > > 
> > > > > Theoretically, sending it once should be more than enough. Better be safe
> > > > > than sorry, so send it periodically.
> > > > 
> > > > This is the part I don't understand.  Why do it periodically?
> > > 
> > > I met (rare!) cases when doing it once was not enough due to a race and 
> > > the signal was missed. However involved testcases were really artificial.
> > >  
> > > > Instead, might it make sense to just send the signals once, and if that
> > > > doesn't work, reverse the transition?  Then we could make patching a
> > > > synchronous operation.  But then, it might be remotely possible that the
> > > > reverse operation also stalls (e.g., on a kthread).  So, maybe it's best
> > > > to just leave all these controls in the hands of the user.
> > > 
> > > And there is 'force' option...
> > > 
> > > So given all this, I'd call klp_send_signals() once and then leave it up 
> > > to the user. Would that work for you?
> > 
> > Well, I don't know.  Since the patching process will already need to be
> > managed by user space, what's the benefit of having the kernel doing
> > only this part of it?
> 
> I'm not sure about 'will already need to be managed by user space' part. 
> Sending the fake signal would unblock transition in certain cases "for 
> free". No user interaction is really needed and we can do it 
> automatically. That's why I find it beneficial.

I wonder if several kicks might be necessary to get a kthread patched.

BTW: This approach has a bit different effect in compare with kGraft.
In kGraft, it was enough to force kthread going over an explicitly
defined safe point where it migrated itself. In upstream, the kthread
must get scheduled outside any patched function. The chance might
be a bit random.

To make it clear. I believe that sending signal once makes sense for
sure. And it would make sense to even repeat it.

BTW: If we allow to repeat it, we should reset the counter in
klp_reverse_transition().

Best Regards,
Petr

[PATCH 2/2] livepatch: Remove signal sysfs attribute

[Miroslav Benes]

The fake signal is send automatically now. We can rely on it completely
and remove the sysfs attribute.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 .../ABI/testing/sysfs-kernel-livepatch        | 12 ---
 Documentation/livepatch/livepatch.txt         | 16 ++--
 kernel/livepatch/core.c                       | 32 --------
 kernel/livepatch/transition.c                 | 80 +++++++++----------
 kernel/livepatch/transition.h                 |  1 -
 5 files changed, 47 insertions(+), 94 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:	live-patching@vger.kernel.org
 		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:	live-patching@vger.kernel.org
-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 011897e2392f..b738d70ab002 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 10 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 10 seconds.
 
 Administrator can also affect a transition through
 /sys/kernel/livepatch/<patch>/force attribute. Writing 1 there clears
@@ -419,8 +417,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 3a4656fb7047..064dee4caf4f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -449,7 +449,6 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /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>
@@ -525,35 +524,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)
 {
@@ -585,12 +555,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 c33d29c74ac6..988dbc270aea 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -356,6 +356,46 @@ 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;
+
+	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
@@ -583,46 +623,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;
-
-	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.17.0