With proper exception boundary detection, it is possible to implment
arch_stack_walk_reliable without sframe.
Note that, arch_stack_walk_reliable does not guarantee getting reliable
stack in all scenarios. Instead, it can reliably detect when the stack
trace is not reliable, which is enough to provide reliable livepatching.
This version has been inspired by Weinan Liu's patch [1].
[1] https://lore.kernel.org/live-patching/20250127213310.2496133-7-wnliu@google.com/
Signed-off-by: Song Liu <song@kernel.org>
---
arch/arm64/Kconfig | 2 +-
arch/arm64/include/asm/stacktrace/common.h | 1 +
arch/arm64/kernel/stacktrace.c | 44 +++++++++++++++++++++-
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 940343beb3d4..ed4f7bf4a879 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -275,6 +275,7 @@ config ARM64
select HAVE_SOFTIRQ_ON_OWN_STACK
select USER_STACKTRACE_SUPPORT
select VDSO_GETRANDOM
+ select HAVE_RELIABLE_STACKTRACE
help
ARM 64-bit (AArch64) Linux support.
@@ -2499,4 +2500,3 @@ endmenu # "CPU Power Management"
source "drivers/acpi/Kconfig"
source "arch/arm64/kvm/Kconfig"
-
diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 821a8fdd31af..072469fd91b7 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -33,6 +33,7 @@ struct unwind_state {
struct stack_info stack;
struct stack_info *stacks;
int nr_stacks;
+ bool unreliable;
};
static inline struct stack_info stackinfo_get_unknown(void)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1d9d51d7627f..69d0567a0c38 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -230,8 +230,14 @@ kunwind_next_frame_record(struct kunwind_state *state)
new_fp = READ_ONCE(record->fp);
new_pc = READ_ONCE(record->lr);
- if (!new_fp && !new_pc)
+ if (!new_fp && !new_pc) {
+ /*
+ * Searching across exception boundaries. The stack is now
+ * unreliable.
+ */
+ state->common.unreliable = true;
return kunwind_next_frame_record_meta(state);
+ }
unwind_consume_stack(&state->common, info, fp, sizeof(*record));
@@ -347,6 +353,7 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
.common = {
.stacks = stacks,
.nr_stacks = ARRAY_SIZE(stacks),
+ .unreliable = false,
},
};
@@ -387,6 +394,41 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
}
+struct kunwind_reliable_consume_entry_data {
+ stack_trace_consume_fn consume_entry;
+ void *cookie;
+ bool unreliable;
+};
+
+static __always_inline bool
+arch_kunwind_reliable_consume_entry(const struct kunwind_state *state, void *cookie)
+{
+ struct kunwind_reliable_consume_entry_data *data = cookie;
+
+ if (state->common.unreliable) {
+ data->unreliable = true;
+ return false;
+ }
+ return data->consume_entry(data->cookie, state->common.pc);
+}
+
+noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
+ void *cookie, struct task_struct *task)
+{
+ struct kunwind_reliable_consume_entry_data data = {
+ .consume_entry = consume_entry,
+ .cookie = cookie,
+ .unreliable = false,
+ };
+
+ kunwind_stack_walk(arch_kunwind_reliable_consume_entry, &data, task, NULL);
+
+ if (data.unreliable)
+ return -EINVAL;
+
+ return 0;
+}
+
struct bpf_unwind_consume_entry_data {
bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
void *cookie;
--
2.43.5On Fri, Mar 07, 2025 at 05:27:41PM -0800, Song Liu wrote: > With proper exception boundary detection, it is possible to implment > arch_stack_walk_reliable without sframe. > > Note that, arch_stack_walk_reliable does not guarantee getting reliable > stack in all scenarios. Instead, it can reliably detect when the stack > trace is not reliable, which is enough to provide reliable livepatching. > > This version has been inspired by Weinan Liu's patch [1]. > > [1] https://lore.kernel.org/live-patching/20250127213310.2496133-7-wnliu@google.com/ > Signed-off-by: Song Liu <song@kernel.org> This looks incomplete. The reliable unwinder needs to be extra paranoid. There are several already-checked-for errors in the unwinder that don't actually set the unreliable bit. There are likely other failure modes it should also be checking for. For example I don't see where it confirms that the unwind completed to the end of the stack (which is typically at a certain offset). See for example all the error conditions in the x86 version of arch_stack_walk_reliable() and in arch/x86/kernel/unwind_frame.c. -- Josh
Hi Josh,
Thanks for the review!
On Tue, Mar 18, 2025 at 11:45 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, Mar 07, 2025 at 05:27:41PM -0800, Song Liu wrote:
> > With proper exception boundary detection, it is possible to implment
> > arch_stack_walk_reliable without sframe.
> >
> > Note that, arch_stack_walk_reliable does not guarantee getting reliable
> > stack in all scenarios. Instead, it can reliably detect when the stack
> > trace is not reliable, which is enough to provide reliable livepatching.
> >
> > This version has been inspired by Weinan Liu's patch [1].
> >
> > [1] https://lore.kernel.org/live-patching/20250127213310.2496133-7-wnliu@google.com/
> > Signed-off-by: Song Liu <song@kernel.org>
>
> This looks incomplete. The reliable unwinder needs to be extra
> paranoid. There are several already-checked-for errors in the unwinder
> that don't actually set the unreliable bit.
>
> There are likely other failure modes it should also be checking for.
> For example I don't see where it confirms that the unwind completed to
> the end of the stack (which is typically at a certain offset).
If I understand the comment correctly, this should be handled by the
meta data type FRAME_META_TYPE_FINAL.
>
> See for example all the error conditions in the x86 version of
> arch_stack_walk_reliable() and in arch/x86/kernel/unwind_frame.c.
I guess I missed this part:
diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
index 69d0567a0c38..3bb8e3ea4c4b 100644
--- i/arch/arm64/kernel/stacktrace.c
+++ w/arch/arm64/kernel/stacktrace.c
@@ -268,6 +268,8 @@ kunwind_next(struct kunwind_state *state)
case KUNWIND_SOURCE_TASK:
case KUNWIND_SOURCE_REGS_PC:
err = kunwind_next_frame_record(state);
+ if (err && err != -ENOENT)
+ state->common.unreliable = true;
break;
default:
err = -EINVAL;
With this part, we should cover all these cases? Did I miss something
else?
Thanks,
Song
On Tue, Mar 18, 2025 at 01:14:40PM -0700, Song Liu wrote:
> >
> > See for example all the error conditions in the x86 version of
> > arch_stack_walk_reliable() and in arch/x86/kernel/unwind_frame.c.
>
> I guess I missed this part:
>
> diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
> index 69d0567a0c38..3bb8e3ea4c4b 100644
> --- i/arch/arm64/kernel/stacktrace.c
> +++ w/arch/arm64/kernel/stacktrace.c
> @@ -268,6 +268,8 @@ kunwind_next(struct kunwind_state *state)
> case KUNWIND_SOURCE_TASK:
> case KUNWIND_SOURCE_REGS_PC:
> err = kunwind_next_frame_record(state);
> + if (err && err != -ENOENT)
> + state->common.unreliable = true;
> break;
> default:
> err = -EINVAL;
I still see some issues:
- do_kunwind() -> kunwind_recover_return_address() can fail
- do_kunwind() -> consume_state() can fail
- even in the -ENOENT case the unreliable bit has already been set
right before the call to kunwind_next_frame_record_meta().
--
Josh
On Tue, Mar 18, 2025 at 4:00 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Tue, Mar 18, 2025 at 01:14:40PM -0700, Song Liu wrote:
> > >
> > > See for example all the error conditions in the x86 version of
> > > arch_stack_walk_reliable() and in arch/x86/kernel/unwind_frame.c.
> >
> > I guess I missed this part:
> >
> > diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
> > index 69d0567a0c38..3bb8e3ea4c4b 100644
> > --- i/arch/arm64/kernel/stacktrace.c
> > +++ w/arch/arm64/kernel/stacktrace.c
> > @@ -268,6 +268,8 @@ kunwind_next(struct kunwind_state *state)
> > case KUNWIND_SOURCE_TASK:
> > case KUNWIND_SOURCE_REGS_PC:
> > err = kunwind_next_frame_record(state);
> > + if (err && err != -ENOENT)
> > + state->common.unreliable = true;
> > break;
> > default:
> > err = -EINVAL;
>
> I still see some issues:
>
> - do_kunwind() -> kunwind_recover_return_address() can fail
>
> - do_kunwind() -> consume_state() can fail
Great points! I have got the following:
diff --git i/arch/arm64/kernel/stacktrace.c w/arch/arm64/kernel/stacktrace.c
index 69d0567a0c38..852e4b322209 100644
--- i/arch/arm64/kernel/stacktrace.c
+++ w/arch/arm64/kernel/stacktrace.c
@@ -139,6 +139,7 @@ kunwind_recover_return_address(struct kunwind_state *state)
(void *)state->common.fp);
if (state->common.pc == orig_pc) {
WARN_ON_ONCE(state->task == current);
+ state->common.unreliable = true;
return -EINVAL;
}
state->common.pc = orig_pc;
@@ -268,6 +269,8 @@ kunwind_next(struct kunwind_state *state)
case KUNWIND_SOURCE_TASK:
case KUNWIND_SOURCE_REGS_PC:
err = kunwind_next_frame_record(state);
+ if (err && err != -ENOENT)
+ state->common.unreliable = true;
break;
default:
err = -EINVAL;
@@ -404,12 +407,16 @@ static __always_inline bool
arch_kunwind_reliable_consume_entry(const struct kunwind_state
*state, void *cookie)
{
struct kunwind_reliable_consume_entry_data *data = cookie;
+ bool ret;
if (state->common.unreliable) {
data->unreliable = true;
return false;
}
- return data->consume_entry(data->cookie, state->common.pc);
+ ret = data->consume_entry(data->cookie, state->common.pc);
+ if (!ret)
+ data->unreliable = true;
+ return ret;
}
noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn
consume_entry,
> - even in the -ENOENT case the unreliable bit has already been set
> right before the call to kunwind_next_frame_record_meta().
For this one, do you mean we set state->common.unreliable, but
failed to propagate it to data.unreliable?
Thanks,
Song
On Tue, Mar 18, 2025 at 04:38:20PM -0700, Song Liu wrote:
> On Tue, Mar 18, 2025 at 4:00 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > - even in the -ENOENT case the unreliable bit has already been set
> > right before the call to kunwind_next_frame_record_meta().
>
> For this one, do you mean we set state->common.unreliable, but
> failed to propagate it to data.unreliable?
Hm, I hadn't noticed that. That code is quite the maze.
It's unfortunate there are two separate 'unreliable' variables. It
looks like consume_state() is the only way they get synced?
How does that work if kunwind_next() returns an error and skips
consume_state()? Or if kunwind_recover_return_address() returns an
error to kunwind_next()?
What I actually meant was the following:
do_kunwind()
kunwind_next()
kunwind_next_frame_record()
state->common.unreliable = true;
kunwind_next_frame_record_meta()
return -ENOENT;
Notice that in the success case (-ENOENT), unreliable has already been
set.
Actually I think it would be much simpler to just propagate -ENOENT down
the call chain. Then no 'unreliable' bits needed.
Like so (instead of original patch):
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c9fe3e7566a6..5713fad567c5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -276,6 +276,7 @@ config ARM64
select HAVE_SOFTIRQ_ON_OWN_STACK
select USER_STACKTRACE_SUPPORT
select VDSO_GETRANDOM
+ select HAVE_RELIABLE_STACKTRACE
help
ARM 64-bit (AArch64) Linux support.
@@ -2509,4 +2510,3 @@ endmenu # "CPU Power Management"
source "drivers/acpi/Kconfig"
source "arch/arm64/kvm/Kconfig"
-
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1d9d51d7627f..e227da842bc3 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -277,22 +277,28 @@ kunwind_next(struct kunwind_state *state)
typedef bool (*kunwind_consume_fn)(const struct kunwind_state *state, void *cookie);
-static __always_inline void
+static __always_inline int
do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state,
void *cookie)
{
- if (kunwind_recover_return_address(state))
- return;
+ int ret;
+
+ ret = kunwind_recover_return_address(state);
+ if (ret)
+ return ret;
while (1) {
int ret;
if (!consume_state(state, cookie))
- break;
+ return -EINVAL;
+
ret = kunwind_next(state);
- if (ret < 0)
- break;
+ if (ret)
+ return ret;
}
+
+ return -EINVAL;
}
/*
@@ -324,7 +330,7 @@ do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state,
: stackinfo_get_unknown(); \
})
-static __always_inline void
+static __always_inline int
kunwind_stack_walk(kunwind_consume_fn consume_state,
void *cookie, struct task_struct *task,
struct pt_regs *regs)
@@ -352,7 +358,7 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
if (regs) {
if (task != current)
- return;
+ return -EINVAL;
kunwind_init_from_regs(&state, regs);
} else if (task == current) {
kunwind_init_from_caller(&state);
@@ -360,7 +366,7 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
kunwind_init_from_task(&state, task);
}
- do_kunwind(&state, consume_state, cookie);
+ return do_kunwind(&state, consume_state, cookie);
}
struct kunwind_consume_entry_data {
@@ -387,6 +393,25 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
}
+noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
+ void *cookie, struct task_struct *task)
+{
+ int ret;
+ struct kunwind_consume_entry_data data = {
+ .consume_entry = consume_entry,
+ .cookie = cookie,
+ };
+
+ ret = kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, NULL);
+ if (ret) {
+ if (ret == -ENOENT)
+ return 0;
+ return ret;
+ }
+
+ return -EINVAL;
+}
+
struct bpf_unwind_consume_entry_data {
bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
void *cookie;
On Tue, Mar 18, 2025 at 6:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Tue, Mar 18, 2025 at 04:38:20PM -0700, Song Liu wrote:
> > On Tue, Mar 18, 2025 at 4:00 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > - even in the -ENOENT case the unreliable bit has already been set
> > > right before the call to kunwind_next_frame_record_meta().
> >
> > For this one, do you mean we set state->common.unreliable, but
> > failed to propagate it to data.unreliable?
>
> Hm, I hadn't noticed that. That code is quite the maze.
>
> It's unfortunate there are two separate 'unreliable' variables. It
> looks like consume_state() is the only way they get synced?
>
> How does that work if kunwind_next() returns an error and skips
> consume_state()? Or if kunwind_recover_return_address() returns an
> error to kunwind_next()?
>
> What I actually meant was the following:
>
> do_kunwind()
> kunwind_next()
> kunwind_next_frame_record()
> state->common.unreliable = true;
> kunwind_next_frame_record_meta()
> return -ENOENT;
>
> Notice that in the success case (-ENOENT), unreliable has already been
> set.
>
> Actually I think it would be much simpler to just propagate -ENOENT down
> the call chain. Then no 'unreliable' bits needed.
Yeah, I was thinking about something like this. This is actually quite
similar to my original RFC version.
On a closer look, I think we also need some logic in unwind_find_stack()
so that we can see when the unwinder hits the exception boundary. For
this reason, we may still need unwind_state.unreliable. I will look into
fixing this and send v2.
Thanks,
Song
>
> Like so (instead of original patch):
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index c9fe3e7566a6..5713fad567c5 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -276,6 +276,7 @@ config ARM64
> select HAVE_SOFTIRQ_ON_OWN_STACK
> select USER_STACKTRACE_SUPPORT
> select VDSO_GETRANDOM
> + select HAVE_RELIABLE_STACKTRACE
> help
> ARM 64-bit (AArch64) Linux support.
>
> @@ -2509,4 +2510,3 @@ endmenu # "CPU Power Management"
> source "drivers/acpi/Kconfig"
>
> source "arch/arm64/kvm/Kconfig"
> -
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 1d9d51d7627f..e227da842bc3 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -277,22 +277,28 @@ kunwind_next(struct kunwind_state *state)
>
> typedef bool (*kunwind_consume_fn)(const struct kunwind_state *state, void *cookie);
>
> -static __always_inline void
> +static __always_inline int
> do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state,
> void *cookie)
> {
> - if (kunwind_recover_return_address(state))
> - return;
> + int ret;
> +
> + ret = kunwind_recover_return_address(state);
> + if (ret)
> + return ret;
>
> while (1) {
> int ret;
>
> if (!consume_state(state, cookie))
> - break;
> + return -EINVAL;
> +
> ret = kunwind_next(state);
> - if (ret < 0)
> - break;
> + if (ret)
> + return ret;
> }
> +
> + return -EINVAL;
> }
>
> /*
> @@ -324,7 +330,7 @@ do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state,
> : stackinfo_get_unknown(); \
> })
>
> -static __always_inline void
> +static __always_inline int
> kunwind_stack_walk(kunwind_consume_fn consume_state,
> void *cookie, struct task_struct *task,
> struct pt_regs *regs)
> @@ -352,7 +358,7 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
>
> if (regs) {
> if (task != current)
> - return;
> + return -EINVAL;
> kunwind_init_from_regs(&state, regs);
> } else if (task == current) {
> kunwind_init_from_caller(&state);
> @@ -360,7 +366,7 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
> kunwind_init_from_task(&state, task);
> }
>
> - do_kunwind(&state, consume_state, cookie);
> + return do_kunwind(&state, consume_state, cookie);
> }
>
> struct kunwind_consume_entry_data {
> @@ -387,6 +393,25 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
> kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
> }
>
> +noinline noinstr int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
> + void *cookie, struct task_struct *task)
> +{
> + int ret;
> + struct kunwind_consume_entry_data data = {
> + .consume_entry = consume_entry,
> + .cookie = cookie,
> + };
> +
> + ret = kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, NULL);
> + if (ret) {
> + if (ret == -ENOENT)
> + return 0;
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> struct bpf_unwind_consume_entry_data {
> bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
> void *cookie;
On Tue, Mar 18, 2025 at 08:58:52PM -0700, Song Liu wrote: > On a closer look, I think we also need some logic in unwind_find_stack() > so that we can see when the unwinder hits the exception boundary. For > this reason, we may still need unwind_state.unreliable. I will look into > fixing this and send v2. Isn't that what FRAME_META_TYPE_PT_REGS is for? Maybe it can just tell kunwind_stack_walk() to set a bit in kunwind_state which tells kunwind_next_frame_record_meta() to quit the unwind early for the FRAME_META_TYPE_PT_REGS case. That also has the benefit of stopping the unwind as soon as the exception is encounterd. -- Josh
On Tue, Mar 18, 2025 at 10:39 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Tue, Mar 18, 2025 at 08:58:52PM -0700, Song Liu wrote: > > On a closer look, I think we also need some logic in unwind_find_stack() > > so that we can see when the unwinder hits the exception boundary. For > > this reason, we may still need unwind_state.unreliable. I will look into > > fixing this and send v2. > > Isn't that what FRAME_META_TYPE_PT_REGS is for? > > Maybe it can just tell kunwind_stack_walk() to set a bit in > kunwind_state which tells kunwind_next_frame_record_meta() to quit the > unwind early for the FRAME_META_TYPE_PT_REGS case. That also has the > benefit of stopping the unwind as soon as the exception is encounterd. > After reviewing the code flow, it seems like we should treat all -EINVALID cases or `FRAME_META_TYPE_PT_REGS` cases as unreliable unwinds. Would a simplification like the one below work? Or we can return a special value for success cases in kunwind_next_regs_pc() ``` diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 69d0567a0c38..0eb69fa6161a 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -296,7 +296,8 @@ do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state, if (!consume_state(state, cookie)) break; ret = kunwind_next(state); - if (ret < 0) + if (ret < 0 || state->source == KUNWIND_SOURCE_REGS_PC) + state->common.unreliable = true; break; } } ``` -- Weinan
On Wed, Mar 19, 2025 at 11:38 AM Weinan Liu <wnliu@google.com> wrote: > > On Tue, Mar 18, 2025 at 10:39 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Tue, Mar 18, 2025 at 08:58:52PM -0700, Song Liu wrote: > > > On a closer look, I think we also need some logic in unwind_find_stack() > > > so that we can see when the unwinder hits the exception boundary. For > > > this reason, we may still need unwind_state.unreliable. I will look into > > > fixing this and send v2. > > > > Isn't that what FRAME_META_TYPE_PT_REGS is for? > > > > Maybe it can just tell kunwind_stack_walk() to set a bit in > > kunwind_state which tells kunwind_next_frame_record_meta() to quit the > > unwind early for the FRAME_META_TYPE_PT_REGS case. That also has the > > benefit of stopping the unwind as soon as the exception is encounterd. > > > > After reviewing the code flow, it seems like we should treat all -EINVALID > cases or `FRAME_META_TYPE_PT_REGS` cases as unreliable unwinds. Agreed with this: all -EINVALID cases or `FRAME_META_TYPE_PT_REGS` should be unreliable, IIUC. > > Would a simplification like the one below work? > Or we can return a special value for success cases in kunwind_next_regs_pc() > > ``` > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 69d0567a0c38..0eb69fa6161a 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -296,7 +296,8 @@ do_kunwind(struct kunwind_state *state, kunwind_consume_fn consume_state, > if (!consume_state(state, cookie)) > break; > ret = kunwind_next(state); > - if (ret < 0) > + if (ret < 0 || state->source == KUNWIND_SOURCE_REGS_PC) > + state->common.unreliable = true; I am current leaning toward not using common.unreliable. It seems to add unnecessary complexity here. But I may change my mind later on. Thanks, Song > break; > } > } > ``` > > -- > Weinan
On Fri, Mar 07, 2025 at 05:27:41PM -0800, Song Liu wrote: > With proper exception boundary detection, it is possible to implment > arch_stack_walk_reliable without sframe. > > Note that, arch_stack_walk_reliable does not guarantee getting reliable > stack in all scenarios. Instead, it can reliably detect when the stack > trace is not reliable, which is enough to provide reliable livepatching. > > This version has been inspired by Weinan Liu's patch [1]. > > [1] https://lore.kernel.org/live-patching/20250127213310.2496133-7-wnliu@google.com/ > Signed-off-by: Song Liu <song@kernel.org> Tested-by: Breno Leitao <leitao@debian.org> > arch/arm64/Kconfig | 2 +- > arch/arm64/include/asm/stacktrace/common.h | 1 + > arch/arm64/kernel/stacktrace.c | 44 +++++++++++++++++++++- > 3 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 940343beb3d4..ed4f7bf4a879 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -275,6 +275,7 @@ config ARM64 > select HAVE_SOFTIRQ_ON_OWN_STACK > select USER_STACKTRACE_SUPPORT > select VDSO_GETRANDOM > + select HAVE_RELIABLE_STACKTRACE Can we really mark this is reliable stacktrace? I am wondering if we need an intermediate state (potentially reliable stacktrace?) until we have a fully reliable stack unwinder. Thanks for working on it. --breno
On Thu, Mar 13, 2025 at 11:12 AM Breno Leitao <leitao@debian.org> wrote: > > On Fri, Mar 07, 2025 at 05:27:41PM -0800, Song Liu wrote: > > With proper exception boundary detection, it is possible to implment > > arch_stack_walk_reliable without sframe. > > > > Note that, arch_stack_walk_reliable does not guarantee getting reliable > > stack in all scenarios. Instead, it can reliably detect when the stack > > trace is not reliable, which is enough to provide reliable livepatching. > > > > This version has been inspired by Weinan Liu's patch [1]. > > > > [1] https://lore.kernel.org/live-patching/20250127213310.2496133-7-wnliu@google.com/ > > Signed-off-by: Song Liu <song@kernel.org> > > Tested-by: Breno Leitao <leitao@debian.org> Thanks for the testing! > > > arch/arm64/Kconfig | 2 +- > > arch/arm64/include/asm/stacktrace/common.h | 1 + > > arch/arm64/kernel/stacktrace.c | 44 +++++++++++++++++++++- > > 3 files changed, 45 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index 940343beb3d4..ed4f7bf4a879 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -275,6 +275,7 @@ config ARM64 > > select HAVE_SOFTIRQ_ON_OWN_STACK > > select USER_STACKTRACE_SUPPORT > > select VDSO_GETRANDOM > > + select HAVE_RELIABLE_STACKTRACE > > Can we really mark this is reliable stacktrace? I am wondering > if we need an intermediate state (potentially reliable stacktrace?) > until we have a fully reliable stack unwinder. AFAICT, we do not expect arch_stack_walk_reliable() to always return a reliable stack. Instead, it is expected to return -EINVAL if the stack trace is not reliable. OTOH, arch_stack_walk() doesn't warn the caller when the stack trace is not reliable. This is exactly what we need for live patch: we just need to make the patch transition when the stack trace is reliable and none of the functions in the stack is being patched. If the stack trace is not reliable, we will retry the transition at a later time. Thanks, Song
© 2016 - 2026 Red Hat, Inc.