[PATCH v3 0/2] livepatch: Introduce signal and force sysfs attributes

[PATCH v3 0/2] livepatch: Introduce signal and force sysfs attributes

[Miroslav Benes]

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

Changes from v2:
- two sysfs attributes instead of one - Petr, Josh
- better documentation about force usage - Pavel
- small changes here and there

Changes from v1:
- better wording, typos, comments, documentation - Libor, Josh
- symbolic names in sysfs instead of numbers - Libor
- exit_to_usermode_loop(), call klp_update_patch_state() before do_signal() - Oleg
- better names - Josh
- mutex and WARN_ON_ONCE not added to klp_force_transitions() - Petr, Josh
- handle idle tasks in klp_force_transitions() too - Josh

Miroslav Benes (2):
  livepatch: send a fake signal to all blocking tasks
  livepatch: force transition process to finish

 Documentation/ABI/testing/sysfs-kernel-livepatch | 19 +++++++
 Documentation/livepatch/livepatch.txt            | 21 ++++++++
 arch/powerpc/kernel/signal.c                     |  6 +--
 arch/x86/entry/common.c                          |  6 +--
 kernel/livepatch/core.c                          | 54 ++++++++++++++++++++
 kernel/livepatch/transition.c                    | 65 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  2 +
 kernel/signal.c                                  |  4 +-
 8 files changed, 170 insertions(+), 7 deletions(-)

-- 
2.14.3

[PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

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.

The problem is that a task can block the transition for quite a long
time, if not forever. It could sleep in a set of patched functions, for
example.  Luckily we can force the task to leave the set by sending it a
fake signal, that is a signal with no data in signal pending structures
(no handler, no sign of proper signal delivered). Suspend/freezer use
this to freeze the tasks as well. The task gets TIF_SIGPENDING set and
is woken up (if it has been sleeping in the kernel before) or kicked by
rescheduling IPI (if it was running on other CPU). This causes the task
to go to kernel/userspace boundary where the signal would be handled and
the task would be marked as safe in terms of live patching.

There are tasks which are not affected by this technique though. The
fake signal is not sent to kthreads. They should be handled differently.
They can be woken up so they leave the patched set and their
TIF_PATCH_PENDING can be cleared thanks to stack checking.

For the sake of completeness, if the task is in TASK_RUNNING state but
not currently running on some CPU it doesn't get the IPI, but it would
eventually handle the signal anyway. Second, if the task runs in the
kernel (in TASK_RUNNING state) it gets the IPI, but the signal is not
handled on return from the interrupt. It would be handled on return to
the userspace in the future when the fake signal is sent again. Stack
checking deals with these cases in a better way.

If the task was sleeping in a syscall it would be woken by our fake
signal, it would check if TIF_SIGPENDING is set (by calling
signal_pending() predicate) and return ERESTART* or EINTR. Syscalls with
ERESTART* return values are restarted in case of the fake signal (see
do_signal()). EINTR is propagated back to the userspace program. This
could disturb the program, but...

* each process dealing with signals should react accordingly to EINTR
  return values.
* syscalls returning EINTR happen to be quite common situation in the
  system even if no fake signal is sent.
* freezer sends the fake signal and does not deal with EINTR anyhow.
  Thus EINTR values are returned when the system is resumed.

The very safe marking is done in architectures' "entry" on syscall and
interrupt/exception exit paths, and in a stack checking functions of
livepatch.  TIF_PATCH_PENDING is cleared and the next
recalc_sigpending() drops TIF_SIGPENDING. In connection with this, also
call klp_update_patch_state() before do_signal(), so that
recalc_sigpending() in dequeue_signal() can clear TIF_PATCH_PENDING
immediately and thus prevent a double call of do_signal().

Note that the fake signal is not sent to stopped/traced tasks. Such task
prevents the patching to finish till it continues again (is not traced
anymore).

Last, sending the fake signal is not automatic. It is done only when
admin requests it by writing 1 to signal sysfs attribute in livepatch
sysfs directory.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: x86@kernel.org
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  9 ++++++
 Documentation/livepatch/livepatch.txt            |  9 ++++++
 arch/powerpc/kernel/signal.c                     |  6 ++--
 arch/x86/entry/common.c                          |  6 ++--
 kernel/livepatch/core.c                          | 27 ++++++++++++++++
 kernel/livepatch/transition.c                    | 40 ++++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 kernel/signal.c                                  |  4 ++-
 8 files changed, 95 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index d5d39748382f..22f6267836c2 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -33,6 +33,15 @@ Contact:	live-patching@vger.kernel.org
 		An attribute which indicates whether the patch is currently in
 		transition.
 
+What:		/sys/kernel/livepatch/<patch>/signal
+Date:		Oct 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 signal to
+		all remaining blocking tasks.
+
 What:		/sys/kernel/livepatch/<patch>/<object>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index ecdb18104ab0..6694530d0894 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -178,6 +178,12 @@ 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.
 
+Administrator can also affect a transition through
+/sys/kernel/livepatch/<patch>/signal attribute. Writing 1 to the attribute sends
+a signal to all remaining blocking tasks. This is an alternative for
+SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
+less harmful to the system.
+
 
 3.1 Adding consistency model support to new architectures
 ---------------------------------------------------------
@@ -435,6 +441,9 @@ 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 attribute allows administrator to affect a
+patching operation.
+
 See Documentation/ABI/testing/sysfs-kernel-livepatch for more details.
 
 
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e9436c5e1e09..bf9c4e7792d1 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -153,6 +153,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
 
+	if (thread_info_flags & _TIF_PATCH_PENDING)
+		klp_update_patch_state(current);
+
 	if (thread_info_flags & _TIF_SIGPENDING) {
 		BUG_ON(regs != current->thread.regs);
 		do_signal(current);
@@ -163,9 +166,6 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 		tracehook_notify_resume(regs);
 	}
 
-	if (thread_info_flags & _TIF_PATCH_PENDING)
-		klp_update_patch_state(current);
-
 	user_enter();
 }
 
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 03505ffbe1b6..fb90bdb3602f 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -153,6 +153,9 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		if (cached_flags & _TIF_UPROBE)
 			uprobe_notify_resume(regs);
 
