I ended with three patches in the end. The first one is the same
as before [1]. Then I added two patches for handling of the stacktrace
error messages.
[1] https://lkml.kernel.org/r/[email protected]
Petr Mladek (3):
livepatch: Convert error about unsupported reliable stacktrace into a
warning
stacktrace: Remove superfluous WARN_ONCE() from
save_stack_trace_tsk_reliable()
livepatch: Cleanup message handling in klp_try_switch_task()
kernel/livepatch/core.c | 5 ++---
kernel/livepatch/transition.c | 19 ++++++++++++++-----
kernel/stacktrace.c | 1 -
3 files changed, 16 insertions(+), 9 deletions(-)
--
2.16.4
WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
The only current user klp_check_stack() writes its own warning when
-ENOSYS is returned.
Signed-off-by: Petr Mladek <[email protected]>
---
kernel/stacktrace.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index f8edee9c792d..83ac0ac5ffd9 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -74,6 +74,5 @@ __weak int
save_stack_trace_tsk_reliable(struct task_struct *tsk,
struct stack_trace *trace)
{
- WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
return -ENOSYS;
}
--
2.16.4
The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
that any livepatch was refused when reliable stacktraces were not supported
on the given architecture.
The limitation is too strong. User space processes are safely migrated
even when entering or leaving the kernel. Kthreads transition would
need to get forced. But it is safe when:
+ The livepatch does not change the semantic of the code.
+ Callbacks do not depend on a safely finished transition.
Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
kernel/livepatch/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..14f33ab6c583 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1003,11 +1003,10 @@ int klp_enable_patch(struct klp_patch *patch)
return -ENODEV;
if (!klp_have_reliable_stack()) {
- pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
- return -EOPNOTSUPP;
+ pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
+ pr_warn("The livepatch transition may never complete.\n");
}
-
mutex_lock(&klp_mutex);
ret = klp_init_patch_early(patch);
--
2.16.4
WARN_ON_ONCE() could not be called safely under rq lock because
of console deadlock issues. Fortunately, simple printk_deferred()
is enough because the warning is printed from a well defined
location and context.
Also klp_try_switch_task() is called under klp_mutex.
Therefore, the buffer for debug messages could be static.
Signed-off-by: Petr Mladek <[email protected]>
---
kernel/livepatch/transition.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 9c89ae8b337a..e8183d18227f 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
static int klp_check_stack(struct task_struct *task, char *err_buf)
{
static unsigned long entries[MAX_STACK_ENTRIES];
+ static int enosys_warned;
struct stack_trace trace;
struct klp_object *obj;
struct klp_func *func;
@@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
trace.nr_entries = 0;
trace.max_entries = MAX_STACK_ENTRIES;
trace.entries = entries;
+
ret = save_stack_trace_tsk_reliable(task, &trace);
- WARN_ON_ONCE(ret == -ENOSYS);
+ if (ret == -ENOSYS) {
+ if (!enosys_warned) {
+ printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
+ __func__);
+ enosys_warned = 1;
+ }
+ return ret;
+ }
if (ret) {
snprintf(err_buf, STACK_ERR_BUF_SIZE,
"%s: %s:%d has an unreliable stack\n",
@@ -297,11 +306,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
*/
static bool klp_try_switch_task(struct task_struct *task)
{
+ static char err_buf[STACK_ERR_BUF_SIZE];
struct rq *rq;
struct rq_flags flags;
int ret;
bool success = false;
- char err_buf[STACK_ERR_BUF_SIZE];
err_buf[0] = '\0';
@@ -336,15 +345,15 @@ static bool klp_try_switch_task(struct task_struct *task)
task_rq_unlock(rq, task, &flags);
/*
- * Due to console deadlock issues, pr_debug() can't be used while
- * holding the task rq lock. Instead we have to use a temporary buffer
+ * printk_deferred() need to be used under rq lock to avoid
+ * console deadlock issues. But it does not support the dynamic
+ * debug feature. Instead we have to use a temporary buffer
* and print the debug message after releasing the lock.
*/
if (err_buf[0] != '\0')
pr_debug("%s", err_buf);
return success;
-
}
/*
--
2.16.4
On Wed 2019-04-24 10:55:49, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed, 24 Apr 2019, Petr Mladek wrote:
> WARN_ON_ONCE() could not be called safely under rq lock because
> of console deadlock issues. Fortunately, simple printk_deferred()
> is enough because the warning is printed from a well defined
> location and context.
>
> Also klp_try_switch_task() is called under klp_mutex.
> Therefore, the buffer for debug messages could be static.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/transition.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 9c89ae8b337a..e8183d18227f 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> static int klp_check_stack(struct task_struct *task, char *err_buf)
> {
> static unsigned long entries[MAX_STACK_ENTRIES];
> + static int enosys_warned;
> struct stack_trace trace;
> struct klp_object *obj;
> struct klp_func *func;
> @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> trace.nr_entries = 0;
> trace.max_entries = MAX_STACK_ENTRIES;
> trace.entries = entries;
> +
> ret = save_stack_trace_tsk_reliable(task, &trace);
> - WARN_ON_ONCE(ret == -ENOSYS);
> + if (ret == -ENOSYS) {
> + if (!enosys_warned) {
> + printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> + __func__);
> + enosys_warned = 1;
... abusing the fact that you are also printk maintainer :) ... looking at
the above, wouldn't it make sense to introduce generic
printk_deferred_once() instead?
--
Jiri Kosina
SUSE Labs
On Wed 2019-04-24 12:41:00, Jiri Kosina wrote:
> On Wed, 24 Apr 2019, Petr Mladek wrote:
>
> > WARN_ON_ONCE() could not be called safely under rq lock because
> > of console deadlock issues. Fortunately, simple printk_deferred()
> > is enough because the warning is printed from a well defined
> > location and context.
> >
> > Also klp_try_switch_task() is called under klp_mutex.
> > Therefore, the buffer for debug messages could be static.
> >
> > Signed-off-by: Petr Mladek <[email protected]>
> > ---
> > kernel/livepatch/transition.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 9c89ae8b337a..e8183d18227f 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> > static int klp_check_stack(struct task_struct *task, char *err_buf)
> > {
> > static unsigned long entries[MAX_STACK_ENTRIES];
> > + static int enosys_warned;
> > struct stack_trace trace;
> > struct klp_object *obj;
> > struct klp_func *func;
> > @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> > trace.nr_entries = 0;
> > trace.max_entries = MAX_STACK_ENTRIES;
> > trace.entries = entries;
> > +
> > ret = save_stack_trace_tsk_reliable(task, &trace);
> > - WARN_ON_ONCE(ret == -ENOSYS);
> > + if (ret == -ENOSYS) {
> > + if (!enosys_warned) {
> > + printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > + __func__);
> > + enosys_warned = 1;
>
> ... abusing the fact that you are also printk maintainer :) ... looking at
> the above, wouldn't it make sense to introduce generic
> printk_deferred_once() instead?
Yeah, I thought about it. Also pr_debug_deferred() would be useful for
the other messages passed via err_buf.
Sigh, printk_deferred() is whack-a-mole approach. We use it because
we do not have anything better for the deadlocks. I am a bit scared
to add more wrappers because it might encourage people to use it
more widely, e.g. to avoid softlockups or secure fast paths.
On the other hand, it still looks better to find all occurrences
easily instead of hiding them into a custom workarounds.
OK, I am going to prepare new patchset and involve printk
people into it.
Best Regards,
Petr
On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/stacktrace.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index f8edee9c792d..83ac0ac5ffd9 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -74,6 +74,5 @@ __weak int
> save_stack_trace_tsk_reliable(struct task_struct *tsk,
> struct stack_trace *trace)
> {
> - WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
> return -ENOSYS;
> }
> --
> 2.16.4
>
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Wed, 24 Apr 2019, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
>
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
>
> + The livepatch does not change the semantic of the code.
> + Callbacks do not depend on a safely finished transition.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
Miroslav
Adding Thomas because this might slightly conflict with some of the
stacktrace.c improvements he's working on.
On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/stacktrace.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index f8edee9c792d..83ac0ac5ffd9 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -74,6 +74,5 @@ __weak int
> save_stack_trace_tsk_reliable(struct task_struct *tsk,
> struct stack_trace *trace)
> {
> - WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
> return -ENOSYS;
> }
> --
> 2.16.4
>
--
Josh
On Wed, Apr 24, 2019 at 10:55:50AM +0200, Petr Mladek wrote:
> WARN_ON_ONCE() could not be called safely under rq lock because
> of console deadlock issues. Fortunately, simple printk_deferred()
> is enough because the warning is printed from a well defined
> location and context.
>
> Also klp_try_switch_task() is called under klp_mutex.
> Therefore, the buffer for debug messages could be static.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/transition.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 9c89ae8b337a..e8183d18227f 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> static int klp_check_stack(struct task_struct *task, char *err_buf)
> {
> static unsigned long entries[MAX_STACK_ENTRIES];
> + static int enosys_warned;
> struct stack_trace trace;
> struct klp_object *obj;
> struct klp_func *func;
> @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> trace.nr_entries = 0;
> trace.max_entries = MAX_STACK_ENTRIES;
> trace.entries = entries;
> +
> ret = save_stack_trace_tsk_reliable(task, &trace);
> - WARN_ON_ONCE(ret == -ENOSYS);
> + if (ret == -ENOSYS) {
> + if (!enosys_warned) {
> + printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> + __func__);
> + enosys_warned = 1;
> + }
> + return ret;
> + }
We already have a similar printk in patch 1, so is this warning really
needed?
--
Josh
On Wed, Apr 24, 2019 at 10:55:48AM +0200, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
>
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
>
> + The livepatch does not change the semantic of the code.
> + Callbacks do not depend on a safely finished transition.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..14f33ab6c583 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1003,11 +1003,10 @@ int klp_enable_patch(struct klp_patch *patch)
> return -ENODEV;
>
> if (!klp_have_reliable_stack()) {
> - pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> - return -EOPNOTSUPP;
> + pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> + pr_warn("The livepatch transition may never complete.\n");
> }
>
> -
> mutex_lock(&klp_mutex);
>
> ret = klp_init_patch_early(patch);
> --
> 2.16.4
>
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Wed, 24 Apr 2019, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
>
> Signed-off-by: Petr Mladek <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
Miroslav
On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote:
> WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous.
> The only current user klp_check_stack() writes its own warning when
> -ENOSYS is returned.
>
> Signed-off-by: Petr Mladek <[email protected]>
Reviewed-by: Kamalesh Babulal <[email protected]>
--
Kamalesh
On Wed, Apr 24, 2019 at 10:55:48AM +0200, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
>
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
>
> + The livepatch does not change the semantic of the code.
> + Callbacks do not depend on a safely finished transition.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
Reviewed-by: Kamalesh Babulal <[email protected]>
--
Kamalesh
On Wed, Apr 24, 2019 at 08:48:58PM +0200, Miroslav Benes wrote:
> On Wed, 24 Apr 2019, Josh Poimboeuf wrote:
>
[...]
> > > ret = save_stack_trace_tsk_reliable(task, &trace);
> > > - WARN_ON_ONCE(ret == -ENOSYS);
> > > + if (ret == -ENOSYS) {
> > > + if (!enosys_warned) {
> > > + printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > > + __func__);
> > > + enosys_warned = 1;
> > > + }
> > > + return ret;
> > > + }
> >
> > We already have a similar printk in patch 1, so is this warning really
> > needed?
>
> I don't think so. pr_warn() in klp_enable_patch() should be enough in my
> opinion.
>
> However,
>
> if (ret == -ENOSYS)
> return ret;
>
> would be justified, wouldn't it?
>
Probably an one line comment on why we return, will be helpful.
--
Kamalesh
On Wed, 24 Apr 2019, Josh Poimboeuf wrote:
> On Wed, Apr 24, 2019 at 10:55:50AM +0200, Petr Mladek wrote:
> > WARN_ON_ONCE() could not be called safely under rq lock because
> > of console deadlock issues. Fortunately, simple printk_deferred()
> > is enough because the warning is printed from a well defined
> > location and context.
> >
> > Also klp_try_switch_task() is called under klp_mutex.
> > Therefore, the buffer for debug messages could be static.
> >
> > Signed-off-by: Petr Mladek <[email protected]>
> > ---
> > kernel/livepatch/transition.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 9c89ae8b337a..e8183d18227f 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -254,6 +254,7 @@ static int klp_check_stack_func(struct klp_func *func,
> > static int klp_check_stack(struct task_struct *task, char *err_buf)
> > {
> > static unsigned long entries[MAX_STACK_ENTRIES];
> > + static int enosys_warned;
> > struct stack_trace trace;
> > struct klp_object *obj;
> > struct klp_func *func;
> > @@ -263,8 +264,16 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
> > trace.nr_entries = 0;
> > trace.max_entries = MAX_STACK_ENTRIES;
> > trace.entries = entries;
> > +
> > ret = save_stack_trace_tsk_reliable(task, &trace);
> > - WARN_ON_ONCE(ret == -ENOSYS);
> > + if (ret == -ENOSYS) {
> > + if (!enosys_warned) {
> > + printk_deferred(KERN_WARNING "%s: save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > + __func__);
> > + enosys_warned = 1;
> > + }
> > + return ret;
> > + }
>
> We already have a similar printk in patch 1, so is this warning really
> needed?
I don't think so. pr_warn() in klp_enable_patch() should be enough in my
opinion.
However,
if (ret == -ENOSYS)
return ret;
would be justified, wouldn't it?
Miroslav
On Wed 2019-04-24 10:55:48, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
>
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
>
> + The livepatch does not change the semantic of the code.
> + Callbacks do not depend on a safely finished transition.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> kernel/livepatch/core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..14f33ab6c583 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1003,11 +1003,10 @@ int klp_enable_patch(struct klp_patch *patch)
> return -ENODEV;
>
> if (!klp_have_reliable_stack()) {
> - pr_err("This architecture doesn't have support for the livepatch consistency model.\n");
> - return -EOPNOTSUPP;
> + pr_warn("This architecture doesn't have support for the livepatch consistency model.\n");
> + pr_warn("The livepatch transition may never complete.\n");
> }
>
> -
> mutex_lock(&klp_mutex);
>
> ret = klp_init_patch_early(patch);
> --
> 2.16.4
I have committed this patch into for-5.2/core branch.
Best Regards,
Petr
PS: I am going to resend 2nd patch separately with more people
interested into kernel/stacktrace.c.
Also I am going to send two separate patches instead of
the 3rd one (complete warning removal, static err_buf).