Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644397 > unrolled thread

[PATCH 0/3] livepatch: Introduce force sysfs attribute

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] livepatch: Introduce force sysfs attribute Miroslav Benes <mbenes@suse.cz> - 2017-05-18 14:10 +0200
    [PATCH 3/3] livepatch: force transition process to finish Miroslav Benes <mbenes@suse.cz> - 2017-05-18 14:10 +0200
      Re: [PATCH 3/3] livepatch: force transition process to finish Libor Pechacek <lpechacek@suse.com> - 2017-05-18 15:20 +0200
        Re: [PATCH 3/3] livepatch: force transition process to finish Miroslav Benes <mbenes@suse.cz> - 2017-05-18 15:30 +0200
      Re: [PATCH 3/3] livepatch: force transition process to finish Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-23 19:30 +0200
        Re: [PATCH 3/3] livepatch: force transition process to finish Miroslav Benes <mbenes@suse.cz> - 2017-05-24 10:40 +0200
      Re: [PATCH 3/3] livepatch: force transition process to finish Petr Mladek <pmladek@suse.com> - 2017-05-24 15:10 +0200
        Re: [PATCH 3/3] livepatch: force transition process to finish Miroslav Benes <mbenes@suse.cz> - 2017-05-24 16:20 +0200
          Re: [PATCH 3/3] livepatch: force transition process to finish Petr Mladek <pmladek@suse.com> - 2017-05-24 17:20 +0200
    [PATCH 1/3] livepatch: Add force sysfs attribute Miroslav Benes <mbenes@suse.cz> - 2017-05-18 14:10 +0200
      Re: [PATCH 1/3] livepatch: Add force sysfs attribute Libor Pechacek <lpechacek@suse.com> - 2017-05-18 15:10 +0200
        Re: [PATCH 1/3] livepatch: Add force sysfs attribute Miroslav Benes <mbenes@suse.cz> - 2017-05-18 15:30 +0200

#1644397 — [PATCH 0/3] livepatch: Introduce force sysfs attribute

Currently, livepatch gradually migrate the system from an unpatched to a
patched state (or vice versa). Each task drops its TIF_PATCH_PENDING
itself when crossing the kernel/user space boundary or it is cleared
using the stack checking approach. If there is a task which sleeps on a
patched function, the whole transition can get stuck indefinitely.

Livepatch has means which can be used in these cases. The transition can
be cancelled and/or immediate flag may be used for the live patch. On
the other hand it might be useful to poke the system a little bit and
help the transition to finish by doing so.

That is what the fake signal can be used for. A task sleeping/waiting in
the kernel gets TIF_SIGPENDING set, it handles it and during that its
TIF_PATCH_PENDING is cleared. Kthreads are only woken up, they do not
handle signals suitably.

Still, there are cases which neither fake signal can solve. A task can
sleep uninterruptibly without reacting to signals at all. Even then, it
may be safe to clear the task's TIF_PATCH_PENDING. As a last resort,
admin may force such clearing for all tasks in the system with this
patch set.

We use the fake signal in SLES for a long time. Moreover, we don't have
a stack checking there, so we rely on the fake signal a lot. We send it
automatically and periodically.

The first patch is only preparatory. It introduces the sysfs attribute
through which both actions are performed. The second patch adds the fake
signal and the third one forced clearing of the flag.

Miroslav Benes (3):
  livepatch: Add force sysfs attribute
  livepatch: send a fake signal to all blocking tasks
  livepatch: force transition process to finish

 Documentation/ABI/testing/sysfs-kernel-livepatch |  9 ++++
 include/linux/livepatch.h                        |  4 ++
 kernel/livepatch/core.c                          | 51 +++++++++++++++++++++
 kernel/livepatch/transition.c                    | 56 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  2 +
 kernel/signal.c                                  |  4 +-
 6 files changed, 125 insertions(+), 1 deletion(-)

-- 
2.12.2

[toc] | [next] | [standalone]