+		if (cached_flags & _TIF_PATCH_PENDING)
+			klp_update_patch_state(current);
+
 		/* deal with pending signal delivery */
 		if (cached_flags & _TIF_SIGPENDING)
 			do_signal(regs);
@@ -165,9 +168,6 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
 			fire_user_return_notifiers();
 
-		if (cached_flags & _TIF_PATCH_PENDING)
-			klp_update_patch_state(current);
-
 		/* Disable IRQs and retry */
 		local_irq_disable();
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bf8c8fd72589..b7c60662baf3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -440,6 +440,7 @@ 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>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
@@ -514,11 +515,37 @@ 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)
+{
+	int ret;
+	bool val;
+
+	/*
+	 * 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)
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		klp_force_signals();
+
+	return count;
+}
+
 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 attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
+	&signal_kobj_attr.attr,
 	NULL
 };
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1fb6032..6700d3b22615 100644
--- a/kernel/livepatch/transition.c
+++ 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("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 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);
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index ce09b326546c..6c480057539a 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -10,5 +10,6 @@ void klp_cancel_transition(void);
 void klp_start_transition(void);
 void klp_try_complete_transition(void);
 void klp_reverse_transition(void);
+void klp_force_signals(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index 800a18f77732..9009b8e77bc3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -40,6 +40,7 @@
 #include <linux/cn_proc.h>
 #include <linux/compiler.h>
 #include <linux/posix-timers.h>
+#include <linux/livepatch.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>
@@ -163,7 +164,8 @@ void recalc_sigpending_and_wake(struct task_struct *t)
 
 void recalc_sigpending(void)
 {
-	if (!recalc_sigpending_tsk(current) && !freezing(current))
+	if (!recalc_sigpending_tsk(current) && !freezing(current) &&
+	    !klp_patch_pending(current))
 		clear_thread_flag(TIF_SIGPENDING);
 
 }
-- 
2.14.3

[PATCH v3 2/2] livepatch: force transition process to finish

[Miroslav Benes]

If a task sleeps in a set of patched functions uninterruptedly, 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 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>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch | 10 +++++++++
 Documentation/livepatch/livepatch.txt            | 24 +++++++++++++++------
 kernel/livepatch/core.c                          | 27 ++++++++++++++++++++++++
 kernel/livepatch/transition.c                    | 25 ++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  1 +
 5 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index 22f6267836c2..105f617008f1 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -42,6 +42,16 @@ Contact:	live-patching@vger.kernel.org
 		course of an existing transition. Writing 1 sends a signal to
 		all remaining blocking tasks.
 
+What:		/sys/kernel/livepatch/<patch>/force
+Date:		Oct 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 clears
+		TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to
+		the patched or unpatched state.
+
 What:		/sys/kernel/livepatch/<patch>/<object>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 6694530d0894..a5f60b96e7b9 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -179,10 +179,22 @@ can be signaled with SIGSTOP and SIGCONT to force them to change their
 patched state.
 
 Administrator can also affect a transition through
-/sys/kernel/livepatch/<patch>/signal attribute. Writing 1 to the attribute sends
-a signal to all remaining blocking tasks. This is an alternative for
-SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
-less harmful to the system.
+/sys/kernel/livepatch/<patch>/signal and /sys/kernel/livepatch/<patch>/force
+attributes. Writing 1 to the signal attribute sends a signal to all remaining blocking
+tasks. This is an alternative for SIGSTOP/SIGCONT approach mentioned in the
+previous paragraph. It should also be less harmful to the system. Writing 1 to
+the force attribute clears TIF_PATCH_PENDING flag of all tasks and thus forces
+the tasks to the patched state.
+
+Important note! Use "force" attribute wisely.  Administrator 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. First, administrator
+needs to check which tasks are not yet (un)patched (see /proc/<pid>/patch_state)
+and why (which functions they're sleeping on. /proc/<pid>/stack may help here.).
+Second, the decision depends on which functions are (un)patched and what is the
+nature of a livepatch. If there is a relation between the two points, "force"
+may cause bad things to your system. Generally, administrator should not use
+this feature without being advised to do so by the livepatch author (vendor).
 
 
 3.1 Adding consistency model support to new architectures
@@ -441,8 +453,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 attribute allows administrator to affect a
-patching operation.
+/sys/kernel/livepatch/<patch>/signal and /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 b7c60662baf3..674ebdf34085 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -441,6 +441,7 @@ EXPORT_SYMBOL_GPL(klp_enable_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>
  */
@@ -539,13 +540,39 @@ static ssize_t signal_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
+{
+	int ret;
+	bool val;
+
+	/*
+	 * 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)
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		klp_force_transitions();
+
+	return count;
+}
+
 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 6700d3b22615..7b17a8889697 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -617,3 +617,28 @@ void klp_force_signals(void)
 	}
 	read_unlock(&tasklist_lock);
 }
+
+/*
+ * Drop TIF_PATCH_PENDING of all tasks on admin's request. This forces an
+ * existing transition to finish.
+ *
+ * NOTE: klp_update_patch_state(task) requires the task to be inactive or
+ * 'current'. This is not the case here and the consistency model could be
+ * broken. Administrator, who is the only one to execute the
+ * klp_force_transitions(), has to be aware of this.
+ */
+void klp_force_transitions(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	pr_warn("forcing remaining tasks to the patched state\n");
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_patch_state(task);
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu)
+		klp_update_patch_state(idle_task(cpu));
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 6c480057539a..da3be48d36bb 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_force_signals(void);
+void klp_force_transitions(void);
 
 #endif /* _LIVEPATCH_TRANSITION_H */
-- 
2.14.3

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

> +/*
> + * 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("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 still has not been migrated.
> +			 */
> +			wake_up_process(task);

So this is not as safe as one would hope. It tries to wake all TASK_NORMAL 
tasks, which could cause headaches. Let's make it

			wake_up_state(task, TASK_INTERRUPTIBLE);

to wake only kthreads sleeping interruptedly.

Thanks Petr for spotting this (offline).

Miroslav

> +		} 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);
> +}

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Petr Mladek]

