[PATCH 0/2] x86, mpx: Fixes for 3.19

[PATCH 0/2] x86, mpx: Fixes for 3.19

[Dave Hansen]

Andi Lutomirski noticed that the current 3.19 code doesn't support
32-bit binaries on 64-bit kernels, but that it is not explicitly
disabled.

I've also noticed a small performance regression that MPX was causing
in some circumstances.  The fix is simple.

 include/asm/mmu_context.h |   20 +++++++++++++++++++-
 mm/mpx.c                  |    6 ++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

[PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We had originally planned on submitting MPX support in one patch
set.  We eventually broke it up in to two pieces for easier
review.  One of the features that didn't make the first round
was supporting 32-bit binaries on 64-bit kernels.

Once we split the set up, we never added code to restrict 32-bit
binaries from _using_ MPX on 64-bit kernels.

The 32-bit bounds tables are a different format than the 64-bit
ones.  Without this patch, the kernel will try to read a 32-bit
binary's tables as if they were the 64-bit version.  They will
likely be noticed as being invalid rather quickly and the app
will get killed, but that's kinda mean.

This patch adds an explicit check, and will make a 64-bit kernel
essentially behave as if it has no MPX support when called from
a 32-bit binary.

I intend to remove this check once the mixed binary support is
added.  Those patches are posted if anyone is interested:

	https://lkml.org/lkml/2014/12/12/632

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
---

 b/arch/x86/mm/mpx.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff -puN arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19	2014-12-22 12:06:18.308911687 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-22 12:06:18.311911822 -0800
@@ -349,6 +349,12 @@ static __user void *task_get_bounds_dir(
 		return MPX_INVALID_BOUNDS_DIR;
 
 	/*
+	 * 32-bit binaries on 64-bit kernels are currently
+	 * unsupported.
+	 */
+	if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
+		return MPX_INVALID_BOUNDS_DIR;
+	/*
 	 * The bounds directory pointer is stored in a register
 	 * only accessible if we first do an xsave.
 	 */
_

Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Andy Lutomirski]

On Mon, Dec 22, 2014 at 12:08 PM, Dave Hansen <dave@sr71.net> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> We had originally planned on submitting MPX support in one patch
> set.  We eventually broke it up in to two pieces for easier
> review.  One of the features that didn't make the first round
> was supporting 32-bit binaries on 64-bit kernels.
>
> Once we split the set up, we never added code to restrict 32-bit
> binaries from _using_ MPX on 64-bit kernels.
>
> The 32-bit bounds tables are a different format than the 64-bit
> ones.  Without this patch, the kernel will try to read a 32-bit
> binary's tables as if they were the 64-bit version.  They will
> likely be noticed as being invalid rather quickly and the app
> will get killed, but that's kinda mean.
>
> This patch adds an explicit check, and will make a 64-bit kernel
> essentially behave as if it has no MPX support when called from
> a 32-bit binary.
>
> I intend to remove this check once the mixed binary support is
> added.  Those patches are posted if anyone is interested:
>
>         https://lkml.org/lkml/2014/12/12/632
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> ---
>
>  b/arch/x86/mm/mpx.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff -puN arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 arch/x86/mm/mpx.c
> --- a/arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19    2014-12-22 12:06:18.308911687 -0800
> +++ b/arch/x86/mm/mpx.c 2014-12-22 12:06:18.311911822 -0800
> @@ -349,6 +349,12 @@ static __user void *task_get_bounds_dir(
>                 return MPX_INVALID_BOUNDS_DIR;
>
>         /*
> +        * 32-bit binaries on 64-bit kernels are currently
> +        * unsupported.
> +        */
> +       if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
> +               return MPX_INVALID_BOUNDS_DIR;

Should this check mm->ia32_compat instead?

Otherwise,

Reviewed-by: Andy Lutomirski <luto@amacapital.net>

> +       /*
>          * The bounds directory pointer is stored in a register
>          * only accessible if we first do an xsave.
>          */
> _



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Dave Hansen]

On 12/22/2014 12:17 PM, Andy Lutomirski wrote:
>> >         /*
>> > +        * 32-bit binaries on 64-bit kernels are currently
>> > +        * unsupported.
>> > +        */
>> > +       if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
>> > +               return MPX_INVALID_BOUNDS_DIR;
> Should this check mm->ia32_compat instead?

set_personality_64bit/ia32() seem to make that and TIF_IA32 awfully
equivalent.  Is there a specific reason for wanting it done this way?

Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Andy Lutomirski]

