linux-kernel - Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190506002108.GB110479@romley-ivt3.sc.intel.com>
Date:   Sun, 5 May 2019 17:21:08 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        Christopherson Sean J <sean.j.christopherson@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Michael Chan <michael.chan@...adcom.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, kvm@...r.kernel.org,
        netdev@...r.kernel.org, linux-wireless@...r.kernel.org
Subject: Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to
 enable/disable split lock detection during run time

On Thu, Apr 25, 2019 at 08:31:15AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@...el.com> wrote:
> 
> > +		disabled if split lock operation in kernel code happens on
> > +		the CPU. The interface doesn't show or control split lock
> > +		detection on individual CPU.
> 
> I.e. implementation and possible actual state are out of sync. Why?
> 
> Also, if it's a global flag, why waste memory on putting a sysfs knob 
> into every CPU's sysfs file?
> 
> Finally, why is a debugging facility in sysfs, why not a debugfs knob? 
> Using a sysctl would solve the percpu vs. global confusion as well ...

Can I put the interface in /sys/kernel/debug/x86/split_lock_detect?

> 
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -35,6 +35,7 @@
> >  DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> >  EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> >  
> > +static DEFINE_MUTEX(split_lock_detect_mutex);
> >  static bool split_lock_detect_enable;
> 
> 'enable' is a verb in plain form - which we use for function names.
> 
> For variable names that denotes current state we typically use past 
> tense, i.e. 'enabled'.
> 
> (The only case where we'd us the split_lock_detect_enable name for a flag 
> if it's a flag to trigger some sort of enabling action - which this 
> isn't.)
> 
> Please review the whole series for various naming mishaps.
OK.

> 
> > +	mutex_lock(&split_lock_detect_mutex);
> > +
> > +	split_lock_detect_enable = val;
> > +
> > +	/* Update the split lock detection setting in MSR on all online CPUs. */
> > +	on_each_cpu(split_lock_update_msr, NULL, 1);
> > +
> > +	if (split_lock_detect_enable)
> > +		pr_info("enabled\n");
> > +	else
> > +		pr_info("disabled\n");
> > +
> > +	mutex_unlock(&split_lock_detect_mutex);
> 
> Instead of a mutex, please just use the global atomic debug flag which 
> controls the warning printout. By using that flag both for the WARN()ing 
> and for controlling MSR state all the races are solved and the code is 
> simplified.

So is it OK to define split_lock_debug and use it in #AC handler and in
here?

static atomic_t split_lock_debug;

in #AC handler:

+       if (atomic_cmpxchg(&split_lock_debug, 0, 1) == 0) {
+               /* Only warn split lock once */
+               WARN_ONCE(1, "split lock operation detected\n");
+               atomic_set(&split_lock_debug, 0);
+       }

And in split_lock_detect_store(), replace the mutex with split_lock_debug
like this:
 
-       mutex_lock(&split_lock_detect_mutex);
+       while (atomic_cmpxchg(&split_lock_debug, 1, 0))
+              cpu_relax();
.... 
-       mutex_unlock(&split_lock_detect_mutex);
+       atomic_set(&split_lock_debug, 0);
 
Is this right code for sync in both #AC handler and in
split_lock_detect_store()?

Thanks.

-Fenghua