#1644409 — [PATCH 3/3] livepatch: force transition process to finish

If a task sleeps in a set of patched functions uninterruptibly, it could
block the whole transition process indefinitely.  Thus it may be useful
to clear its TIF_PATCH_PENDING to allow the process to finish.

Admin can do that now by writing 2 to force sysfs attribute in livepatch
sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
transition can finish successfully.

Important note! Use wisely. Admin must be sure that it is safe to
execute such action. This means that it must be checked that by doing so
the consistency model guarantees are not violated.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h     |  1 +
 kernel/livepatch/core.c       |  3 +++
 kernel/livepatch/transition.c | 16 ++++++++++++++++
 kernel/livepatch/transition.h |  1 +
 4 files changed, 21 insertions(+)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 43cfeebeb42b..b567208a1c6e 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -31,6 +31,7 @@
 
 /* values for sysfs force attribute */
 #define KLP_FORCE_FAKE		1
+#define KLP_FORCE_UNMARK	2
 
 /* task patch states */
 #define KLP_UNDEFINED	-1
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bb3b78fa7d2b..9bc1103348c9 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 	case KLP_FORCE_FAKE:
 		klp_send_fake_signal();
 		break;
+	case KLP_FORCE_UNMARK:
+		klp_unmark_tasks();
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index bb61aaa196d3..d057a34510e6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
 	}
 	read_unlock(&tasklist_lock);
 }
+
+/*
+ * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
+ * existing transition to finish.
+ */
+void klp_unmark_tasks(void)
+{
+	struct task_struct *g, *task;
+
+	pr_warn("all tasks marked as migrated on admin's request\n");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_patch_state(task);
+	read_unlock(&tasklist_lock);
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 1c7ede6eaa77..c129397b3985 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -11,5 +11,6 @@ void klp_start_transition(void);
 void klp_try_complete_transition(void);
 void klp_reverse_transition(void);
 void klp_send_fake_signal(void);
+void klp_unmark_tasks(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1644484 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Thu 18-05-17 14:00:43, Miroslav Benes wrote:
[...]
> Admin can do that now by writing 2 to force sysfs attribute in livepatch
> sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> transition can finish successfully.
[...]
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -31,6 +31,7 @@
>  
>  /* values for sysfs force attribute */
>  #define KLP_FORCE_FAKE		1
> +#define KLP_FORCE_UNMARK	2

Again, needs documentation. I would consider using symbolic names instead of
numbers, like /sys/power/state does, even more friendly. Possibility to read a
list of available actions would help user space tools identify what is
available in case the list grows in the future.

Libor
-- 
Libor Pechacek
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1644504 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Thu, 18 May 2017, Libor Pechacek wrote:

> On Thu 18-05-17 14:00:43, Miroslav Benes wrote:
> [...]
> > Admin can do that now by writing 2 to force sysfs attribute in livepatch
> > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > transition can finish successfully.
> [...]
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -31,6 +31,7 @@
> >  
> >  /* values for sysfs force attribute */
> >  #define KLP_FORCE_FAKE		1
> > +#define KLP_FORCE_UNMARK	2
> 
> Again, needs documentation. I would consider using symbolic names instead of
> numbers, like /sys/power/state does, even more friendly. Possibility to read a
> list of available actions would help user space tools identify what is
> available in case the list grows in the future.

I like that! -> v2

Thanks,
Miroslav

[toc] | [prev] | [next] | [standalone]

#1648291 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Thu, May 18, 2017 at 02:00:43PM +0200, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptibly, it could
> block the whole transition process indefinitely.  Thus it may be useful
> to clear its TIF_PATCH_PENDING to allow the process to finish.
> 
> Admin can do that now by writing 2 to force sysfs attribute in livepatch
> sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> transition can finish successfully.
> 
> Important note! Use wisely. Admin must be sure that it is safe to
> execute such action. This means that it must be checked that by doing so
> the consistency model guarantees are not violated.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

These patches look good to me.  Just some minor comments.

> ---
>  include/linux/livepatch.h     |  1 +
>  kernel/livepatch/core.c       |  3 +++
>  kernel/livepatch/transition.c | 16 ++++++++++++++++
>  kernel/livepatch/transition.h |  1 +
>  4 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 43cfeebeb42b..b567208a1c6e 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -31,6 +31,7 @@
>  
>  /* values for sysfs force attribute */
>  #define KLP_FORCE_FAKE		1
> +#define KLP_FORCE_UNMARK	2
>  
>  /* task patch states */
>  #define KLP_UNDEFINED	-1
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bb3b78fa7d2b..9bc1103348c9 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	case KLP_FORCE_FAKE:
>  		klp_send_fake_signal();
>  		break;
> +	case KLP_FORCE_UNMARK:
> +		klp_unmark_tasks();
> +		break;

I think the naming could be a little clearer, and more consistent.  What
do you think about:

KLP_FORCE_SIGNALS -> klp_force_signals()
KLP_FORCE_TRANSITIONS -> klp_force_transitions()

>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index bb61aaa196d3..d057a34510e6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
>  	}
>  	read_unlock(&tasklist_lock);
>  }
> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + */
> +void klp_unmark_tasks(void)
> +{
> +	struct task_struct *g, *task;
> +
> +	pr_warn("all tasks marked as migrated on admin's request\n");

The user might not know what migrated means.  How about "forcing
remaining tasks to the patched state" or something similar?

> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task)
> +		klp_update_patch_state(task);
> +	read_unlock(&tasklist_lock);