On Mon, Dec 22, 2014 at 12:21 PM, Dave Hansen <dave@sr71.net> wrote:
> On 12/22/2014 12:17 PM, Andy Lutomirski wrote:
>>> >         /*
>>> > +        * 32-bit binaries on 64-bit kernels are currently
>>> > +        * unsupported.
>>> > +        */
>>> > +       if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
>>> > +               return MPX_INVALID_BOUNDS_DIR;
>> Should this check mm->ia32_compat instead?
>
> set_personality_64bit/ia32() seem to make that and TIF_IA32 awfully
> equivalent.  Is there a specific reason for wanting it done this way?

My general desire to remove various bogus TIF_IA32 references.  But
this is only temporary, so I don't really care.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Dec 22, 2014 at 12:21 PM, Dave Hansen <dave@sr71.net> wrote:
> > On 12/22/2014 12:17 PM, Andy Lutomirski wrote:
> >>> >         /*
> >>> > +        * 32-bit binaries on 64-bit kernels are currently
> >>> > +        * unsupported.
> >>> > +        */
> >>> > +       if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
> >>> > +               return MPX_INVALID_BOUNDS_DIR;
> >> Should this check mm->ia32_compat instead?
> >
> > set_personality_64bit/ia32() seem to make that and TIF_IA32 awfully
> > equivalent.  Is there a specific reason for wanting it done this way?
> 
> My general desire to remove various bogus TIF_IA32 references. 
> [...]

So we generally want to use mm->context.ia32_compat instead of 
TIF_IA32, because in the end TIF_IA32 will go away altogether?

Or do you just want to audit all TIF_IA32 places (because most of 
them are wrong), and using mm->context.ia32_compat where it's 
justified and eliminating TIF_IA32 use is a nice way to document 
that ongoing audit without breaking stuff and such?

> [...]  But this is only temporary, so I don't really care.

New code that touches this area should better use new principles, 
so I have no problem with requiring this, as long as it's well 
explained and logical and desirable to everyone.

Thanks,

	Ingo

Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels

[Andy Lutomirski]

On Tue, Dec 23, 2014 at 1:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Mon, Dec 22, 2014 at 12:21 PM, Dave Hansen <dave@sr71.net> wrote:
>> > On 12/22/2014 12:17 PM, Andy Lutomirski wrote:
>> >>> >         /*
>> >>> > +        * 32-bit binaries on 64-bit kernels are currently
>> >>> > +        * unsupported.
>> >>> > +        */
>> >>> > +       if (IS_ENABLED(CONFIG_X86_64) && test_thread_flag(TIF_IA32))
>> >>> > +               return MPX_INVALID_BOUNDS_DIR;
>> >> Should this check mm->ia32_compat instead?
>> >
>> > set_personality_64bit/ia32() seem to make that and TIF_IA32 awfully
>> > equivalent.  Is there a specific reason for wanting it done this way?
>>
>> My general desire to remove various bogus TIF_IA32 references.
>> [...]
>
> So we generally want to use mm->context.ia32_compat instead of
> TIF_IA32, because in the end TIF_IA32 will go away altogether?
>
> Or do you just want to audit all TIF_IA32 places (because most of
> them are wrong), and using mm->context.ia32_compat where it's
> justified and eliminating TIF_IA32 use is a nice way to document
> that ongoing audit without breaking stuff and such?

