| Message ID | 20200501225838.9866-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
On 02.05.2020 00:58, Andrew Cooper wrote: > The only user of this facility is dom_crash_sync_extable() by passing 0 into > asm_domain_crash_synchronous(). The common error cases are already covered > with show_page_walk(), leaving only %ss/%fs selector/segment errors in the > compat case. > > Point at dom_crash_sync_extable in the error message, which is about as good > as the error hints from other users of asm_domain_crash_synchronous(), and > drop last_extable_addr. While I'm not entirely opposed, I'd like you to clarify that you indeed mean to (mostly) revert your own improvement from 6 or 7 years back (commit 8e0da8c07f4f). I'm also surprised to find this as part of the series it's in - in what way does this logic get in the way of CET-SS? Jan
On 04/05/2020 13:44, Jan Beulich wrote: > On 02.05.2020 00:58, Andrew Cooper wrote: >> The only user of this facility is dom_crash_sync_extable() by passing 0 into >> asm_domain_crash_synchronous(). The common error cases are already covered >> with show_page_walk(), leaving only %ss/%fs selector/segment errors in the >> compat case. >> >> Point at dom_crash_sync_extable in the error message, which is about as good >> as the error hints from other users of asm_domain_crash_synchronous(), and >> drop last_extable_addr. > While I'm not entirely opposed, I'd like you to clarify that you indeed > mean to (mostly) revert your own improvement from 6 or 7 years back > (commit 8e0da8c07f4f). I'm also surprised to find this as part of the > series it's in - in what way does this logic get in the way of CET-SS? It was part of the exception_fixup() cleanup. The first 4 patches not specifically related to CET-SS. ~Andrew
On 11.05.2020 16:53, Andrew Cooper wrote: > On 04/05/2020 13:44, Jan Beulich wrote: >> On 02.05.2020 00:58, Andrew Cooper wrote: >>> The only user of this facility is dom_crash_sync_extable() by passing 0 into >>> asm_domain_crash_synchronous(). The common error cases are already covered >>> with show_page_walk(), leaving only %ss/%fs selector/segment errors in the >>> compat case. >>> >>> Point at dom_crash_sync_extable in the error message, which is about as good >>> as the error hints from other users of asm_domain_crash_synchronous(), and >>> drop last_extable_addr. >> While I'm not entirely opposed, I'd like you to clarify that you indeed >> mean to (mostly) revert your own improvement from 6 or 7 years back >> (commit 8e0da8c07f4f). I'm also surprised to find this as part of the >> series it's in - in what way does this logic get in the way of CET-SS? > > It was part of the exception_fixup() cleanup. The first 4 patches not > specifically related to CET-SS. "Cleanup" doesn't mean it's needed as a prereq for the CET-SS, so I'm afraid it's still not really clear to me whether we indeed want to effectively revert the earlier change, which had a reason to be done the way it was done. Jan
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 33e5d21ece..fe9457cdb6 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -96,7 +96,6 @@ static char __read_mostly opt_nmi[10] = "fatal"; string_param("nmi", opt_nmi); DEFINE_PER_CPU(uint64_t, efer); -static DEFINE_PER_CPU(unsigned long, last_extable_addr); DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, gdt); DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, gdt_l1e); @@ -786,7 +785,6 @@ static void do_trap(struct cpu_user_regs *regs) { dprintk(XENLOG_ERR, "Trap %u: %p [%ps] -> %p\n", trapnr, _p(regs->rip), _p(regs->rip), _p(fixup)); - this_cpu(last_extable_addr) = regs->rip; regs->rip = fixup; return; } @@ -1099,7 +1097,6 @@ void do_invalid_op(struct cpu_user_regs *regs) die: if ( (fixup = search_exception_table(regs)) != 0 ) { - this_cpu(last_extable_addr) = regs->rip; regs->rip = fixup; return; } @@ -1122,7 +1119,6 @@ void do_int3(struct cpu_user_regs *regs) if ( (fixup = search_exception_table(regs)) != 0 ) { - this_cpu(last_extable_addr) = regs->rip; dprintk(XENLOG_DEBUG, "Trap %u: %p [%ps] -> %p\n", TRAP_int3, _p(regs->rip), _p(regs->rip), _p(fixup)); regs->rip = fixup; @@ -1461,7 +1457,6 @@ void do_page_fault(struct cpu_user_regs *regs) perfc_incr(copy_user_faults); if ( unlikely(regs->error_code & PFEC_reserved_bit) ) reserved_bit_page_fault(addr, regs); - this_cpu(last_extable_addr) = regs->rip; regs->rip = fixup; return; } @@ -1591,7 +1586,6 @@ void do_general_protection(struct cpu_user_regs *regs) { dprintk(XENLOG_INFO, "GPF (%04x): %p [%ps] -> %p\n", regs->error_code, _p(regs->rip), _p(regs->rip), _p(fixup)); - this_cpu(last_extable_addr) = regs->rip; regs->rip = fixup; return; } @@ -2085,10 +2079,7 @@ void asm_domain_crash_synchronous(unsigned long addr) */ clac(); - if ( addr == 0 ) - addr = this_cpu(last_extable_addr); - - printk("domain_crash_sync called from entry.S: fault at %p %pS\n", + printk("domain_crash_sync called from entry.S: issue around %p %pS\n", _p(addr), _p(addr)); __domain_crash(current->domain); diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index d55453f3f3..a3ce298529 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -527,7 +527,7 @@ ENTRY(dom_crash_sync_extable) sete %al leal (%rax,%rax,2),%eax orb %al,UREGS_cs(%rsp) - xorl %edi,%edi + lea dom_crash_sync_extable(%rip), %rdi jmp asm_domain_crash_synchronous /* Does not return */ .popsection #endif /* CONFIG_PV */
The only user of this facility is dom_crash_sync_extable() by passing 0 into asm_domain_crash_synchronous(). The common error cases are already covered with show_page_walk(), leaving only %ss/%fs selector/segment errors in the compat case. Point at dom_crash_sync_extable in the error message, which is about as good as the error hints from other users of asm_domain_crash_synchronous(), and drop last_extable_addr. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/traps.c | 11 +---------- xen/arch/x86/x86_64/entry.S | 2 +- 2 files changed, 2 insertions(+), 11 deletions(-)