So klp_update_patch_state() has the following comment:

 * NOTE: If task is not 'current', the caller must ensure the task is inactive.
 * Otherwise klp_ftrace_handler() might read the wrong 'patch_state' value.

This code doesn't ensure the task is inactive.  But I think that's ok as
long as we document the fact that this could break the consistency
model, right?

On a related note, I think the new sysfs entry should also be documented
in Documentation/livepatch/livepatch.txt somewhere.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1649281 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Tue, 23 May 2017, Josh Poimboeuf wrote:

> On Thu, May 18, 2017 at 02:00:43PM +0200, Miroslav Benes wrote:
> > If a task sleeps in a set of patched functions uninterruptibly, it could
> > block the whole transition process indefinitely.  Thus it may be useful
> > to clear its TIF_PATCH_PENDING to allow the process to finish.
> > 
> > Admin can do that now by writing 2 to force sysfs attribute in livepatch
> > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > transition can finish successfully.
> > 
> > Important note! Use wisely. Admin must be sure that it is safe to
> > execute such action. This means that it must be checked that by doing so
> > the consistency model guarantees are not violated.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> These patches look good to me.  Just some minor comments.
> 
> > ---
> >  include/linux/livepatch.h     |  1 +
> >  kernel/livepatch/core.c       |  3 +++
> >  kernel/livepatch/transition.c | 16 ++++++++++++++++
> >  kernel/livepatch/transition.h |  1 +
> >  4 files changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 43cfeebeb42b..b567208a1c6e 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -31,6 +31,7 @@
> >  
> >  /* values for sysfs force attribute */
> >  #define KLP_FORCE_FAKE		1
> > +#define KLP_FORCE_UNMARK	2
> >  
> >  /* task patch states */
> >  #define KLP_UNDEFINED	-1
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index bb3b78fa7d2b..9bc1103348c9 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -469,6 +469,9 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  	case KLP_FORCE_FAKE:
> >  		klp_send_fake_signal();
> >  		break;
> > +	case KLP_FORCE_UNMARK:
> > +		klp_unmark_tasks();
> > +		break;
> 
> I think the naming could be a little clearer, and more consistent.  What
> do you think about:
> 
> KLP_FORCE_SIGNALS -> klp_force_signals()
> KLP_FORCE_TRANSITIONS -> klp_force_transitions()