TBH, I haven't gotten that far.  But of the uses I've looked at, most
seem to want TS_COMPAT instead (is_ia32_task, which is IMO terribly
named), some are so buggier than just using a strange flag, and I
suspect that the rest would make more sense using
mm->context.ia32_compat.

I was planning on sending some patches to incrementally improve the
situation and then seeing where we end up.

>
>> [...]  But this is only temporary, so I don't really care.
>
> New code that touches this area should better use new principles,
> so I have no problem with requiring this, as long as it's well
> explained and logical and desirable to everyone.
>

Given that this patch is a fix for 3.19, the code looks correct, and
the check will go away in 3.20, I see no reason to worry about this
particular patch.

--Andy

> Thanks,
>
>         Ingo



-- 
Andy Lutomirski
AMA Capital Management, LLC

[PATCH 2/2] x86 mpx: fix potential performance issue on unmaps

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The 3.19 release window saw some TLB modifications merged which
caused a performance regression.  They were fixed in commit
045bbb9fa.

Once that fix was applied, I also noticed that there was a small
but intermittent regression still present.  It was not present
consistently enough to bisect reliably, but I'm fairly confident
that it came from (my own) MPX patches.  The source was reading
a relatively unused field in the mm_struct via arch_unmap.

I also noted that this code was in the main instruction flow of
do_munmap() and probably had more icache impact than we want.

This patch does two things:
1. Adds a static (via Kconfig) and dynamic (via cpuid) check
   for MPX with cpu_feature_enabled().  This keeps us from
   reading that cacheline in the mm and trades it for a check
   of the global CPUID variables at least on CPUs without MPX.
2. Adds an unlikely() to ensure that the MPX call ends up out
   of the main instruction flow in do_munmap().  I've added
   a detailed comment about why this was done and why we want
   it even on systems where MPX is present.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/mmu_context.h |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff -puN arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap	2014-12-22 12:06:18.677928330 -0800
+++ b/arch/x86/include/asm/mmu_context.h	2014-12-22 12:06:18.680928465 -0800
@@ -130,7 +130,25 @@ static inline void arch_bprm_mm_init(str
 static inline void arch_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
 			      unsigned long start, unsigned long end)
 {
-	mpx_notify_unmap(mm, vma, start, end);
+	/*
+	 * mpx_notify_unmap() goes and reads a rarely-hot
+	 * cacheline in the mm_struct.  That can be expensive
+	 * enough to be seen in profiles.
+	 *
+	 * The mpx_notify_unmap() call and its contents have been
+	 * observed to affect munmap() performance on hardware
+	 * where MPX is not present.
+	 *
+	 * The unlikely() optimizes for the fast case: no MPX
+	 * in the CPU, or no MPX use in the process.  Even if
+	 * we get this wrong (in the unlikely event that MPX
+	 * is widely enabled on some system) the overhead of
+	 * MPX itself (reading bounds tables) is expected to
+	 * overwhelm the overhead of getting this unlikely()
+	 * consistently wrong.
+	 */
+	if (unlikely(cpu_feature_enabled(X86_FEATURE_MPX)))
+		mpx_notify_unmap(mm, vma, start, end);
 }
 
 #endif /* _ASM_X86_MMU_CONTEXT_H */
_

