Messages in this thread | | | Date | Thu, 11 Jun 2020 10:46:36 -0700 | From | Sean Christopherson <> | Subject | Re: [PATCH v2] x86/split_lock: Sanitize userspace and guest error output |
| |
On Thu, Jun 11, 2020 at 01:37:19PM -0400, Prarit Bhargava wrote: > > > On 6/8/20 1:15 PM, Sean Christopherson wrote: > > On Mon, Jun 08, 2020 at 08:21:14AM -0400, Prarit Bhargava wrote: > >> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > >> index 166d7c355896..e02ec81fe1eb 100644 > >> --- a/arch/x86/kernel/cpu/intel.c > >> +++ b/arch/x86/kernel/cpu/intel.c > >> @@ -1074,10 +1074,17 @@ static void split_lock_init(void) > >> split_lock_verify_msr(sld_state != sld_off); > >> } > >> > >> -static void split_lock_warn(unsigned long ip) > >> +static bool split_lock_warn(unsigned long ip, int fatal_no_warn) > >> { > >> - pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n", > >> - current->comm, current->pid, ip); > >> + if (fatal_no_warn) > >> + return false; > > > > This misses the point Xiaoyao was making. If EFLAGS.AC=1 then the #AC is a > > legacy alignment check fault and should not be treated as a split-lock #AC. > > The basic premise of the patch makes sense, but the end result is confusing > > because incorporating "fatal" and the EFLAGS.AC state into split_lock_warn() > > bastardizes both the "split_lock" and "warn" aspects of the function. > > > > E.g. something like this yields the same net effect, it's just organized > > differently. If so desired, the "bogus" message could be dropped via > > Xiaoyao's prep patch[*] so that this change would only affect the sld_fatal > > messages. > > > > [*] https://lkml.kernel.org/r/20200509110542.8159-3-xiaoyao.li@intel.com > > > > > > Sean, I will just take your patch to make things easy. I will add you as a > Signed-off-by. > > /me is testing the patch right now
Sure, here's an official SOB if it happens to work.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > > index 23fd5f319908..1aad0b8e394c 100644 > > --- a/arch/x86/kernel/cpu/intel.c > > +++ b/arch/x86/kernel/cpu/intel.c > > @@ -1071,11 +1071,14 @@ static void split_lock_init(void) > > split_lock_verify_msr(sld_state != sld_off); > > } > > > > -static void split_lock_warn(unsigned long ip) > > +static bool handle_split_lock(unsigned long ip) > > { > > pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n", > > current->comm, current->pid, ip); > > > > + if (sld_state != sld_warn) > > + return false; > > + > > /* > > * Disable the split lock detection for this task so it can make > > * progress and set TIF_SLD so the detection is re-enabled via > > @@ -1083,18 +1086,13 @@ static void split_lock_warn(unsigned long ip) > > */ > > sld_update_msr(false); > > set_tsk_thread_flag(current, TIF_SLD); > > + return true; > > } > > > > bool handle_guest_split_lock(unsigned long ip) > > { > > - if (sld_state == sld_warn) { > > - split_lock_warn(ip); > > + if (handle_split_lock(ip)) > > return true; > > - } > > - > > - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n", > > - current->comm, current->pid, > > - sld_state == sld_fatal ? "fatal" : "bogus", ip); > > > > current->thread.error_code = 0; > > current->thread.trap_nr = X86_TRAP_AC; > > @@ -1105,10 +1103,10 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock); > > > > bool handle_user_split_lock(struct pt_regs *regs, long error_code) > > { > > - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal) > > + if (regs->flags & X86_EFLAGS_AC) > > return false; > > - split_lock_warn(regs->ip); > > - return true; > > + > > + return handle_split_lock(regs->ip); > > } > > > > /* > > > > > >> + > >> + pr_warn_ratelimited("#AC: %s/%d %ssplit_lock trap at address: 0x%lx\n", > >> + current->comm, current->pid, > >> + sld_state == sld_fatal ? "fatal " : "", ip); > >> + > >> + if (sld_state == sld_fatal) > >> + return false; > >> > >> /* > >> * Disable the split lock detection for this task so it can make > >> @@ -1086,18 +1093,13 @@ static void split_lock_warn(unsigned long ip) > >> */ > >> sld_update_msr(false); > >> set_tsk_thread_flag(current, TIF_SLD); > >> + return true; > >> } > >> > >> bool handle_guest_split_lock(unsigned long ip) > >> { > >> - if (sld_state == sld_warn) { > >> - split_lock_warn(ip); > >> + if (split_lock_warn(ip, 0)) > >> return true; > >> - } > >> - > >> - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n", > >> - current->comm, current->pid, > >> - sld_state == sld_fatal ? "fatal" : "bogus", ip); > >> > >> current->thread.error_code = 0; > >> current->thread.trap_nr = X86_TRAP_AC; > >> @@ -1108,10 +1110,7 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock); > >> > >> bool handle_user_split_lock(struct pt_regs *regs, long error_code) > >> { > >> - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal) > >> - return false; > >> - split_lock_warn(regs->ip); > >> - return true; > >> + return split_lock_warn(regs->ip, regs->flags & X86_EFLAGS_AC); > >> } > >> > >> /* > >> -- > >> 2.21.3 > >> > > >
| |