On Tue 2017-10-31 12:48:52, 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.
> 
> The problem is that a task can block the transition for quite a long
> time, if not forever. It could sleep in a set of patched functions, for
> example.  Luckily we can force the task to leave the set by sending it a
> fake signal, that is a signal with no data in signal pending structures
> (no handler, no sign of proper signal delivered). Suspend/freezer use
> this to freeze the tasks as well. The task gets TIF_SIGPENDING set and
> is woken up (if it has been sleeping in the kernel before) or kicked by
> rescheduling IPI (if it was running on other CPU). This causes the task
> to go to kernel/userspace boundary where the signal would be handled and
> the task would be marked as safe in terms of live patching.
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index b004a1fb6032..6700d3b22615 100644
> --- a/kernel/livepatch/transition.c
> +++ 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("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 still has not been migrated.
> +			 */
> +			wake_up_process(task);

I have just noticed that freezer used wake_up_state(p, TASK_INTERRUPTIBLE);
IMHO, we should do so as well.

wake_up_process() wakes also tasks in TASK_UNINTERRUPTIBLE state.
These might not be ready for an unexpected wakeup. For example,
see concat_dev_erase() in drivers/mtd/mtdcontact.c.

With this change, feel free to use

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Petr Mladek]

On Tue 2017-10-31 12:48:53, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptedly, 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 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>

