linux-kernel - Re: [RFC][PATCH 4/4] x86, mpx: context-switch new MPX address size MSR

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] [day] [month] [year] [list]

Date:   Fri, 27 Jan 2017 09:31:22 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [RFC][PATCH 4/4] x86, mpx: context-switch new MPX address size
 MSR


* Dave Hansen <dave.hansen@...ux.intel.com> wrote:

> + * The MPX tables change sizes based on the size of the virtual
> + * (aka. linear) address space.  There is an MSR to tell the CPU
> + * whether we want the legacy-style ones or the larger ones when
> + * we are running with an eXtended virtual address space.
> + */
> +static void switch_mawa(struct mm_struct *prev, struct mm_struct *next)
> +{
> +	/*
> +	 * Note: there is one and only one bit in use in the MSR
> +	 * at this time, so we do not have to be concerned with
> +	 * preseving any of the other bits.  Just write 0 or 1.
> +	 */
> +	unsigned IA32_MPX_LAX_ENABLE_MASK = 0x00000001;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_MPX))
> +		return;
> +	/*
> +	 * FIXME: do we want a check here for the 5-level paging
> +	 * CR4 bit or CPUID bit, or is the mawa check below OK?
> +	 * It's not obvious what would be the fastest or if it
> +	 * matters.
> +	 */
> +
> +	/*
> +	 * Avoid the relatively costly MSR if we are not changing
> +	 * MAWA state.  All processes not using MPX will have a
> +	 * mpx_mawa_shift()=0, so we do not need to check
> +	 * separately for whether MPX management is enabled.
> +	 */
> +	if (mpx_mawa_shift(prev) == mpx_mawa_shift(next))
> +		return;

Please stop the senseless looking wrappery - if the field is name sensibly then it 
can be accessed directly through mm_struct.

> +
> +	if (mpx_mawa_shift(next)) {
> +		wrmsr(MSR_IA32_MPX_LAX, IA32_MPX_LAX_ENABLE_MASK, 0x0);
> +	} else {
> +		/* clear the enable bit: */
> +		wrmsr(MSR_IA32_MPX_LAX, 0x0, 0x0);
> +	}
> +}
> +
>  void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  			struct task_struct *tsk)
>  {
> @@ -136,6 +177,7 @@ void switch_mm_irqs_off(struct mm_struct
>  		/* Load per-mm CR4 state */
>  		load_mm_cr4(next);
>  
> +		switch_mawa(prev, next);

This implementation adds about 4-5 unnecessary instructions to the context 
switching hot path of every non-MPX task, even on non-MPX hardware.

Please make sure that this is something like:

	if (unlikely(prev->mpx_msr_val != next->mpx_msr_val))
		switch_mpx(prev, next);

... which reduces the hot path overhead to something like 2 instruction (if we are 
lucky).

This can be put into switch_mpx() and can be inlined - just make sure that on a 
defconfig the generated machine code is sane.

Thanks,

	Ingo