Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
> That comment is indeed now obsolete. I can squash something like so: > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 81d56fdef1c3..ce67437aaf3f 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -958,21 +958,8 @@ static int add_call_destinations(struct objtool_file > *file) > } > > /* > - * The .alternatives section requires some extra special care, over and above > - * what other special sections require: > - * > - * 1. Because alternatives are patched in-place, we need to insert a fake > jump > - *instruction at the end so that validate_branch() skips all the original > - *replaced instructions when validating the new instruction path. > - * > - * 2. An added wrinkle is that the new instruction length might be zero. In > - *that case the old instructions are replaced with noops. We simulate > that > - *by creating a fake jump as the only new instruction. > - * > - * 3. In some cases, the alternative section includes an instruction which > - *conditionally jumps to the _end_ of the entry. We have to modify these > - *jumps' destinations to point back to .text rather than the end of the > - *entry in .altinstr_replacement. > + * The .alternatives section requires some extra special care over and above > + * other special sections because alternatives are patched in place. > */ > static int handle_group_alt(struct objtool_file *file, > struct special_alt *special_alt, Looks good to me. Miroslav
Re: [PATCH 3/3] objtool: Support stack layout changes in alternatives
On Tue, 22 Dec 2020, Josh Poimboeuf wrote: > BTW, another benefit of these changes is that, thanks to some related > cleanups (new fake nops and alt_group struct) objtool can finally be rid > of fake jumps, which were a constant source of headaches. \o/ You may also want to remove/edit the comment right before handle_group_alt() now that fake jumps are gone. Anyway, I walked through the patch (set) and I think it should work fine (but I am not confident enough to give it Reviewed-by. My head spins :)). I even like the change. Also, 1/3 is a benefit on its own, so if nothing else, it could go in. Regards Miroslav
[Xen-devel] [PATCH v3 0/2] x86/xen: Make idle tasks reliable
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. v2->v3: - change prototype of asm_cpu_bringup_and_idle() - replace %_ASM_SP with %rsp and %esp respectively - fix build for !CONFIG_XEN_PV_SMP v1->v2: - call instruction used instead of push+jmp - initial_stack used directly v1 https://lore.kernel.org/live-patching/20200312142007.11488-1-mbe...@suse.cz/ v2 https://lore.kernel.org/live-patching/20200319095606.23627-1-mbe...@suse.cz/ 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 | 18 -- 2 files changed, 18 insertions(+), 3 deletions(-) -- 2.25.1
[Xen-devel] [PATCH v3 2/2] x86/xen: Make the secondary CPU idle tasks reliable
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.
Introduce a wrapper in assembly, which just calls
cpu_bringup_and_idle(). The return address is thus pushed on the stack
and the wrapper contains the annotation hint for the unwinder regarding
the stack state.
Signed-off-by: Miroslav Benes
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 802ee5bba66c..8fb8a50a28b4 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) =
{ .irq = -1 };
static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
+void asm_cpu_bringup_and_idle(void);
static void cpu_bringup(void)
{
@@ -309,7 +310,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct
*idle)
* pointing just below where pt_regs would be if it were a normal
* kernel entry.
*/
- ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
+ ctxt->user_regs.eip = (unsigned long)asm_cpu_bringup_and_idle;
ctxt->flags = VGCF_IN_KERNEL;
ctxt->user_regs.eflags = 0x1000; /* IOPL_RING1 */
ctxt->user_regs.ds = __USER_DS;
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index d63806e1ff7a..7d1c4fcbe8f7 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -58,6 +58,16 @@ SYM_CODE_START(startup_xen)
call xen_start_kernel
SYM_CODE_END(startup_xen)
__FINIT
+
+#ifdef CONFIG_XEN_PV_SMP
+.pushsection .text
+SYM_CODE_START(asm_cpu_bringup_and_idle)
+ UNWIND_HINT_EMPTY
+
+ call cpu_bringup_and_idle
+SYM_CODE_END(asm_cpu_bringup_and_idle)
+.popsection
+#endif
#endif
.pushsection .text
--
2.25.1
[Xen-devel] [PATCH v3 1/2] x86/xen: Make the boot CPU idle task reliable
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, which is exactly what call instruction does. Signed-off-by: Miroslav Benes --- arch/x86/xen/xen-head.S | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 1d0cee3163e4..d63806e1ff7a 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -35,7 +35,11 @@ SYM_CODE_START(startup_xen) rep __ASM_SIZE(stos) mov %_ASM_SI, xen_start_info - mov $init_thread_union+THREAD_SIZE, %_ASM_SP +#ifdef CONFIG_X86_64 + mov initial_stack(%rip), %rsp +#else + mov pa(initial_stack), %esp +#endif #ifdef CONFIG_X86_64 /* Set up %gs. @@ -51,7 +55,7 @@ SYM_CODE_START(startup_xen) wrmsr #endif - jmp xen_start_kernel + call xen_start_kernel SYM_CODE_END(startup_xen) __FINIT #endif -- 2.25.1
Re: [Xen-devel] [PATCH v2 2/2] x86/xen: Make the secondary CPU idle tasks reliable
On Thu, 19 Mar 2020, Jan Beulich wrote:
> On 19.03.2020 10:56, Miroslav Benes wrote:
> > --- a/arch/x86/xen/smp_pv.c
> > +++ b/arch/x86/xen/smp_pv.c
> > @@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq,
> > xen_irq_work) = { .irq = -1 };
> > static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
> >
> > static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
> > +extern unsigned char asm_cpu_bringup_and_idle[];
>
> Imo this would better reflect the actual type, i.e. be a function
> decl. If left as an array one, I guess you may want to add const.
I sticked to what x86 has for secondary_startup_64. I can make it
void asm_cpu_bringup_and_idle(void);
Miroslav
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] x86/xen: Make the boot CPU idle task reliable
On Thu, 19 Mar 2020, Jan Beulich wrote: > On 19.03.2020 10:56, Miroslav Benes wrote: > > 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, which is > > exactly what call instruction does. > > > > Signed-off-by: Miroslav Benes > > --- > > arch/x86/xen/xen-head.S | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > > index 1d0cee3163e4..edc776af0e0a 100644 > > --- a/arch/x86/xen/xen-head.S > > +++ b/arch/x86/xen/xen-head.S > > @@ -35,7 +35,11 @@ SYM_CODE_START(startup_xen) > > rep __ASM_SIZE(stos) > > > > mov %_ASM_SI, xen_start_info > > - mov $init_thread_union+THREAD_SIZE, %_ASM_SP > > +#ifdef CONFIG_X86_64 > > + mov initial_stack(%rip), %_ASM_SP > > +#else > > + mov pa(initial_stack), %_ASM_SP > > +#endif > > If you need to distinguish the two anyway, why not use %rsp and > %esp respectively? I could, I just preferred the unification instead. Will change it if you think it would be better. Miroslav ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 1/2] x86/xen: Make the boot CPU idle task reliable
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, which is exactly what call instruction does. Signed-off-by: Miroslav Benes --- arch/x86/xen/xen-head.S | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 1d0cee3163e4..edc776af0e0a 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -35,7 +35,11 @@ SYM_CODE_START(startup_xen) rep __ASM_SIZE(stos) mov %_ASM_SI, xen_start_info - mov $init_thread_union+THREAD_SIZE, %_ASM_SP +#ifdef CONFIG_X86_64 + mov initial_stack(%rip), %_ASM_SP +#else + mov pa(initial_stack), %_ASM_SP +#endif #ifdef CONFIG_X86_64 /* Set up %gs. @@ -51,7 +55,7 @@ SYM_CODE_START(startup_xen) wrmsr #endif - jmp xen_start_kernel + call xen_start_kernel SYM_CODE_END(startup_xen) __FINIT #endif -- 2.25.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 0/2] x86/xen: Make idle tasks reliable
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. v1->v2: - call instruction used instead of push+jmp - initial_stack used directly There is a thing which makes me slightly uncomfortable. s/jmp/call/ means that, theoretically, the called function could return. GCC then generates not so nice code and there is asm_cpu_bringup_and_idle+0x5/0x1000 symbol last on the stack due to alignment in asm/x86/xen/xen-head.S which could be confusing. Practically it is all fine, because neither xen_start_kernel(), nor cpu_bringup_and_idle() return (there is unbounded loop in cpu_startup_entry() around do_idle()). __noreturn annotation of these functions did not help. So I don't think it is really a problem, but one may wonder. 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 | 16 ++-- 2 files changed, 16 insertions(+), 3 deletions(-) -- 2.25.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/2] x86/xen: Make the secondary CPU idle tasks reliable
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.
Introduce a wrapper in assembly, which just calls
cpu_bringup_and_idle(). The return address is thus pushed on the stack
and the wrapper contains the annotation hint for the unwinder regarding
the stack state.
Signed-off-by: Miroslav Benes
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 8
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 802ee5bba66c..6b88cdcbef8f 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) =
{ .irq = -1 };
static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
+extern unsigned char asm_cpu_bringup_and_idle[];
static void cpu_bringup(void)
{
@@ -309,7 +310,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct
*idle)
* pointing just below where pt_regs would be if it were a normal
* kernel entry.
*/
- ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
+ ctxt->user_regs.eip = (unsigned long)asm_cpu_bringup_and_idle;
ctxt->flags = VGCF_IN_KERNEL;
ctxt->user_regs.eflags = 0x1000; /* IOPL_RING1 */
ctxt->user_regs.ds = __USER_DS;
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index edc776af0e0a..9dc6f9a420a8 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -58,6 +58,14 @@ SYM_CODE_START(startup_xen)
call xen_start_kernel
SYM_CODE_END(startup_xen)
__FINIT
+
+.pushsection .text
+SYM_CODE_START(asm_cpu_bringup_and_idle)
+ UNWIND_HINT_EMPTY
+
+ call cpu_bringup_and_idle
+SYM_CODE_END(asm_cpu_bringup_and_idle)
+.popsection
#endif
.pushsection .text
--
2.25.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/xen: Make the secondary CPU idle tasks reliable
On Mon, 16 Mar 2020, Josh Poimboeuf wrote:
> On Mon, Mar 16, 2020 at 04:51:12PM +0100, Miroslav Benes wrote:
> > > diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
> > > index 6b88cdcbef8f..39afd88309cb 100644
> > > --- a/arch/x86/xen/smp_pv.c
> > > +++ b/arch/x86/xen/smp_pv.c
> > > @@ -92,6 +92,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
> > > {
> > > cpu_bringup();
> > > boot_init_stack_canary();
> > > + asm volatile (UNWIND_HINT(ORC_REG_UNDEFINED, 0, ORC_TYPE_CALL,
> > > 1));
> > > cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
> > > }
> > >
> > > 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.
Ack.
Thanks
Miroslav
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/xen: Make the boot CPU idle task reliable
On Mon, 16 Mar 2020, Boris Ostrovsky wrote: > > > On 3/12/20 10:20 AM, Miroslav Benes wrote: > > --- a/arch/x86/xen/xen-head.S > > +++ b/arch/x86/xen/xen-head.S > > @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen) > > rep __ASM_SIZE(stos) > > > > mov %_ASM_SI, xen_start_info > > - mov $init_thread_union+THREAD_SIZE, %_ASM_SP > > + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP > > This is initial_stack, isn't it? It is. I'll change it. Thanks Miroslav ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/xen: Make the secondary CPU idle tasks reliable
On Fri, 13 Mar 2020, Miroslav Benes wrote:
> On Fri, 13 Mar 2020, Jürgen Groß wrote:
>
> > On 12.03.20 15:20, Miroslav Benes wrote:
> > > 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
> > > ---
> > > arch/x86/xen/smp_pv.c | 3 ++-
> > > arch/x86/xen/xen-head.S | 10 ++
> > > 2 files changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
> > > index 802ee5bba66c..6b88cdcbef8f 100644
> > > --- a/arch/x86/xen/smp_pv.c
> > > +++ b/arch/x86/xen/smp_pv.c
> > > @@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq,
> > > xen_irq_work)
> > > = { .irq = -1 };
> > > static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1
> > > };
> > >
> > > static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
> > > +extern unsigned char asm_cpu_bringup_and_idle[];
> > >
> > > static void cpu_bringup(void)
> > > {
> >
> > 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
>
> diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
> index 6b88cdcbef8f..39afd88309cb 100644
> --- a/arch/x86/xen/smp_pv.c
> +++ b/arch/x86/xen/smp_pv.c
> @@ -92,6 +92,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
> {
> cpu_bringup();
> boot_init_stack_canary();
> + asm volatile (UNWIND_HINT(ORC_REG_UNDEFINED, 0, ORC_TYPE_CALL, 1));
> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
> }
>
> 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___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 2/2] x86/xen: Make the secondary CPU idle tasks reliable
On Fri, 13 Mar 2020, Jürgen Groß wrote:
> On 12.03.20 15:20, Miroslav Benes wrote:
> > 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
> > ---
> > arch/x86/xen/smp_pv.c | 3 ++-
> > arch/x86/xen/xen-head.S | 10 ++
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
> > index 802ee5bba66c..6b88cdcbef8f 100644
> > --- a/arch/x86/xen/smp_pv.c
> > +++ b/arch/x86/xen/smp_pv.c
> > @@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work)
> > = { .irq = -1 };
> > static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
> >
> > static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
> > +extern unsigned char asm_cpu_bringup_and_idle[];
> >
> > static void cpu_bringup(void)
> > {
>
> 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
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 6b88cdcbef8f..39afd88309cb 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -92,6 +92,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
{
cpu_bringup();
boot_init_stack_canary();
+ asm volatile (UNWIND_HINT(ORC_REG_UNDEFINED, 0, ORC_TYPE_CALL, 1));
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}
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___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/xen: Make the boot CPU idle task reliable
On Thu, 12 Mar 2020, Andrew Cooper wrote: > On 12/03/2020 14:20, Miroslav Benes wrote: > > 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 > > --- > > arch/x86/xen/xen-head.S | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > > index 1d0cee3163e4..642f346bfe02 100644 > > --- a/arch/x86/xen/xen-head.S > > +++ b/arch/x86/xen/xen-head.S > > @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen) > > rep __ASM_SIZE(stos) > > > > mov %_ASM_SI, xen_start_info > > - mov $init_thread_union+THREAD_SIZE, %_ASM_SP > > + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP > > > > #ifdef CONFIG_X86_64 > > /* Set up %gs. > > @@ -51,7 +51,9 @@ SYM_CODE_START(startup_xen) > > wrmsr > > #endif > > > > + push $1f > > jmp xen_start_kernel > > +1: > > Hang on. Isn't this just a `call` instruction written in longhand? It is (as far as I know). I wanted to keep it opencoded for a reason I don't remember now. I'll change it. Thanks. Miroslav___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [RFC PATCH 2/2] x86/xen: Make the secondary CPU idle tasks reliable
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
---
arch/x86/xen/smp_pv.c | 3 ++-
arch/x86/xen/xen-head.S | 10 ++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 802ee5bba66c..6b88cdcbef8f 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -53,6 +53,7 @@ static DEFINE_PER_CPU(struct xen_common_irq, xen_irq_work) =
{ .irq = -1 };
static DEFINE_PER_CPU(struct xen_common_irq, xen_pmu_irq) = { .irq = -1 };
static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id);
+extern unsigned char asm_cpu_bringup_and_idle[];
static void cpu_bringup(void)
{
@@ -309,7 +310,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct
*idle)
* pointing just below where pt_regs would be if it were a normal
* kernel entry.
*/
- ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
+ ctxt->user_regs.eip = (unsigned long)asm_cpu_bringup_and_idle;
ctxt->flags = VGCF_IN_KERNEL;
ctxt->user_regs.eflags = 0x1000; /* IOPL_RING1 */
ctxt->user_regs.ds = __USER_DS;
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 642f346bfe02..c9a9c0bb79ed 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -56,6 +56,16 @@ SYM_CODE_START(startup_xen)
1:
SYM_CODE_END(startup_xen)
__FINIT
+
+.pushsection .text
+SYM_CODE_START(asm_cpu_bringup_and_idle)
+ UNWIND_HINT_EMPTY
+
+ push $1f
+ jmp cpu_bringup_and_idle
+1:
+SYM_CODE_END(asm_cpu_bringup_and_idle)
+.popsection
#endif
.pushsection .text
--
2.25.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] x86/xen: Make the boot CPU idle task reliable
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 --- arch/x86/xen/xen-head.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 1d0cee3163e4..642f346bfe02 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -35,7 +35,7 @@ SYM_CODE_START(startup_xen) rep __ASM_SIZE(stos) mov %_ASM_SI, xen_start_info - mov $init_thread_union+THREAD_SIZE, %_ASM_SP + mov $init_thread_union+THREAD_SIZE-SIZEOF_PTREGS, %_ASM_SP #ifdef CONFIG_X86_64 /* Set up %gs. @@ -51,7 +51,9 @@ SYM_CODE_START(startup_xen) wrmsr #endif + push $1f jmp xen_start_kernel +1: SYM_CODE_END(startup_xen) __FINIT #endif -- 2.25.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/2] x86/xen: Make idle tasks reliable
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
