[PATCH v4 0/3] livepatch: Cleanup of reliable stacktrace warnings

[PATCH v4 0/3] livepatch: Cleanup of reliable stacktrace warnings

[Miroslav Benes]

This is the fourth attempt to improve the situation of reliable stack
trace warnings in livepatch. Based on discussion in
20190531074147.27616-1-pmladek@suse.com (v3).

Changes against v3:
+ weak save_stack_trace_tsk_reliable() removed, because it is not needed
  anymore thanks to Thomas' recent improvements
+ klp_have_reliable_stack() check reintroduced in klp_try_switch_task()

Changes against v2:

+ Put back the patch removing WARN_ONCE in the weak
  save_stack_trace_tsk_reliable(). It is related.
+ Simplified patch removing the duplicate warning from klp_check_stack()
+ Update commit message for 3rd patch [Josh]

Miroslav Benes (2):
  stacktrace: Remove weak version of save_stack_trace_tsk_reliable()
  Revert "livepatch: Remove reliable stacktrace check in
    klp_try_switch_task()"

Petr Mladek (1):
  livepatch: Remove duplicate warning about missing reliable stacktrace
    support

 kernel/livepatch/transition.c | 8 +++++++-
 kernel/stacktrace.c           | 8 --------
 2 files changed, 7 insertions(+), 9 deletions(-)

-- 
2.21.0

[PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Miroslav Benes]

Recent rework of stack trace infrastructure introduced a new set of
helpers for common stack trace operations (commit e9b98e162aa5
("stacktrace: Provide helpers for common stack trace operations") and
related). As a result, save_stack_trace_tsk_reliable() is not directly
called anywhere. Livepatch, currently the only user of the reliable
stack trace feature, now calls stack_trace_save_tsk_reliable().

When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
x86_64 defines the former, ppc64le the latter. All other architectures
do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
defines -ENOSYS returning version for them.

In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
include/linux/stacktrace.h serves the same purpose as the old weak
version of save_stack_trace_tsk_reliable() which is therefore no longer
needed.

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/stacktrace.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 36139de0a3c4..0c3f00db9069 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -255,14 +255,6 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 	WARN_ONCE(1, KERN_INFO "save_stack_trace_regs() not implemented yet.\n");
 }
 
