Re: [PATCH v2 2/3] livepatch: send a fake signal to all blocking tasks
On Thu 2017-08-10 12:48:14, Miroslav Benes wrote:
> Live patching consistency model is of LEAVE_PATCHED_SET and
> SWITCH_THREAD. This means that all tasks in the system have to be marked
> one by one as safe to call a new patched function. Safe means when a
> task is not (sleeping) in a set of patched functions. That is, no
> patched function is on the task's stack. Another clearly safe place is
> the boundary between kernel and userspace. The patching waits for all
> tasks to get outside of the patched set or to cross the boundary. The
> transition is completed afterwards.
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 79022b7eca2c..a359340c924d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -452,7 +452,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
> static ssize_t force_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> - return sprintf(buf, "No operation is currently permitted.\n");
> + return sprintf(buf, "signal\n");
This makes invalid the "NOTE:" above this function ;-)
Best Regards,
Petr
Re: [PATCH v2 2/3] livepatch: send a fake signal to all blocking tasks
On Fri, 11 Aug 2017, Josh Poimboeuf wrote:
> On Thu, Aug 10, 2017 at 12:48:14PM +0200, Miroslav Benes wrote:
> > Last, sending the fake signal is not automatic. It is done only when
> > admin requests it by writing 1 to force sysfs attribute in livepatch
> > sysfs directory.
>
> 'writing 1' -> 'writing "signal"'
>
> (unless you take my suggestion to change to two separate sysfs files)
I'll take two separate sysfs files instead.
> > @@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj,
> > struct kobj_attribute *attr,
> > return -EINVAL;
> > }
> >
> > - return -EINVAL;
> > + if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
> > + klp_force_signals();
>
> Any reason why you can't just do a strcmp()?
Not really IIRC. I just borrowed the code from
mm/huge_memory.c:enabled_store().
> > +++ b/kernel/livepatch/transition.c
> > @@ -577,3 +577,43 @@ 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. Only admin can
> > request this
> > + * action currently.
> > + */
> > +void klp_force_signals(void)
> > +{
> > + struct task_struct *g, *task;
> > +
> > + pr_notice("signalling remaining tasks\n");
>
> As a native US speaker with possible OCD spelling tendencies, it bothers
> me to see "signalling" with two l's instead of one. According to
> Google, the UK spelling of the word has two l's, so maybe it's not a
> typo. I'll forgive you if you don't fix it :-)
If it bothers you, I'll fix it. As a non-native speaker, I can live with
both.
> > +
> > + 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 still has not been migrated.
> > +*/
> > + wake_up_process(task);
> > + } 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);
>
> I can't remember if we talked about this before, is it possible to also
> signal/wake the idle tasks?
Jiri mentioned that in his email. It is not that easy. Take a look at
pick_next_task() in kernel/sched/core.c. idle_sched_class is always the
last one to be checked. Of course we could do something like this
there...
if (klp_patch_pending(rq->idle)) {
p = idle_sched_class.pick_next_task(rq, prev);
return p;
}
... but people may be watching, so I didn't say anything.
Thanks,
Miroslav
Re: [PATCH v2 2/3] livepatch: send a fake signal to all blocking tasks
On Fri, 11 Aug 2017, Josh Poimboeuf wrote:
> > + 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 still has not been migrated.
> > +*/
> > + wake_up_process(task);
> > + } 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);
>
> I can't remember if we talked about this before, is it possible to also
> signal/wake the idle tasks?
Scheduler won't select idle task in case there is *anything* else runnable
in any other sched class anyway. And if that is the case, there is no need
for explicit wakeup, as idle task would get scheduled anyway implicitly.
So idle task is a little bit more difficult than that, unfortunately.
--
Jiri Kosina
SUSE Labs
Re: [PATCH v2 2/3] livepatch: send a fake signal to all blocking tasks
On Thu, Aug 10, 2017 at 12:48:14PM +0200, Miroslav Benes wrote:
> Last, sending the fake signal is not automatic. It is done only when
> admin requests it by writing 1 to force sysfs attribute in livepatch
> sysfs directory.
'writing 1' -> 'writing "signal"'
(unless you take my suggestion to change to two separate sysfs files)
> @@ -468,7 +468,12 @@ static ssize_t force_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> return -EINVAL;
> }
>
> - return -EINVAL;
> + if (!memcmp("signal", buf, min(sizeof("signal")-1, count)))
> + klp_force_signals();
Any reason why you can't just do a strcmp()?
> +++ b/kernel/livepatch/transition.c
> @@ -577,3 +577,43 @@ 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. Only admin can request
> this
> + * action currently.
> + */
> +void klp_force_signals(void)
> +{
> + struct task_struct *g, *task;
> +
> + pr_notice("signalling remaining tasks\n");
As a native US speaker with possible OCD spelling tendencies, it bothers
me to see "signalling" with two l's instead of one. According to
Google, the UK spelling of the word has two l's, so maybe it's not a
typo. I'll forgive you if you don't fix it :-)
> +
> + 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 still has not been migrated.
> + */
> + wake_up_process(task);
> + } 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);
I can't remember if we talked about this before, is it possible to also
signal/wake the idle tasks?
--
Josh