Yes, that is better.
 
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index bb61aaa196d3..d057a34510e6 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
> >  	}
> >  	read_unlock(&tasklist_lock);
> >  }
> > +
> > +/*
> > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > + * existing transition to finish.
> > + */
> > +void klp_unmark_tasks(void)
> > +{
> > +	struct task_struct *g, *task;
> > +
> > +	pr_warn("all tasks marked as migrated on admin's request\n");
> 
> The user might not know what migrated means.  How about "forcing
> remaining tasks to the patched state" or something similar?

Ok.

> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process_thread(g, task)
> > +		klp_update_patch_state(task);
> > +	read_unlock(&tasklist_lock);
> 
> So klp_update_patch_state() has the following comment:
> 
>  * NOTE: If task is not 'current', the caller must ensure the task is inactive.
>  * Otherwise klp_ftrace_handler() might read the wrong 'patch_state' value.
> 
> This code doesn't ensure the task is inactive.  But I think that's ok as
> long as we document the fact that this could break the consistency
> model, right?

Correct. I'll add a comment to klp_unmark_tasks()/klp_force_transitions().

> On a related note, I think the new sysfs entry should also be documented
> in Documentation/livepatch/livepatch.txt somewhere.

No problem.

Thanks,
Miroslav

[toc] | [prev] | [next] | [standalone]

#1649607 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Thu 2017-05-18 14:00:43, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptibly, it could
> block the whole transition process indefinitely.  Thus it may be useful
> to clear its TIF_PATCH_PENDING to allow the process to finish.
> 
> Admin can do that now by writing 2 to force sysfs attribute in livepatch
> sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> transition can finish successfully.
> 
> Important note! Use wisely. Admin must be sure that it is safe to
> execute such action. This means that it must be checked that by doing so
> the consistency model guarantees are not violated.
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index bb61aaa196d3..d057a34510e6 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
>  	}
>  	read_unlock(&tasklist_lock);
>  }
> +
> +/*
> + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> + * existing transition to finish.
> + */
> +void klp_unmark_tasks(void)
> +{
> +	struct task_struct *g, *task;
> +
> +	pr_warn("all tasks marked as migrated on admin's request\n");
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task)
> +		klp_update_patch_state(task);
> +	read_unlock(&tasklist_lock);

This should get called under klp_mutex. The following race comes to my mind:

CPU0:					CPU1:

klp_transition_work_fn()
  klp_try_complete_transition()
    for_each_process()
	if (!klp_try_switch_task(task))

	# success

   klp_complete_transition()

     for_each_process()
	task->patch_state = KLP_UNDEFINED;


					klp_unmark_tasks()
					  for_each_process()
					    klp_update_patch_state()
					      task->patch_state =
						klp_target_state;

	klp_target_state = KLP_UNDEFINED;

=> CPU1 might happily set an obsolete state and create a mess.

It would be possible to solve this by reodering, barriers.
But much better solution seems to serialize both actions
using klp_mutex.

In fact, I would suggest to take klp_mutex in force_store()
and do all actions synchronously, including the check
of klp_transition_patch.

Best Regards,
Petr

PS: I know that I talked about this with Mirek and suggested
doing the check for klp_transition_patch without the lock.
It made perfect sense. But I have changed my mind when
seeing the final code.

[toc] | [prev] | [next] | [standalone]

#1649653 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Wed, 24 May 2017, Petr Mladek wrote:

