* Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
2019-05-17 18:51 [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Joe Lawrence
@ 2019-05-17 18:56 ` Joe Lawrence
2019-05-17 19:10 ` Josh Poimboeuf
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Joe Lawrence @ 2019-05-17 18:56 UTC (permalink / raw)
To: live-patching, linux-kernel; +Cc: jikos, jpoimboe, pmladek, tglx
On Fri, May 17, 2019 at 02:51:17PM -0400, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> kernel/stacktrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index 27bafc1e271e..90d3e0bf0302 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -206,7 +206,7 @@ int stack_trace_save_tsk_reliable(struct task_struct *tsk, unsigned long *store,
>
> ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
> put_task_stack(tsk);
> - return ret;
> + return ret ? ret : c.len;
> }
> #endif
>
> --
> 2.20.1
>
Hi Thomas,
This change is *very* lightly tested. It passes the livepatch
self-tests and a test driver that I wrote to debug this. If a more
substantial change is needed, feel free to grab this one.
FWIW, here's the debugging module that I used to first verify that the
return code was always 0 (ie, no nr_entries) and then that I was getting
sane values back from the modified version. It's simple, echo in a task
pointer and it will dump the stack trace to dmesg:
% dmesg -C
% echo 0xffff8a4107082f40 > /sys/module/checkstack/parameters/task_param
% dmesg
[ 1909.546463] checkstack: task @ ffff8a4107082f40
[ 1909.549280] checkstack: nr_entries = 7
[ 1909.552268] checkstack: [ffffffffa608fb29] schedule+0x29/0x90
[ 1909.555583] checkstack: [ffffffffa60941ed] schedule_hrtimeout_range_clock+0x18d/0x1a0
[ 1909.556864] checkstack: [ffffffffa5b27e38] ep_poll+0x428/0x450
[ 1909.557645] checkstack: [ffffffffa5b27f10] do_epoll_wait+0xb0/0xd0
[ 1909.558454] checkstack: [ffffffffa5b27f4a] __x64_sys_epoll_wait+0x1a/0x20
[ 1909.559354] checkstack: [ffffffffa58041e5] do_syscall_64+0x55/0x1a0
[ 1909.560233] checkstack: [ffffffffa620008c] entry_SYSCALL_64_after_hwframe+0x44/0xa9
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/cpu.h>
#include <linux/kallsyms.h>
#include <linux/stacktrace.h>
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Joe Lawrence <joe.lawrence@stratus.com>");
#define MAX_STACK_ENTRIES 100
/* No exports, no fun */
static int (*p_stack_trace_save_tsk_reliable)(struct task_struct *tsk, unsigned long *store, unsigned int size);
int checkstack(struct task_struct *task)
{
static unsigned long entries[MAX_STACK_ENTRIES];
unsigned long address;
int ret, nr_entries, i;
if (!task)
return 0;
pr_info("task @ %lx\n", (unsigned long) task);
ret = p_stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries));
WARN_ON_ONCE(ret == -ENOSYS);
if (ret < 0) {
pr_err("%s: %s:%d has an unreliable stack\n",
__func__, task->comm, task->pid);
return ret;
}
nr_entries = ret;
pr_info("nr_entries = %d\n", nr_entries);
for (i = 0; i < nr_entries; i++) {
address = entries[i];
pr_info("\t[%lx] %pS\n", address, (void *) address);
}
return 0;
}
static unsigned long task_param = 0;
static int task_param_set(const char *val, const struct kernel_param *kp)
{
int ret;
ret = param_set_ulong(val, kp);
if (ret < 0)
return ret;
return checkstack((struct task_struct *)task_param);
}
static const struct kernel_param_ops task_param_ops = {
.set = task_param_set,
.get = param_get_ulong,
};
module_param_cb(task_param, &task_param_ops, &task_param, 0644);
MODULE_PARM_DESC(task_param, "task (default=0)");
int __init init_module(void)
{
p_stack_trace_save_tsk_reliable =
(void *)kallsyms_lookup_name("stack_trace_save_tsk_reliable");
if (!p_stack_trace_save_tsk_reliable) {
pr_err("can't find stack_trace_save_tsk_reliable symbol\n");
return -ENXIO;
}
pr_info("p_stack_trace_save_tsk_reliable @ %lx\n",
(unsigned long) p_stack_trace_save_tsk_reliable);
return 0;
}
void cleanup_module(void)
{
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
2019-05-17 18:51 [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Joe Lawrence
2019-05-17 18:56 ` Joe Lawrence
@ 2019-05-17 19:10 ` Josh Poimboeuf
2019-05-18 13:52 ` Kamalesh Babulal
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Josh Poimboeuf @ 2019-05-17 19:10 UTC (permalink / raw)
To: Joe Lawrence; +Cc: live-patching, linux-kernel, jikos, pmladek, tglx
On Fri, May 17, 2019 at 02:51:17PM -0400, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
It's great to see the livepatch selftests working and finding
regressions.
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
--
Josh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
2019-05-17 18:51 [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Joe Lawrence
2019-05-17 18:56 ` Joe Lawrence
2019-05-17 19:10 ` Josh Poimboeuf
@ 2019-05-18 13:52 ` Kamalesh Babulal
2019-05-19 9:46 ` [tip:core/urgent] stacktrace: Unbreak stack_trace_save_tsk_reliable() tip-bot for Joe Lawrence
2019-05-28 22:25 ` [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Jiri Kosina
4 siblings, 0 replies; 6+ messages in thread
From: Kamalesh Babulal @ 2019-05-18 13:52 UTC (permalink / raw)
To: Joe Lawrence; +Cc: live-patching, linux-kernel, jikos, jpoimboe, pmladek, tglx
On Fri, May 17, 2019 at 02:51:17PM -0400, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* [tip:core/urgent] stacktrace: Unbreak stack_trace_save_tsk_reliable()
2019-05-17 18:51 [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Joe Lawrence
` (2 preceding siblings ...)
2019-05-18 13:52 ` Kamalesh Babulal
@ 2019-05-19 9:46 ` tip-bot for Joe Lawrence
2019-05-28 22:25 ` [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Jiri Kosina
4 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Joe Lawrence @ 2019-05-19 9:46 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, tglx, kamalesh, linux-kernel, joe.lawrence, hpa, jpoimboe,
mbenes
Commit-ID: 7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Gitweb: https://git.kernel.org/tip/7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Author: Joe Lawrence <joe.lawrence@redhat.com>
AuthorDate: Fri, 17 May 2019 14:51:17 -0400
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 19 May 2019 11:43:22 +0200
stacktrace: Unbreak stack_trace_save_tsk_reliable()
Miroslav reported that the livepatch self-tests were failing, specifically
a case in which the consistency model ensures that a current executing
function is not allowed to be patched, "TEST: busy target module".
Recent renovations of stack_trace_save_tsk_reliable() left it returning
only an -ERRNO success indication in some configuration combinations:
klp_check_stack()
ret = stack_trace_save_tsk_reliable()
#ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
stack_trace_save_tsk_reliable()
ret = arch_stack_walk_reliable()
return 0
return -EINVAL
...
return ret;
...
if (ret < 0)
/* stack_trace_save_tsk_reliable error */
nr_entries = ret; << 0
Previously (and currently for !CONFIG_ARCH_STACKWALK &&
CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable() returned
the number of entries that it consumed in the passed storage array.
In the case of the above config and trace, be sure to return the
stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
Reported-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: live-patching@vger.kernel.org
Cc: jikos@kernel.org
Cc: pmladek@suse.com
Link: https://lkml.kernel.org/r/20190517185117.24642-1-joe.lawrence@redhat.com
---
kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 27bafc1e271e..90d3e0bf0302 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -206,7 +206,7 @@ int stack_trace_save_tsk_reliable(struct task_struct *tsk, unsigned long *store,
ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
put_task_stack(tsk);
- return ret;
+ return ret ? ret : c.len;
}
#endif
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
2019-05-17 18:51 [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return Joe Lawrence
` (3 preceding siblings ...)
2019-05-19 9:46 ` [tip:core/urgent] stacktrace: Unbreak stack_trace_save_tsk_reliable() tip-bot for Joe Lawrence
@ 2019-05-28 22:25 ` Jiri Kosina
4 siblings, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2019-05-28 22:25 UTC (permalink / raw)
To: Joe Lawrence; +Cc: live-patching, linux-kernel, jpoimboe, pmladek, tglx
On Fri, 17 May 2019, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Tested-by: Jiri Kosina <jkosina@suse.cz>
Reviewed-by: Jiri Kosina <jkosina@suse.cz>
IMHO this should go in ASAP. Sorry for the delay,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread