Messages in this thread | | | Subject | Re: [PATCH 03/15] x86/split_lock: Handle #AC exception for split lock in kernel mode | From | Dave Hansen <> | Date | Tue, 15 May 2018 08:51:24 -0700 |
| |
On 05/14/2018 11:52 AM, Fenghua Yu wrote: > +#define delay_ms 1
That seems like a dangerously-generic name that should not be a #define anyway.
> +static void delayed_reenable_split_lock(struct work_struct *w) > +{ > + if (split_lock_ac == ENABLE_SPLIT_LOCK_AC) > + _setup_split_lock(ENABLE_SPLIT_LOCK_AC); > +}
This seems like it might get confusing. We have the split lock ac *mode* (what the kernel is doing overall) and also the *status* (what mode the CPU is in at the moment).
The naming here doesn't really split up those two concepts very well.
> +/* Will the faulting instruction be re-executed? */ > +static bool re_execute(struct pt_regs *regs) > +{ > + /* > + * The only reason for generating #AC from kernel is because of > + * split lock. The kernel faulting instruction will be re-executed. > + */ > + if (!user_mode(regs)) > + return true; > + > + return false; > +}
This helper with a single user is a bit unnecessary. Just open-code this and move the comments into the caller.
> +/* > + * #AC handler for kernel split lock is called by generic #AC handler. > + * > + * Disable #AC for split lock on this CPU so that the faulting instruction > + * gets executed. The #AC for split lock is re-enabled later. > + */ > +bool do_split_lock_exception(struct pt_regs *regs, unsigned long error_code) > +{ > + unsigned long delay = msecs_to_jiffies(delay_ms); > + unsigned long address = read_cr2(); /* Get the faulting address */ > + int this_cpu = smp_processor_id();
How does this end up working? This seems to depend on this handler not getting preempted.
> + if (!re_execute(regs)) > + return false; > + > + pr_info_ratelimited("Alignment check for split lock at %lx\n", address);
This is a potential KASLR bypass, I believe. We shouldn't be printing raw kernel addresses.
We have some nice printk's for page faults that give you kernel symbols. Could you copy one of those?
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 03f3d7695dac..c07b817bbbe9 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -61,6 +61,7 @@ > #include <asm/mpx.h> > #include <asm/vm86.h> > #include <asm/umip.h> > +#include <asm/cpu.h> > > #ifdef CONFIG_X86_64 > #include <asm/x86_init.h> > @@ -286,10 +287,21 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str, > unsigned long trapnr, int signr) > { > siginfo_t info; > + int ret; > > RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); > > /* > + * #AC exception could be handled by split lock handler. > + * If the handler can't handle the exception, go to generic #AC handler. > + */ > + if (trapnr == X86_TRAP_AC) { > + ret = do_split_lock_exception(regs, error_code); > + if (ret) > + return; > + }
Why are you hooking into do_error_trap()? Shouldn't you just be installing do_split_lock_exception() as *the* #AC handler and put it in the IDT?
| |