> On Thu 2017-05-18 14:00:43, Miroslav Benes wrote:
> > If a task sleeps in a set of patched functions uninterruptibly, it could
> > block the whole transition process indefinitely.  Thus it may be useful
> > to clear its TIF_PATCH_PENDING to allow the process to finish.
> > 
> > Admin can do that now by writing 2 to force sysfs attribute in livepatch
> > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > transition can finish successfully.
> > 
> > Important note! Use wisely. Admin must be sure that it is safe to
> > execute such action. This means that it must be checked that by doing so
> > the consistency model guarantees are not violated.
> > 
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index bb61aaa196d3..d057a34510e6 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
> >  	}
> >  	read_unlock(&tasklist_lock);
> >  }
> > +
> > +/*
> > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > + * existing transition to finish.
> > + */
> > +void klp_unmark_tasks(void)
> > +{
> > +	struct task_struct *g, *task;
> > +
> > +	pr_warn("all tasks marked as migrated on admin's request\n");
> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process_thread(g, task)
> > +		klp_update_patch_state(task);
> > +	read_unlock(&tasklist_lock);
> 
> This should get called under klp_mutex. The following race comes to my mind:
> 
> CPU0:					CPU1:
> 
> klp_transition_work_fn()
>   klp_try_complete_transition()
>     for_each_process()
> 	if (!klp_try_switch_task(task))
> 
> 	# success
> 
>    klp_complete_transition()
> 
>      for_each_process()
> 	task->patch_state = KLP_UNDEFINED;
> 
> 
> 					klp_unmark_tasks()
> 					  for_each_process()
> 					    klp_update_patch_state()
> 					      task->patch_state =
> 						klp_target_state;
> 
> 	klp_target_state = KLP_UNDEFINED;
> 
> => CPU1 might happily set an obsolete state and create a mess.

This should not happen. klp_update_patch_state() use 
test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING) and only if true, 
task->patch_state is set.

And all TIF_PATCH_PENDING are cleared when you get 
klp_complete_transition().
 
> It would be possible to solve this by reodering, barriers.
> But much better solution seems to serialize both actions
> using klp_mutex.
> 
> In fact, I would suggest to take klp_mutex in force_store()
> and do all actions synchronously, including the check
> of klp_transition_patch.

I still think it is better not do it. klp_unmark_tasks() does nothing else 
than tasks already do. They call klp_update_patch_state() by themselves 
and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
only forces this action.

On the other hand, I do not see a problem in doing that. We already have a 
relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
is safe. It would only serialize things needlessly.

Thanks,
Miroslav

[toc] | [prev] | [next] | [standalone]

#1649701 — Re: [PATCH 3/3] livepatch: force transition process to finish

On Wed 2017-05-24 16:15:49, Miroslav Benes wrote:
> On Wed, 24 May 2017, Petr Mladek wrote:
> 
> > On Thu 2017-05-18 14:00:43, Miroslav Benes wrote:
> > > If a task sleeps in a set of patched functions uninterruptibly, it could
> > > block the whole transition process indefinitely.  Thus it may be useful
> > > to clear its TIF_PATCH_PENDING to allow the process to finish.
> > > 
> > > Admin can do that now by writing 2 to force sysfs attribute in livepatch
> > > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > > transition can finish successfully.
> > > 
> > > Important note! Use wisely. Admin must be sure that it is safe to
> > > execute such action. This means that it must be checked that by doing so
> > > the consistency model guarantees are not violated.
> > > 
> > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > > index bb61aaa196d3..d057a34510e6 100644
> > > --- a/kernel/livepatch/transition.c
> > > +++ b/kernel/livepatch/transition.c
> > > @@ -591,3 +591,19 @@ void klp_send_fake_signal(void)
> > >  	}
> > >  	read_unlock(&tasklist_lock);
> > >  }
> > > +
> > > +/*
> > > + * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
> > > + * existing transition to finish.
> > > + */
> > > +void klp_unmark_tasks(void)
> > > +{
> > > +	struct task_struct *g, *task;
> > > +
> > > +	pr_warn("all tasks marked as migrated on admin's request\n");
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +	for_each_process_thread(g, task)
> > > +		klp_update_patch_state(task);
> > > +	read_unlock(&tasklist_lock);
> > 
> > This should get called under klp_mutex. The following race comes to my mind:
> > 
> > CPU0:					CPU1:
> > 
> > klp_transition_work_fn()
> >   klp_try_complete_transition()
> >     for_each_process()
> > 	if (!klp_try_switch_task(task))
> > 
> > 	# success
> > 
> >    klp_complete_transition()
> > 
> >      for_each_process()
> > 	task->patch_state = KLP_UNDEFINED;
> > 
> > 
> > 					klp_unmark_tasks()
> > 					  for_each_process()
> > 					    klp_update_patch_state()
> > 					      task->patch_state =
> > 						klp_target_state;
> > 
> > 	klp_target_state = KLP_UNDEFINED;
> > 
> > => CPU1 might happily set an obsolete state and create a mess.
> 
> This should not happen. klp_update_patch_state() use 
> test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING) and only if true, 
> task->patch_state is set.
> 
> And all TIF_PATCH_PENDING are cleared when you get 
> klp_complete_transition().