If no animals were harmed when developing this brute force then
feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Oleg Nesterov]

On 11/01, Petr Mladek wrote:
>
> On Tue 2017-10-31 12:48:52, Miroslav Benes wrote:
> > +		if (task->flags & PF_KTHREAD) {
> > +			/*
> > +			 * Wake up a kthread which still has not been migrated.
> > +			 */
> > +			wake_up_process(task);
>
> I have just noticed that freezer used wake_up_state(p, TASK_INTERRUPTIBLE);
> IMHO, we should do so as well.

I won't argue, but...

> wake_up_process() wakes also tasks in TASK_UNINTERRUPTIBLE state.
> These might not be ready for an unexpected wakeup. For example,
> see concat_dev_erase() in drivers/mtd/mtdcontact.c.

I'd say that concat_dev_erase() should be fixed, any code should be ready
for spurious wakeup.

Note also that wake_up_state(TASK_INTERRUPTIBLE) won't wakeup the TASK_IDLE
kthreads, and most of the kthreads which use TASK_INTERRUPTIBLE should use
TASK_IDLE today, because in most cases TASK_INTERRUPTIBLE was used to not
contribute to loadavg.

Oleg.

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Wed, 1 Nov 2017, Oleg Nesterov wrote:

> On 11/01, Petr Mladek wrote:
> >
> > On Tue 2017-10-31 12:48:52, Miroslav Benes wrote:
> > > +		if (task->flags & PF_KTHREAD) {
> > > +			/*
> > > +			 * Wake up a kthread which still has not been migrated.
> > > +			 */
> > > +			wake_up_process(task);
> >
> > I have just noticed that freezer used wake_up_state(p, TASK_INTERRUPTIBLE);
> > IMHO, we should do so as well.
> 
> I won't argue, but...
> 
> > wake_up_process() wakes also tasks in TASK_UNINTERRUPTIBLE state.
> > These might not be ready for an unexpected wakeup. For example,
> > see concat_dev_erase() in drivers/mtd/mtdcontact.c.
> 
> I'd say that concat_dev_erase() should be fixed, any code should be ready
> for spurious wakeup.

I agree.
 
> Note also that wake_up_state(TASK_INTERRUPTIBLE) won't wakeup the TASK_IDLE
> kthreads, and most of the kthreads which use TASK_INTERRUPTIBLE should use
> TASK_IDLE today, because in most cases TASK_INTERRUPTIBLE was used to not
> contribute to loadavg.

Yes. Unfortunately, we have TASK_IDLE for more than two years now and 
nothing much has happened yet. TASK_IDLE is still used sporadically. I'd 
like to be on the safe side with livepatch and given that 
TASK_INTERRUPTIBLE loops should be prepared for spurious wakeups by 
definition, I think it is better to have wake_up_state(TASK_INTERRUPTIBLE) 
there. At least till all "concat_dev_erase" beauties are fixed (sigh).

Miroslav

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Josh Poimboeuf]

On Tue, Oct 31, 2017 at 12:48:52PM +0100, Miroslav Benes wrote:
> +
> +/*
> + * 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)

Since "force" now has a separate meaning, it's a little confusing to
have it in the name of this function.  How about klp_send_signals() or
klp_signal()?

-- 
Josh

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Josh Poimboeuf]

On Tue, Oct 31, 2017 at 12:48:53PM +0100, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptedly, 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.

The phrase "transition process" (here and in the patch title) confused
me a little bit, since elsewhere we just call it "transition".

> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	int ret;
> +	bool val;
> +
> +	/*
> +	 * 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)
> +		return -EINVAL;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val)
> +		klp_force_transitions();

The plural "transitions" is inconsistent with the rest of the code,
which uses it in the singular.  How about klp_force_transition() or
klp_force()?

-- 
Josh

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Josh Poimboeuf]

On Tue, Oct 31, 2017 at 12:48:52PM +0100, Miroslav Benes wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bf8c8fd72589..b7c60662baf3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -440,6 +440,7 @@ 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>/<object>
>   * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
>   */
> @@ -514,11 +515,37 @@ 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)
> +{
> +	int ret;
> +	bool val;
> +
> +	/*
> +	 * 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)
> +		return -EINVAL;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val)
> +		klp_force_signals();
> +
> +	return count;
> +}

The function still has global functionality even though the sysfs entry
is now per-patch.  So if you do

  echo 1 > /sys/kernel/livepatch/patch1/signal

But patch2 is in transition, then it will send signals based on patch2.
Instead it should probably return an error.

There's a similar issue with force_store().

-- 
Josh

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Oleg Nesterov]

On 11/02, Miroslav Benes wrote:
>
> On Wed, 1 Nov 2017, Oleg Nesterov wrote:
>
> > Note also that wake_up_state(TASK_INTERRUPTIBLE) won't wakeup the TASK_IDLE
> > kthreads, and most of the kthreads which use TASK_INTERRUPTIBLE should use
> > TASK_IDLE today, because in most cases TASK_INTERRUPTIBLE was used to not
> > contribute to loadavg.
>
> Yes. Unfortunately, we have TASK_IDLE for more than two years now and
> nothing much has happened yet. TASK_IDLE is still used sporadically. I'd
> like to be on the safe side with livepatch

OK, as I said I won't argue,

> and given that
> TASK_INTERRUPTIBLE loops should be prepared for spurious wakeups by
> definition,

Not really when it comes to kthreads.

Once again, unless kthread does allow_signal() TASK_INTERRUPTIBLE does
not really differ from TASK_UNINTERRUPTIBLE except the latter contributes
to loadavg. And that is why TASK_INTERRUPTIBLE was commonly used instead
of TASK_UNINTERRUPTIBLE, so I do not think that TASK_INTERRUPTIBLE loops
are more ready in general than TASK_UNINTERRUPTIBLE.

Oleg.

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Thu, 2 Nov 2017, Josh Poimboeuf wrote:

> On Tue, Oct 31, 2017 at 12:48:52PM +0100, Miroslav Benes wrote:
> > +
> > +/*
> > + * 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)
> 
> Since "force" now has a separate meaning, it's a little confusing to
> have it in the name of this function.  How about klp_send_signals() or
> klp_signal()?

or klp_send_signal()?

Miroslav

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Miroslav Benes]

On Thu, 2 Nov 2017, Josh Poimboeuf wrote:

> On Tue, Oct 31, 2017 at 12:48:52PM +0100, Miroslav Benes wrote:
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index bf8c8fd72589..b7c60662baf3 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -440,6 +440,7 @@ 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>/<object>
> >   * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
> >   */
> > @@ -514,11 +515,37 @@ 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)
> > +{
> > +	int ret;
> > +	bool val;
> > +
> > +	/*
> > +	 * 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)
> > +		return -EINVAL;
> > +
> > +	ret = kstrtobool(buf, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val)
> > +		klp_force_signals();
> > +
> > +	return count;
> > +}
> 
> The function still has global functionality even though the sysfs entry
> is now per-patch.  So if you do
> 
>   echo 1 > /sys/kernel/livepatch/patch1/signal
> 
> But patch2 is in transition, then it will send signals based on patch2.
> Instead it should probably return an error.
> 
> There's a similar issue with force_store().

Bah. I still find having "signal" and "force" in 
/sys/kernel/livepatch/<patch>/ a bit odd, but I'll add

        struct klp_patch *patch;

        patch = container_of(kobj, struct klp_patch, kobj);
	/*
	 ...
	 */
	if (patch != klp_transition_patch)
		return -EINVAL;


That should solve it for all cases.

Thanks,
Miroslav

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Miroslav Benes]

