From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-12 14:20:13
The unwinder reports idle tasks' stack on XEN PV as unreliable which
complicates things for at least live patching. The two patches in the
series try to amend that by using similar approach as non-XEN x86 does.
However, I did not come up with a nice solution for secondary CPUs idle
tasks. The patch just shows the idea what should be done but it is an
ugly hack. Ideas are more than welcome.
Miroslav Benes (2):
x86/xen: Make the boot CPU idle task reliable
x86/xen: Make the secondary CPU idle tasks reliable
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 14 +++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
--
2.25.1
From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-12 14:20:16
The unwinder reports the secondary CPU idle tasks' stack on XEN PV as
unreliable, which affects at least live patching.
cpu_initialize_context() sets up the context of the CPU through
VCPUOP_initialise hypercall. After it is woken up, the idle task starts
in cpu_bringup_and_idle() function and its stack starts at the offset
right below pt_regs. The unwinder correctly detects the end of stack
there but it is confused by NULL return address in the last frame.
RFC: I haven't found the way to teach the unwinder about the state of
the stack there. Thus the ugly hack using assembly. Similar to what
startup_xen() has got for boot CPU.
It introduces objtool "unreachable instruction" warning just right after
the jump to cpu_bringup_and_idle(). It should show the idea what needs
to be done though, I think. Ideas welcome.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-12 14:20:20
The unwinder reports the boot CPU idle task's stack on XEN PV as
unreliable, which affects at least live patching. There are two reasons
for this. First, the task does not follow the x86 convention that its
stack starts at the offset right below saved pt_regs. It allows the
unwinder to easily detect the end of the stack and verify it. Second,
startup_xen() function does not store the return address before jumping
to xen_start_kernel() which confuses the unwinder.
Amend both issues by moving the starting point of initial stack in
startup_xen() and storing the return address before the jump.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/xen-head.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Andrew Cooper <hidden> Date: 2020-03-12 15:11:50
On 12/03/2020 14:20, Miroslav Benes wrote:
quoted hunk
The unwinder reports the boot CPU idle task's stack on XEN PV as
unreliable, which affects at least live patching. There are two reasons
for this. First, the task does not follow the x86 convention that its
stack starts at the offset right below saved pt_regs. It allows the
unwinder to easily detect the end of the stack and verify it. Second,
startup_xen() function does not store the return address before jumping
to xen_start_kernel() which confuses the unwinder.
Amend both issues by moving the starting point of initial stack in
startup_xen() and storing the return address before the jump.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/xen-head.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-12 15:17:09
On Thu, 12 Mar 2020, Andrew Cooper wrote:
On 12/03/2020 14:20, Miroslav Benes wrote:
quoted
The unwinder reports the boot CPU idle task's stack on XEN PV as
unreliable, which affects at least live patching. There are two reasons
for this. First, the task does not follow the x86 convention that its
stack starts at the offset right below saved pt_regs. It allows the
unwinder to easily detect the end of the stack and verify it. Second,
startup_xen() function does not store the return address before jumping
to xen_start_kernel() which confuses the unwinder.
Amend both issues by moving the starting point of initial stack in
startup_xen() and storing the return address before the jump.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/xen-head.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Jürgen Groß <jgross@suse.com> Date: 2020-03-13 08:27:04
On 12.03.20 15:20, Miroslav Benes wrote:
quoted hunk
The unwinder reports the secondary CPU idle tasks' stack on XEN PV as
unreliable, which affects at least live patching.
cpu_initialize_context() sets up the context of the CPU through
VCPUOP_initialise hypercall. After it is woken up, the idle task starts
in cpu_bringup_and_idle() function and its stack starts at the offset
right below pt_regs. The unwinder correctly detects the end of stack
there but it is confused by NULL return address in the last frame.
RFC: I haven't found the way to teach the unwinder about the state of
the stack there. Thus the ugly hack using assembly. Similar to what
startup_xen() has got for boot CPU.
It introduces objtool "unreachable instruction" warning just right after
the jump to cpu_bringup_and_idle(). It should show the idea what needs
to be done though, I think. Ideas welcome.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-13 09:54:17
On Fri, 13 Mar 2020, Jürgen Groß wrote:
On 12.03.20 15:20, Miroslav Benes wrote:
quoted
The unwinder reports the secondary CPU idle tasks' stack on XEN PV as
unreliable, which affects at least live patching.
cpu_initialize_context() sets up the context of the CPU through
VCPUOP_initialise hypercall. After it is woken up, the idle task starts
in cpu_bringup_and_idle() function and its stack starts at the offset
right below pt_regs. The unwinder correctly detects the end of stack
there but it is confused by NULL return address in the last frame.
RFC: I haven't found the way to teach the unwinder about the state of
the stack there. Thus the ugly hack using assembly. Similar to what
startup_xen() has got for boot CPU.
It introduces objtool "unreachable instruction" warning just right after
the jump to cpu_bringup_and_idle(). It should show the idea what needs
to be done though, I think. Ideas welcome.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
Would adding this here work?
+ asm volatile (UNWIND_HINT(ORC_REG_UNDEFINED, 0, ORC_TYPE_CALL, 1));
I tried something similar. It did not work, because than the hint is
"bound" to the closest next call in the function which is cr4_init() in
this case. The unwinder would not take it into account.
In my case, I placed it at the beginning of cpu_bringup_and_idle(). I also
open coded it and played with the offset in the orc entry, but that did
not work for some other reason.
However, now I tried this
and that seems to work. I need to properly verify and test, but the
explanation is that as opposed to the above, cpu_startup_entry() is on the
idle task's stack and the hint is then taken into account. The unwound
stack seems to be complete, so it could indeed be the fix.
Thanks
Miroslav
From: Miroslav Benes <mbenes@suse.cz> Date: 2020-03-16 15:51:19
On Fri, 13 Mar 2020, Miroslav Benes wrote:
quoted hunk
On Fri, 13 Mar 2020, Jürgen Groß wrote:
quoted
On 12.03.20 15:20, Miroslav Benes wrote:
quoted
The unwinder reports the secondary CPU idle tasks' stack on XEN PV as
unreliable, which affects at least live patching.
cpu_initialize_context() sets up the context of the CPU through
VCPUOP_initialise hypercall. After it is woken up, the idle task starts
in cpu_bringup_and_idle() function and its stack starts at the offset
right below pt_regs. The unwinder correctly detects the end of stack
there but it is confused by NULL return address in the last frame.
RFC: I haven't found the way to teach the unwinder about the state of
the stack there. Thus the ugly hack using assembly. Similar to what
startup_xen() has got for boot CPU.
It introduces objtool "unreachable instruction" warning just right after
the jump to cpu_bringup_and_idle(). It should show the idea what needs
to be done though, I think. Ideas welcome.
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
Would adding this here work?
+ asm volatile (UNWIND_HINT(ORC_REG_UNDEFINED, 0, ORC_TYPE_CALL, 1));
I tried something similar. It did not work, because than the hint is
"bound" to the closest next call in the function which is cr4_init() in
this case. The unwinder would not take it into account.
In my case, I placed it at the beginning of cpu_bringup_and_idle(). I also
open coded it and played with the offset in the orc entry, but that did
not work for some other reason.
However, now I tried this
and that seems to work. I need to properly verify and test, but the
explanation is that as opposed to the above, cpu_startup_entry() is on the
idle task's stack and the hint is then taken into account. The unwound
stack seems to be complete, so it could indeed be the fix.
Not the correct one though. Objtool rightfully complains with
arch/x86/xen/smp_pv.o: warning: objtool: cpu_bringup_and_idle()+0x6a: undefined stack state
and all the other hacks I tried ended up in the same dead alley. It seems
to me the correct fix is that all orc entries for cpu_bringup_and_idle()
should have "end" property set to 1, since it is the first function on the
stack. I don't know how to achieve that without the assembly hack in the
patch I sent. If I am not missing something, of course.
Josh, any idea?
Thanks
Miroslav
and that seems to work. I need to properly verify and test, but the
explanation is that as opposed to the above, cpu_startup_entry() is on the
idle task's stack and the hint is then taken into account. The unwound
stack seems to be complete, so it could indeed be the fix.
Not the correct one though. Objtool rightfully complains with
arch/x86/xen/smp_pv.o: warning: objtool: cpu_bringup_and_idle()+0x6a: undefined stack state
and all the other hacks I tried ended up in the same dead alley. It seems
to me the correct fix is that all orc entries for cpu_bringup_and_idle()
should have "end" property set to 1, since it is the first function on the
stack. I don't know how to achieve that without the assembly hack in the
patch I sent. If I am not missing something, of course.
Josh, any idea?
Yeah, I think mucking with the unwind hints in C code is going to be
precarious. You could maybe have something like
asm("
UNWIND_HINT_EMPTY\n
mov $CPUHP_AP_ONLINE_IDLE, %rdi\n
call cpu_startup_entry\n
)"
unreachable();
but that's pretty ugly (and it might not work anyway).
I suppose we could add a new facility to mark an entire C function as an
"end" point.
But I think it would be cleanest to just do something like your patch
and have the entry code be asm which then calls cpu_bringup_and_idle().
That would make it consistent with all other entry code, which all lives
in asm.
--
Josh
and that seems to work. I need to properly verify and test, but the
explanation is that as opposed to the above, cpu_startup_entry() is on the
idle task's stack and the hint is then taken into account. The unwound
stack seems to be complete, so it could indeed be the fix.
Not the correct one though. Objtool rightfully complains with
arch/x86/xen/smp_pv.o: warning: objtool: cpu_bringup_and_idle()+0x6a: undefined stack state
and all the other hacks I tried ended up in the same dead alley. It seems
to me the correct fix is that all orc entries for cpu_bringup_and_idle()
should have "end" property set to 1, since it is the first function on the
stack. I don't know how to achieve that without the assembly hack in the
patch I sent. If I am not missing something, of course.
Josh, any idea?
Yeah, I think mucking with the unwind hints in C code is going to be
precarious. You could maybe have something like
asm("
UNWIND_HINT_EMPTY\n
mov $CPUHP_AP_ONLINE_IDLE, %rdi\n
call cpu_startup_entry\n
)"
unreachable();
but that's pretty ugly (and it might not work anyway).
I suppose we could add a new facility to mark an entire C function as an
"end" point.
I think it would be an overkill for what I perceive as one-off scenario.
Maybe if there are more use cases in the future, but I doubt it.
But I think it would be cleanest to just do something like your patch
and have the entry code be asm which then calls cpu_bringup_and_idle().
That would make it consistent with all other entry code, which all lives
in asm.