You are right. I missed that klp_update_patch_state() checked
the TIF flag before setting the state.


> > In fact, I would suggest to take klp_mutex in force_store()
> > and do all actions synchronously, including the check
> > of klp_transition_patch.
> 
> I still think it is better not do it. klp_unmark_tasks() does nothing else 
> than tasks already do. They call klp_update_patch_state() by themselves 
> and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> only forces this action.

You have a point. But I am not convinced ;-) klp_update_patch_state()
was called very carefully only when it was safe. The forcing
intentionally breaks the consistency model. User should really know
what they are doing when they use this feature.

I think that we should actually taint the kernel. Developers should
know when users were pulling their legs.


> On the other hand, I do not see a problem in doing that. We already have a 
> relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> is safe.

Yup.

> It would only serialize things needlessly.

I do not agree. The speed is not important here. Also look
into klp_reverse_transition(). We explicitly clear all
TIF_PATCH_PENDING flags and call synchronize_rcu() just
to make the situation easier and reduce space for potential
mistakes.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1644416 — [PATCH 1/3] livepatch: Add force sysfs attribute

Add write-only force attribute to livepatch sysfs infrastructure. We can
use it later to force couple of events during a live patching process.
Be it a sending of a fake signal or forcing of the tasks' successful
conversion.

It does not make sense to use the force facility when there is no
transaction running (although there is no harm doing that). Therefore we
limit only to situations when klp_transition_patch variable is set.
Normally, klp_mutex lock should be grabbed, because the variable is
shared. However that would hold the action back unnecessarily because of
waiting for the lock, so we omit the lock here. The resulting race
window is harmless (using force when there is no transaction running).

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  9 +++++
 kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index d5d39748382f..26e9f58cea9e 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -8,6 +8,15 @@ Contact:	live-patching@vger.kernel.org
 		The /sys/kernel/livepatch directory contains subdirectories for
 		each loaded live patch module.
 
+What:		/sys/kernel/livepatch/force
+Date:		May 2017
+KernelVersion:	4.13.0
+Contact:	live-patching@vger.kernel.org
+Description:
+		A write-only attribute that allows administrator to affect the
+		course of an existing transition. A fake signal can be send or
+		tasks TIF can be cleared.
+
 What:		/sys/kernel/livepatch/<patch>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..84f8944704ad 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -437,6 +437,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * Sysfs Interface
  *
  * /sys/kernel/livepatch