Re: [PATCH 2/2] x86 mpx: fix potential performance issue on unmaps

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> The 3.19 release window saw some TLB modifications merged which
> caused a performance regression.  They were fixed in commit
> 045bbb9fa.
> 
> Once that fix was applied, I also noticed that there was a small
> but intermittent regression still present.  It was not present
> consistently enough to bisect reliably, but I'm fairly confident
> that it came from (my own) MPX patches.  The source was reading
> a relatively unused field in the mm_struct via arch_unmap.
> 
> I also noted that this code was in the main instruction flow of
> do_munmap() and probably had more icache impact than we want.
> 
> This patch does two things:
> 1. Adds a static (via Kconfig) and dynamic (via cpuid) check
>    for MPX with cpu_feature_enabled().  This keeps us from
>    reading that cacheline in the mm and trades it for a check
>    of the global CPUID variables at least on CPUs without MPX.
> 2. Adds an unlikely() to ensure that the MPX call ends up out
>    of the main instruction flow in do_munmap().  I've added
>    a detailed comment about why this was done and why we want
>    it even on systems where MPX is present.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/include/asm/mmu_context.h |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff -puN arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap arch/x86/include/asm/mmu_context.h
> --- a/arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap	2014-12-22 12:06:18.677928330 -0800
> +++ b/arch/x86/include/asm/mmu_context.h	2014-12-22 12:06:18.680928465 -0800
> @@ -130,7 +130,25 @@ static inline void arch_bprm_mm_init(str
>  static inline void arch_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
>  			      unsigned long start, unsigned long end)
>  {
> -	mpx_notify_unmap(mm, vma, start, end);
> +	/*
> +	 * mpx_notify_unmap() goes and reads a rarely-hot
> +	 * cacheline in the mm_struct.  That can be expensive
> +	 * enough to be seen in profiles.
> +	 *
> +	 * The mpx_notify_unmap() call and its contents have been
> +	 * observed to affect munmap() performance on hardware
> +	 * where MPX is not present.
> +	 *
> +	 * The unlikely() optimizes for the fast case: no MPX
> +	 * in the CPU, or no MPX use in the process.  Even if
> +	 * we get this wrong (in the unlikely event that MPX
> +	 * is widely enabled on some system) the overhead of
> +	 * MPX itself (reading bounds tables) is expected to
> +	 * overwhelm the overhead of getting this unlikely()
> +	 * consistently wrong.
> +	 */
> +	if (unlikely(cpu_feature_enabled(X86_FEATURE_MPX)))
> +		mpx_notify_unmap(mm, vma, start, end);
>  }

Hm, so this patch still does not help people who have an MPX 
capable CPU but don't have (or don't have many) MPX using apps. 
What about them?

Thanks,

	Ingo

Re: [PATCH 2/2] x86 mpx: fix potential performance issue on unmaps

[Dave Hansen]

On 12/23/2014 01:14 AM, Ingo Molnar wrote:
>> >  {
>> > -	mpx_notify_unmap(mm, vma, start, end);
>> > +	/*
>> > +	 * mpx_notify_unmap() goes and reads a rarely-hot
>> > +	 * cacheline in the mm_struct.  That can be expensive
>> > +	 * enough to be seen in profiles.
>> > +	 *
>> > +	 * The mpx_notify_unmap() call and its contents have been
>> > +	 * observed to affect munmap() performance on hardware
>> > +	 * where MPX is not present.
>> > +	 *
>> > +	 * The unlikely() optimizes for the fast case: no MPX
>> > +	 * in the CPU, or no MPX use in the process.  Even if
>> > +	 * we get this wrong (in the unlikely event that MPX
>> > +	 * is widely enabled on some system) the overhead of
>> > +	 * MPX itself (reading bounds tables) is expected to
>> > +	 * overwhelm the overhead of getting this unlikely()
>> > +	 * consistently wrong.
>> > +	 */
>> > +	if (unlikely(cpu_feature_enabled(X86_FEATURE_MPX)))
>> > +		mpx_notify_unmap(mm, vma, start, end);
>> >  }
> Hm, so this patch still does not help people who have an MPX 
> capable CPU but don't have (or don't have many) MPX using apps. 
> What about them?

Sorry for the delayed resposne.

The performance regression, as far as I could tell, was the result of a
consistent branch misprediction near the read of mm->bd_addr.  I believe
the CPU was able to better predict cpu_feature_enabled() than the
contents of mm->bd_addr.

In running this on a CPU which actually contains MPX, I wasn't able to
see the same regression.  The same branch was getting predicted correctly.

I also have a patch to add a global, boot-time MPX disable.  It will
clear out the X86_FEATURE_MPX at __setup time.  While not optimal, this
would at least let someone who did not have any MPX apps avoid any
potential issues.  I was planning on submitting that patch for 3.20.