-__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;
-}
-
 /**
  * stack_trace_save - Save a stack trace into a storage array
  * @store:	Pointer to storage array
-- 
2.21.0

Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Petr Mladek]

On Tue 2019-06-11 16:13:18, Miroslav Benes wrote:
> Recent rework of stack trace infrastructure introduced a new set of
> helpers for common stack trace operations (commit e9b98e162aa5
> ("stacktrace: Provide helpers for common stack trace operations") and
> related). As a result, save_stack_trace_tsk_reliable() is not directly
> called anywhere. Livepatch, currently the only user of the reliable
> stack trace feature, now calls stack_trace_save_tsk_reliable().
> 
> When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
> CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
> arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
> x86_64 defines the former, ppc64le the latter. All other architectures
> do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
> defines -ENOSYS returning version for them.
> 
> In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
> include/linux/stacktrace.h serves the same purpose as the old weak
> version of save_stack_trace_tsk_reliable() which is therefore no longer
> needed.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

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

Best Regards,
Petr

Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Kamalesh Babulal]

On Tue, Jun 11, 2019 at 04:13:18PM +0200, Miroslav Benes wrote:
> Recent rework of stack trace infrastructure introduced a new set of
> helpers for common stack trace operations (commit e9b98e162aa5
> ("stacktrace: Provide helpers for common stack trace operations") and
> related). As a result, save_stack_trace_tsk_reliable() is not directly
> called anywhere. Livepatch, currently the only user of the reliable
> stack trace feature, now calls stack_trace_save_tsk_reliable().
> 
> When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
> CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
> arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
> x86_64 defines the former, ppc64le the latter. All other architectures
> do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
> defines -ENOSYS returning version for them.
> 
> In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
> include/linux/stacktrace.h serves the same purpose as the old weak
> version of save_stack_trace_tsk_reliable() which is therefore no longer
> needed.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Thomas Gleixner]

On Tue, 11 Jun 2019, Miroslav Benes wrote:

> Recent rework of stack trace infrastructure introduced a new set of
> helpers for common stack trace operations (commit e9b98e162aa5
> ("stacktrace: Provide helpers for common stack trace operations") and
> related). As a result, save_stack_trace_tsk_reliable() is not directly
> called anywhere. Livepatch, currently the only user of the reliable
> stack trace feature, now calls stack_trace_save_tsk_reliable().
> 
> When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
> CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
> arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
> x86_64 defines the former, ppc64le the latter. All other architectures
> do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
> defines -ENOSYS returning version for them.
> 
> In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
> include/linux/stacktrace.h serves the same purpose as the old weak
> version of save_stack_trace_tsk_reliable() which is therefore no longer
> needed.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Petr Mladek]

Hi Thomas,

On Sun 2019-06-16 10:44:59, Thomas Gleixner wrote:
> On Tue, 11 Jun 2019, Miroslav Benes wrote:
> 
> > Recent rework of stack trace infrastructure introduced a new set of
> > helpers for common stack trace operations (commit e9b98e162aa5
> > ("stacktrace: Provide helpers for common stack trace operations") and
> > related). As a result, save_stack_trace_tsk_reliable() is not directly
> > called anywhere. Livepatch, currently the only user of the reliable
> > stack trace feature, now calls stack_trace_save_tsk_reliable().
> > 
> > When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
> > CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
> > arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
> > x86_64 defines the former, ppc64le the latter. All other architectures
> > do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
> > defines -ENOSYS returning version for them.
> > 
> > In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
> > include/linux/stacktrace.h serves the same purpose as the old weak
> > version of save_stack_trace_tsk_reliable() which is therefore no longer
> > needed.
> > 
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Would you like to push this patch via your tree?
Or is it OK to push it via the livepatch tree for 5.3?

Best Regards,
Petr

Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()

[Thomas Gleixner]

On Mon, 17 Jun 2019, Petr Mladek wrote:
> On Sun 2019-06-16 10:44:59, Thomas Gleixner wrote:
> > On Tue, 11 Jun 2019, Miroslav Benes wrote:
> > 
> > > Recent rework of stack trace infrastructure introduced a new set of
> > > helpers for common stack trace operations (commit e9b98e162aa5
> > > ("stacktrace: Provide helpers for common stack trace operations") and
> > > related). As a result, save_stack_trace_tsk_reliable() is not directly
> > > called anywhere. Livepatch, currently the only user of the reliable
> > > stack trace feature, now calls stack_trace_save_tsk_reliable().
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > 
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Would you like to push this patch via your tree?
> Or is it OK to push it via the livepatch tree for 5.3?

Just pick it up. I don't think we have anything conflicting.

Thanks,

	tglx

[PATCH v4 2/3] Revert "livepatch: Remove reliable stacktrace check in klp_try_switch_task()"

[Miroslav Benes]

This reverts commit 1d98a69e5cef3aeb68bcefab0e67e342d6bb4dad. Commit
31adf2308f33 ("livepatch: Convert error about unsupported reliable
stacktrace into a warning") weakened the enforcement for architectures
to have reliable stack traces support. The system only warns now about
it.

It only makes sense to reintroduce the compile time checking in
klp_try_switch_task() again and bail out early.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/transition.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 8e8c79d4b204..7e7ef04689d1 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -293,6 +293,13 @@ static bool klp_try_switch_task(struct task_struct *task)
 	if (task->patch_state == klp_target_state)
 		return true;
 
+	/*
+	 * For arches which don't have reliable stack traces, we have to rely
+	 * on other methods (e.g., switching tasks at kernel exit).
+	 */
+	if (!klp_have_reliable_stack())
+		return false;
+
 	/*
 	 * Now try to check the stack for any to-be-patched or to-be-unpatched
 	 * functions.  If all goes well, switch the task to the target patch
-- 
2.21.0

Re: [PATCH v4 2/3] Revert "livepatch: Remove reliable stacktrace check in klp_try_switch_task()"

[Petr Mladek]

On Tue 2019-06-11 16:13:19, Miroslav Benes wrote:
> This reverts commit 1d98a69e5cef3aeb68bcefab0e67e342d6bb4dad. Commit
> 31adf2308f33 ("livepatch: Convert error about unsupported reliable
> stacktrace into a warning") weakened the enforcement for architectures
> to have reliable stack traces support. The system only warns now about
> it.
> 
> It only makes sense to reintroduce the compile time checking in
> klp_try_switch_task() again and bail out early.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

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

Best Regards,
Petr

Re: [PATCH v4 2/3] Revert "livepatch: Remove reliable stacktrace check in klp_try_switch_task()"

[Kamalesh Babulal]

On Tue, Jun 11, 2019 at 04:13:19PM +0200, Miroslav Benes wrote:
> This reverts commit 1d98a69e5cef3aeb68bcefab0e67e342d6bb4dad. Commit
> 31adf2308f33 ("livepatch: Convert error about unsupported reliable
> stacktrace into a warning") weakened the enforcement for architectures
> to have reliable stack traces support. The system only warns now about
> it.
> 
> It only makes sense to reintroduce the compile time checking in
> klp_try_switch_task() again and bail out early.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

[PATCH v4 3/3] livepatch: Remove duplicate warning about missing reliable stacktrace support

[Miroslav Benes]

From: Petr Mladek <pmladek@suse.com>

WARN_ON_ONCE() could not be called safely under rq lock because
of console deadlock issues. Moreover WARN_ON_ONCE() is superfluous in
klp_check_stack(), because stack_trace_save_tsk_reliable() cannot return
-ENOSYS thanks to klp_have_reliable_stack() check in
klp_try_switch_task().

Signed-off-by: Petr Mladek <pmladek@suse.com>
[ mbenes: changelog edited ]
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/transition.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7e7ef04689d1..cdf318d86dd6 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf)
 	int ret, nr_entries;
 
 	ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries));
-	WARN_ON_ONCE(ret == -ENOSYS);
 	if (ret < 0) {
 		snprintf(err_buf, STACK_ERR_BUF_SIZE,
 			 "%s: %s:%d has an unreliable stack\n",
-- 
2.21.0

Re: [PATCH v4 3/3] livepatch: Remove duplicate warning about missing reliable stacktrace support

[Kamalesh Babulal]

On Tue, Jun 11, 2019 at 04:13:20PM +0200, Miroslav Benes wrote:
> From: Petr Mladek <pmladek@suse.com>
> 
> WARN_ON_ONCE() could not be called safely under rq lock because
> of console deadlock issues. Moreover WARN_ON_ONCE() is superfluous in
> klp_check_stack(), because stack_trace_save_tsk_reliable() cannot return
> -ENOSYS thanks to klp_have_reliable_stack() check in
> klp_try_switch_task().
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> [ mbenes: changelog edited ]
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Re: [PATCH v4 0/3] livepatch: Cleanup of reliable stacktrace warnings

[Josh Poimboeuf]

On Tue, Jun 11, 2019 at 04:13:17PM +0200, Miroslav Benes wrote:
> This is the fourth attempt to improve the situation of reliable stack
> trace warnings in livepatch. Based on discussion in
> 20190531074147.27616-1-pmladek@suse.com (v3).
> 
> Changes against v3:
> + weak save_stack_trace_tsk_reliable() removed, because it is not needed
>   anymore thanks to Thomas' recent improvements
> + klp_have_reliable_stack() check reintroduced in klp_try_switch_task()
> 
> Changes against v2:
> 
> + Put back the patch removing WARN_ONCE in the weak
>   save_stack_trace_tsk_reliable(). It is related.
> + Simplified patch removing the duplicate warning from klp_check_stack()
> + Update commit message for 3rd patch [Josh]
> 
> Miroslav Benes (2):
>   stacktrace: Remove weak version of save_stack_trace_tsk_reliable()
>   Revert "livepatch: Remove reliable stacktrace check in
>     klp_try_switch_task()"
> 
> Petr Mladek (1):
>   livepatch: Remove duplicate warning about missing reliable stacktrace
>     support
> 
>  kernel/livepatch/transition.c | 8 +++++++-
>  kernel/stacktrace.c           | 8 --------
>  2 files changed, 7 insertions(+), 9 deletions(-)

Thanks Miroslav for wrapping this up, and thanks to Petr for his
previous work on this.

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v4 0/3] livepatch: Cleanup of reliable stacktrace warnings

[Petr Mladek]

On Sat 2019-06-15 15:43:20, Josh Poimboeuf wrote:
> On Tue, Jun 11, 2019 at 04:13:17PM +0200, Miroslav Benes wrote:
> > This is the fourth attempt to improve the situation of reliable stack
> > trace warnings in livepatch. Based on discussion in
> > 20190531074147.27616-1-pmladek@suse.com (v3).
> > 
> > Changes against v3:
> > + weak save_stack_trace_tsk_reliable() removed, because it is not needed
> >   anymore thanks to Thomas' recent improvements
> > + klp_have_reliable_stack() check reintroduced in klp_try_switch_task()
> > 
> > Changes against v2:
> > 
> > + Put back the patch removing WARN_ONCE in the weak
> >   save_stack_trace_tsk_reliable(). It is related.
> > + Simplified patch removing the duplicate warning from klp_check_stack()
> > + Update commit message for 3rd patch [Josh]
> > 
> > Miroslav Benes (2):
> >   stacktrace: Remove weak version of save_stack_trace_tsk_reliable()
> >   Revert "livepatch: Remove reliable stacktrace check in
> >     klp_try_switch_task()"
> > 
> > Petr Mladek (1):
> >   livepatch: Remove duplicate warning about missing reliable stacktrace
> >     support
> > 
> >  kernel/livepatch/transition.c | 8 +++++++-
> >  kernel/stacktrace.c           | 8 --------
> >  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> Thanks Miroslav for wrapping this up, and thanks to Petr for his
> previous work on this.
> 
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

All three patches have been commited into for-5.3/core branch.

Best Regards,
Petr