Re: [PATCH v14] x86/split_lock: Enable split lock detection by kernel

[Thomas Gleixner <tglx@linutronix.de>]

Tony,

"Luck, Tony" <tony.luck@intel.com> writes:
> +	split_lock_detect=
> +			[X86] Enable split lock detection
> +
> +			When enabled (and if hardware support is present), atomic
> +			instructions that access data across cache line
> +			boundaries will result in an alignment check exception.
> +
> +			off	- not enabled
> +
> +			warn	- the kernel will pr_alert about applications

pr_alert is not a verb. And the implementation uses
pr_warn_ratelimited(). So this should be something like:

                       The kernel will emit rate limited warnings about
                       applications ...

> +				  triggering the #AC exception
> @@ -40,4 +40,21 @@ int mwait_usable(const struct cpuinfo_x86 *);
>  unsigned int x86_family(unsigned int sig);
>  unsigned int x86_model(unsigned int sig);
>  unsigned int x86_stepping(unsigned int sig);
> +#ifdef CONFIG_CPU_SUP_INTEL
> +extern void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
> +extern bool split_lock_detect_enabled(void);

That function is unused.

> +extern bool handle_user_split_lock(struct pt_regs *regs, long error_code);
> +extern void switch_sld(struct task_struct *);
> +#else
> +static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
> +static inline bool split_lock_detect_enabled(void)
> +{
> +	return false;
> +}
> +static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> +{
> +	return false;
> +}
> +static inline void switch_sld(struct task_struct *prev) {}
> +#endif
  
> +enum split_lock_detect_state {
> +	sld_off = 0,
> +	sld_warn,
> +	sld_fatal,
> +};
> +
> +/*
> + * Default to sld_off because most systems do not support
> + * split lock detection. split_lock_setup() will switch this

Can you please add: If supported, then ...

> + * to sld_warn, and then check to see if there is a command
> + * line override.

I had to read this 3 times and then stare at the code.

> + */
> +static enum split_lock_detect_state sld_state = sld_off;
> +
> +static void __init split_lock_setup(void)
> +{
> +	enum split_lock_detect_state sld;
> +	char arg[20];
> +	int i, ret;
> +
> +	sld_state = sld = sld_warn;

This intermediate variable is pointless.

> +	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> +
> +	ret = cmdline_find_option(boot_command_line, "split_lock_detect",
> +				  arg, sizeof(arg));
> +	if (ret < 0)
> +		goto print;
> +
> +	for (i = 0; i < ARRAY_SIZE(sld_options); i++) {
> +		if (match_option(arg, ret, sld_options[i].option)) {
> +			sld = sld_options[i].state;
> +			break;
> +		}
> +	}
> +
> +	if (sld != sld_state)
> +		sld_state = sld;
> +
> +print:

> +/*
> + * The TEST_CTRL MSR is per core. So multiple threads can
> + * read/write the MSR in parallel. But it's possible to
> + * simplify the read/write without locking and without
> + * worry about overwriting the MSR because only bit 29
> + * is implemented in the MSR and the bit is set as 1 by all
> + * threads. Locking may be needed in the future if situation
> + * is changed e.g. other bits are implemented.

This sentence doesn't parse. Something like this perhaps:

     Locking is not required at the moment because only bit 29 of this
     MSR is implemented and locking would not prevent that the operation
     of one thread is immediately undone by the sibling thread.

This implies that locking might become necessary when new bits are added.

> + */
> +
> +static bool __sld_msr_set(bool on)
> +{
> +	u64 test_ctrl_val;
> +
> +	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
> +		return false;
> +
> +	if (on)
> +		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> +	else
> +		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> +
> +	if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val))
> +		return false;
> +
> +	return true;

	return !wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val);

> +}
> +
> +static void split_lock_init(void)
> +{
> +	if (sld_state == sld_off)
> +		return;
> +
> +	if (__sld_msr_set(true))
> +		return;
> +
> +	/*
> +	 * If this is anything other than the boot-cpu, you've done
> +	 * funny things and you get to keep whatever pieces.
> +	 */
> +	pr_warn("MSR fail -- disabled\n");
> +	__sld_msr_set(sld_off);

That should do:

        sld_state = sld_off;

for consistency sake.

> +}
> +
> +bool split_lock_detect_enabled(void)
> +{
> +	return sld_state != sld_off;
> +}
> +
> +bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> +{
> +	if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
> +		return false;
> +
> +	pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n",
> +		 current->comm, current->pid, regs->ip);

So with 10 prints per 5 seconds an intentional offender can still fill dmesg
pretty good. A standard dmesg buffer should be full of this in
~15min. Not a big issue, but it might be annoying. Let's start with this
and deal with it when people complain.

The magic below really lacks a comment. Something like:

	/*
         * Disable the split lock detection for this task so it can make
         * progress and set TIF_SLD so the detection is reenabled via
         * switch_to_sld() when the task is scheduled out.
         */

> +	__sld_msr_set(false);
> +	set_tsk_thread_flag(current, TIF_SLD);
> +	return true;
> +}
> +
> +void switch_sld(struct task_struct *prev)

switch_to_sld() perhaps?

> +{
> +	__sld_msr_set(true);
> +	clear_tsk_thread_flag(prev, TIF_SLD);
> +}
>  
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> +	const char str[] = "alignment check";
> +
> +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> +	if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS) == NOTIFY_STOP)
> +		return;
> +
> +	if (!user_mode(regs))
> +		die("Split lock detected\n", regs, error_code);
> +
> +	cond_local_irq_enable(regs);

This cond is pointless. We recently removed the ability for user space
to disable interrupts and even if that would still be allowed then
keeping interrupts disabled here does not make sense.

Other than those details, I really like this approach.

Thanks,

        tglx