netdev - Re: [PATCH v6 14/20] 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]

Date:   Thu, 4 Apr 2019 21:11:18 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v6 14/20] x86/split_lock: Add a sysfs interface to
 enable/disable split lock detection during run time

On Wed, 3 Apr 2019, Fenghua Yu wrote:
> +
> +static ssize_t
> +split_lock_detect_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	return sprintf(buf, "%u\n", READ_ONCE(split_lock_detect_val));

Please stop sprinkling READ_ONCE all over the place or can you explain why
this is in any way useful? You know what READ/WRITE_ONCE() is for, right?

> +}
> +
> +static ssize_t
> +split_lock_detect_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	u32 val, l, h;
> +	int cpu, ret;
> +
> +	ret = kstrtou32(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != DISABLE_SPLIT_LOCK_DETECT && val != ENABLE_SPLIT_LOCK_DETECT)
> +		return -EINVAL;

As this is really a simple boolean you can just use strtobool() and be done
with it.

> +
> +	/*
> +	 * Since split lock could be disabled by kernel #AC handler or user
> +	 * may directly change bit 29 in MSR_TEST_CTL, split lock setting on

The user can change bit 29 in that MSR? If you talk about /dev/msr then I
really do not care. That interface should die.

Aside of that your usage of the term 'user' is really misleading and
inconsistent all over the place.

> +	 * each CPU may be different from global setting split_lock_detect_val
> +	 * by now. Update MSR on each CPU, so all of CPUs will have same split
> +	 * lock setting.

That helps in which way? If #AC was detected in the kernel then

     1) It's likely to be switched off again right away

     2) The WARN_ONCE() already triggered and will not warn again.

So what's the point here, really? If the kernel triggers #AC, game
over. Fix the kernel first. If your kernel is clean, then why do you need
that knob at all?

> +	 */
> +	mutex_lock(&split_lock_detect_mutex);
> +
> +	WRITE_ONCE(split_lock_detect_val, val);

Oh well.

> +	/*
> +	 * Get MSR_TEST_CTL on this CPU, assuming all CPUs have same value
> +	 * in the MSR except split lock detection bit (bit 29).

And some day in the future this breaks because MRS_TEST_CTL has some other
shiny bits.

> +	 */
> +	rdmsr(MSR_TEST_CTL, l, h);
> +	l = new_sp_test_ctl_val(l);
> +	/* Update the split lock detection setting on all online CPUs. */
> +	for_each_online_cpu(cpu)

And what exactly protects the online cpu mask?

> +		wrmsr_on_cpu(cpu, MSR_TEST_CTL, l, h);

Oh well. Instead of just having a function which does:

fun() 
   	 if (ac_...enabled)
	 	msr_set_bit()
	 else
	 	msr_clear_bit()

and invoke that from cpu init code and from here via on_each_cpu() or such?

> +	mutex_unlock(&split_lock_detect_mutex);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(split_lock_detect);
> +
> +static int __init split_lock_init(void)
> +{
> +	int ret;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> +		return -ENODEV;
> +
> +	ret = device_create_file(cpu_subsys.dev_root,
> +				 &dev_attr_split_lock_detect);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

What's wrong with:

       return device_create_file();

??? Not hard enough to read, right?

> +}
> +

Pointless empty line.

> +subsys_initcall(split_lock_init);

Thanks,

	tglx