LinuxLists.cc - [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk

2019-05-17 18:52:58

Subject: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return

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 <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
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

2019-05-17 19:35:33

by Josh Poimboeuf

[permalink] [raw]

Subject: Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return

2019-05-17 20:55:28

by Joe Lawrence

[permalink] [raw]

Subject: Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return

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 <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> 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 <[email protected]>");

#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)
{
}

2019-05-18 15:26:07

by Kamalesh Babulal

[permalink] [raw]

Subject: Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return

2019-05-19 17:23:28

by tip-bot for Vasyl Gomonovych

[permalink] [raw]

Subject: [tip:core/urgent] stacktrace: Unbreak stack_trace_save_tsk_reliable()

Commit-ID: 7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Gitweb: https://git.kernel.org/tip/7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Author: Joe Lawrence <[email protected]>
AuthorDate: Fri, 17 May 2019 14:51:17 -0400
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Kamalesh Babulal <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]

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

2019-05-28 23:10:00

by Jiri Kosina

[permalink] [raw]

Subject: Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return

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 <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>

Tested-by: Jiri Kosina <[email protected]>
Reviewed-by: Jiri Kosina <[email protected]>

IMHO this should go in ASAP. Sorry for the delay,

--
Jiri Kosina
SUSE Labs