On Thu, 2 Nov 2017, Josh Poimboeuf wrote:

> On Tue, Oct 31, 2017 at 12:48:53PM +0100, Miroslav Benes wrote:
> > If a task sleeps in a set of patched functions uninterruptedly, 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.
> 
> The phrase "transition process" (here and in the patch title) confused
> me a little bit, since elsewhere we just call it "transition".

Ok.
 
> > +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	int ret;
> > +	bool val;
> > +
> > +	/*
> > +	 * 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)
> > +		return -EINVAL;
> > +
> > +	ret = kstrtobool(buf, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val)
> > +		klp_force_transitions();
> 
> The plural "transitions" is inconsistent with the rest of the code,
> which uses it in the singular.  How about klp_force_transition() or
> klp_force()?

klp_force_transition() it is.

Miroslav

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Josh Poimboeuf]

On Fri, Nov 03, 2017 at 09:02:50AM +0100, Miroslav Benes wrote:
> On Thu, 2 Nov 2017, Josh Poimboeuf wrote:
> 
> > On Tue, Oct 31, 2017 at 12:48:52PM +0100, Miroslav Benes wrote:
> > > +
> > > +/*
> > > + * 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)
> > 
> > Since "force" now has a separate meaning, it's a little confusing to
> > have it in the name of this function.  How about klp_send_signals() or
> > klp_signal()?
> 
> or klp_send_signal()?

It can send more than one signal, so I'd prefer the plural form:
klp_send_signals().

-- 
Josh

Re: [PATCH v3 1/2] livepatch: send a fake signal to all blocking tasks

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

Hi!

> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -33,6 +33,15 @@ Contact:	live-patching@vger.kernel.org
>  		An attribute which indicates whether the patch is currently in
>  		transition.
>  
> +What:		/sys/kernel/livepatch/<patch>/signal
> +Date:		Oct 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 signal to
> +		all remaining blocking tasks.

What kind of signal?

>  What:		/sys/kernel/livepatch/<patch>/<object>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
> diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
> index ecdb18104ab0..6694530d0894 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -178,6 +178,12 @@ 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.
>  
> +Administrator can also affect a transition through
> +/sys/kernel/livepatch/<patch>/signal attribute. Writing 1 to the attribute sends
> +a signal to all remaining blocking tasks. This is an alternative for
> +SIGSTOP/SIGCONT approach mentioned in the previous paragraph. It should also be
> +less harmful to the system.

Well... If SIGSTOP / SIGCONT is considered harmful (it probably is),
it should be mentioned above, and not in note here...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On Tue 2017-10-31 12:48:53, Miroslav Benes wrote:
> If a task sleeps in a set of patched functions uninterruptedly, 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 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>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch | 10 +++++++++
>  Documentation/livepatch/livepatch.txt            | 24 +++++++++++++++------
>  kernel/livepatch/core.c                          | 27 ++++++++++++++++++++++++
>  kernel/livepatch/transition.c                    | 25 ++++++++++++++++++++++
>  kernel/livepatch/transition.h                    |  1 +
>  5 files changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index 22f6267836c2..105f617008f1 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -42,6 +42,16 @@ Contact:	live-patching@vger.kernel.org
>  		course of an existing transition. Writing 1 sends a signal to
>  		all remaining blocking tasks.
>  
> +What:		/sys/kernel/livepatch/<patch>/force
> +Date:		Oct 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 clears
> +		TIF_PATCH_PENDING flag of all tasks and thus forces the tasks to
> +		the patched or unpatched state.
> +

NAK. Admin does not have chance to decide if this is safe or not.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Jiri Kosina]

On Mon, 6 Nov 2017, Pavel Machek wrote:

> NAK. Admin does not have chance to decide if this is safe or not.

The process that is actually being suggested here (and towards which the 
implementation is heading) is that in case of the convergence being stuck 
for unknown reason, the admin of the system collects all the necessary 
data (namely stacktraces of the relevant tasks), and requests a clearance 
from the livepatch distributor to force the transition.

But I agree with you that this might not be completely clear from the 
documentation, and should be made more explicit.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 2/2] livepatch: force transition process to finish

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Mon 2017-11-06 13:03:32, Jiri Kosina wrote:
> On Mon, 6 Nov 2017, Pavel Machek wrote:
> 
> > NAK. Admin does not have chance to decide if this is safe or not.
> 
> The process that is actually being suggested here (and towards which the 
> implementation is heading) is that in case of the convergence being stuck 
> for unknown reason, the admin of the system collects all the necessary 
> data (namely stacktraces of the relevant tasks), and requests a clearance 
> from the livepatch distributor to force the transition.
> 
> But I agree with you that this might not be completely clear from the 
> documentation, and should be made more explicit.

Yes, please.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]