x86, mpx: Fixes for 3.19
cover x86, mpx: Fixes for 3.19
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
From: Dave Hansen <[email protected]>
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
---
b/arch/x86/mm/mpx.c | 6 ++++++ 1 file changed, 6 insertions(+)
arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 +6 -0
@@ -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
Quoted 37 lines · Dave Hansen
From: Dave Hansen <[email protected]>
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/632Signed-off-by Dave Hansen <[email protected]>Cc Andy Lutomirski <[email protected]>---
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_19arch/x86/mm/mpx.c
arch/x86/mm/mpx.c 2014-12-22 12:06:18.311911822 -0800 static __user void *task_get_bounds_dir( · line 356
Quoted 8 lines
arch/x86/mm/mpx.c 2014-12-22 12:06:18.311911822 -0800 renamed from 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 +5
-0@@ -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,
Quoted 5 lines
arch/x86/mm/mpx.c 2014-12-22 12:06:18.311911822 -0800 renamed from 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 +1
-0+ /* * The bounds directory pointer is stored in a register * only accessible if we first do an xsave. */_
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels
Quoted 7 lines · Andy Lutomirski
Quoted 6 lines
arch/x86/mm/mpx.c static __user void *task_get_bounds_dir( · line 356
Quoted 6 lines
/*arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 +5
-0+* 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_compatinstead?
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
Quoted 11 lines · Dave Hansen
Quoted 7 lines · Andy Lutomirski
Quoted 6 lines
arch/x86/mm/mpx.c static __user void *task_get_bounds_dir( · line 356
Quoted 6 lines
/*arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 +5
-0+* 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_compatinstead?set_personality_64bit/ia32() seem to make that and
TIF_IA32awfully 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
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 1/2] x86, mpx: explicitly disable 32-bit MPX support on 64-bit kernels
Quoted 8 lines · Andy Lutomirski
Quoted 7 lines · Dave Hansen
Quoted 6 lines · Andy Lutomirski
Quoted 6 lines
Quoted 6 lines
/*arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 +5
-0+* 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 ;
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Should this check
mm->ia32_compatinstead?set_personality_64bit/ia32() seem to make that and
TIF_IA32awfully equivalent. Is there a specific reason for wanting it done this way?My general desire to remove various bogus
TIF_IA32references. [...]
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?
Quoted 1 line
[...] 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
Quoted 19 lines · Ingo Molnar
Quoted 8 lines · Andy Lutomirski
Quoted 7 lines · Dave Hansen
Quoted 6 lines · Andy Lutomirski
Quoted 6 lines
Quoted 6 lines
/*arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-explicitly-disable-32-bit-on-64-bit-for-3_19 +5
-0+* 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 ;Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Should this check
mm->ia32_compatinstead?set_personality_64bit/ia32() seem to make that and
TIF_IA32awfully equivalent. Is there a specific reason for wanting it done this way?My general desire to remove various bogus
TIF_IA32references. [...]
Quoted 7 lines
So we generally want to use
mm->context.ia32_compatinstead ofTIF_IA32, because in the endTIF_IA32will go away altogether?Or do you just want to audit all
TIF_IA32places (because most of them are wrong), and usingmm->context.ia32_compatwhere it's justified and eliminatingTIF_IA32use 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.
Quoted 7 lines
Quoted 2 lines
[...] 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
Quoted 3 lines
Thanks,
Ingo
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
PATCH 2/2 x86 mpx: fix potential performance issue on unmaps
In reply to Dave HansenFrom: Dave Hansen <[email protected]>
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.
---
b/arch/x86/include/asm/mmu_context.h | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
arch/x86/include/asm/mmu_context.h renamed from arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap +19 -1
@@ -130,7 +130,25 @@ static inline void arch_bprm_mm_init(strstatic 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
Quoted 50 lines · Dave Hansen
From: Dave Hansen <[email protected]>
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_structviaarch_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 (viaKconfig) and dynamic (via cpuid) check for MPX withcpu_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 anunlikely()to ensure that the MPX call ends up out of the main instruction flow indo_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 <[email protected]>---
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-unmaparch/x86/include/asm/mmu_context.harch/x86/include/asm/mmu_context.h renamed from arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap +10
-1@@ -130,7 +130,25 @@ static inline void arch_bprm_mm_init(strstatic 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
arch/x86/include/asm/mmu_context.h static inline void arch_bprm_mm_init(str · line 152
Quoted 10 lines
arch/x86/include/asm/mmu_context.h renamed from arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap +9
-0+* 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
Quoted 17 lines · Ingo Molnar
Quoted 17 lines
Quoted 17 lines
{
arch/x86/include/asm/mmu_context.h renamed from arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap +15
-1-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()
Quoted 8 lines
Quoted 5 lines
arch/x86/include/asm/mmu_context.h static inline void arch_bprm_mm_init(str · line 152
Quoted 5 lines
arch/x86/include/asm/mmu_context.h renamed from arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap +4
-0+* 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.