+ * /sys/kernel/livepatch/force
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
@@ -444,6 +445,43 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * klp_mutex lock is not grabbed here intentionally. It is not really
+	 * needed. The race window is harmless and grabbing the lock would only
+	 * hold the action back.
+	 */
+	if (!klp_transition_patch) {
+		pr_info("no patching in progress. Force not allowed\n");
+		return -EINVAL;
+	}
+
+	switch (val) {
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
+static struct attribute *klp_attrs[] = {
+	&force_kobj_attr.attr,
+	NULL
+};
+static struct attribute_group klp_sysfs_group = {
+	.attrs = klp_attrs,
+};
+
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 			     const char *buf, size_t count)
 {
@@ -954,6 +992,13 @@ static int __init klp_init(void)
 	if (!klp_root_kobj)
 		return -ENOMEM;
 
+	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
+	if (ret) {
+		pr_err("cannot create livepatch attributes in sysfs\n");
+		kobject_put(klp_root_kobj);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1644464 — Re: [PATCH 1/3] livepatch: Add force sysfs attribute

On Thu 18-05-17 14:00:41, Miroslav Benes wrote:
> Add write-only force attribute to livepatch sysfs infrastructure. We can
> use it later to force couple of events during a live patching process.
> Be it a sending of a fake signal or forcing of the tasks' successful
> conversion.
> 
> It does not make sense to use the force facility when there is no
> transaction running (although there is no harm doing that). Therefore we
> limit only to situations when klp_transition_patch variable is set.
> Normally, klp_mutex lock should be grabbed, because the variable is
> shared. However that would hold the action back unnecessarily because of
> waiting for the lock, so we omit the lock here. The resulting race
> window is harmless (using force when there is no transaction running).
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  9 +++++
>  kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index d5d39748382f..26e9f58cea9e 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -8,6 +8,15 @@ Contact:	live-patching@vger.kernel.org
>  		The /sys/kernel/livepatch directory contains subdirectories for
>  		each loaded live patch module.
>  
> +What:		/sys/kernel/livepatch/force
> +Date:		May 2017
> +KernelVersion:	4.13.0
> +Contact:	live-patching@vger.kernel.org
> +Description:
> +		A write-only attribute that allows administrator to affect the
> +		course of an existing transition. A fake signal can be send or
								       ^^^^
								      "sent"

> +		tasks TIF can be cleared.
> +
>  What:		/sys/kernel/livepatch/<patch>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..84f8944704ad 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -437,6 +437,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * Sysfs Interface
>   *
>   * /sys/kernel/livepatch
> + * /sys/kernel/livepatch/force
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/transition
> @@ -444,6 +445,43 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
>   */
>  
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * klp_mutex lock is not grabbed here intentionally. It is not really
> +	 * needed. The race window is harmless and grabbing the lock would only
> +	 * hold the action back.
> +	 */
> +	if (!klp_transition_patch) {
> +		pr_info("no patching in progress. Force not allowed\n");

proposing smoother wording and information sharing
pr_info("no patching in progress, forced action (%d) ineffective", val);

> +		return -EINVAL;
> +	}
> +
> +	switch (val) {

I felt strong confusion for a while looking at a function what does nothing. A
comment that this is intentionally an empty shell, at this stage, would be
welcome.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
> +static struct attribute *klp_attrs[] = {
> +	&force_kobj_attr.attr,
> +	NULL
> +};
> +static struct attribute_group klp_sysfs_group = {
> +	.attrs = klp_attrs,
> +};
> +
>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			     const char *buf, size_t count)
>  {
> @@ -954,6 +992,13 @@ static int __init klp_init(void)
>  	if (!klp_root_kobj)
>  		return -ENOMEM;
>  
> +	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
> +	if (ret) {
> +		pr_err("cannot create livepatch attributes in sysfs\n");
> +		kobject_put(klp_root_kobj);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  

Libor

> -- 
> 2.12.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Libor Pechacek
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1644498 — Re: [PATCH 1/3] livepatch: Add force sysfs attribute

On Thu, 18 May 2017, Libor Pechacek wrote:

> On Thu 18-05-17 14:00:41, Miroslav Benes wrote:
> >
> > +		pr_info("no patching in progress. Force not allowed\n");
> 
> proposing smoother wording and information sharing
> pr_info("no patching in progress, forced action (%d) ineffective", val);

That is better. Thanks.

> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (val) {
> 
> I felt strong confusion for a while looking at a function what does nothing. A
> comment that this is intentionally an empty shell, at this stage, would be
> welcome.

Yes, I wanted to keep that sysfs glue separate from both implementations 
to make the review easier. Despite the ugly outcome. And it is confusing.

Comment sounds good.

Thanks,
Miroslav

> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return count;
> > +}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web