Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On 07/27/2015 10:29 PM, Andy Lutomirski wrote: > modify_ldt has questionable locking and does not synchronize > threads. Improve it: redesign the locking and synchronize all > threads' LDTs using an IPI on all modifications. > > This will dramatically slow down modify_ldt in multithreaded > programs, but there shouldn't be any multithreaded programs that > care about modify_ldt's performance in the first place. > ... except 32-bit programs compiled with one specific version of glibc. Do we care? I don't think so. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On 07/30/2015 03:25 PM, Andy Lutomirski wrote: On Thu, Jul 30, 2015 at 11:35 AM, Boris Ostrovsky wrote: [OK]LDT entry 0 has AR 0x0040F200 and limit 0x [OK]LDT entry 0 is invalid [RUN] Cross-CPU LDT invalidation Segmentation fault (core dumped) That's not good. Can you backtrace it? (I.e. compite ldt_gdt_32 with -g and load the core dumb in gdb?) My best guesses are either a signal delivery failure (although that shouldn't be a problem for 32-bit userspace on any kernel) or an actual LDT access fault, and the latter would be interesting. I haven't been able to reproduce this. This looks like a userspace bug. Breaks on F18, works fine in F22. Possibly something about signal handling --- I noticed on F18 I'd get two SEGV's in a row whereas we should only get one. Anyway, this is not an issue as far as this thread is concerned. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On Thu, Jul 30, 2015 at 11:35 AM, Boris Ostrovsky wrote: > On 07/30/2015 02:14 PM, Andy Lutomirski wrote: >> >> On Thu, Jul 30, 2015 at 10:56 AM, Boris Ostrovsky >> wrote: >>> >>> On 07/28/2015 01:29 AM, Andy Lutomirski wrote: >>> + +static inline void load_mm_ldt(struct mm_struct *mm) +{ + struct ldt_struct *ldt; + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); >>> >>> >>> >>> I thought this was supposed to be checking preemptible()? >> >> v6 fixes that. Check your future inbox :) I'm goint to rework the >> Xen bit too based on the long discussion. >> >> Is that the only failure you're seeing? > > > Yes. > >> ldt_gdt_32 passes on 64-bit for me > > > With your patch: > > root@haswell> uname -a > Linux dhcp-burlington7-2nd-B-east-10-152-55-89.usdhcp.oraclecorp.com > 4.2.0-rc4 #107 SMP Thu Jul 30 11:05:19 EDT 2015 x86_64 x86_64 x86_64 > GNU/Linux > root@haswell> cd tmp/linux/tools/testing/selftests/x86/ > root@haswell> ls -l ldt_gdt_32 > -rwxr-xr-x 1 root root 25975 Jul 30 11:48 ldt_gdt_32 > root@haswell> ./ldt_gdt_32 > [OK]LDT entry 0 has AR 0x0040FA00 and limit 0x000A > [OK]LDT entry 0 has AR 0x00C0FA00 and limit 0xAFFF > [OK]LDT entry 1 is invalid > [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF > [OK]LDT entry 1 is invalid > [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00D0FA00 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00D07A00 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00907A00 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00D07200 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00D07000 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00D07400 and limit 0xAFFF > [OK]LDT entry 2 has AR 0x00507600 and limit 0x000A > [OK]LDT entry 2 has AR 0x00507E00 and limit 0x000A > [OK]LDT entry 2 has AR 0x00507C00 and limit 0x000A > [OK]LDT entry 2 has AR 0x00507A00 and limit 0x000A > [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A > [RUN] Test fork > [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A > [OK]LDT entry 1 is invalid > [OK]Child succeeded > [RUN] Test size > [DONE] Size test > [OK]modify_ldt failure 22 > [OK]LDT entry 0 has AR 0xF200 and limit 0x > [OK]LDT entry 0 has AR 0x7200 and limit 0x > [OK]LDT entry 0 has AR 0xF000 and limit 0x > [OK]LDT entry 0 has AR 0x7200 and limit 0x > [OK]LDT entry 0 has AR 0x7000 and limit 0x0001 > [OK]LDT entry 0 has AR 0x7000 and limit 0x > [OK]LDT entry 0 is invalid > [OK]LDT entry 0 has AR 0x0040F200 and limit 0x > [OK]LDT entry 0 is invalid > [RUN] Cross-CPU LDT invalidation > Segmentation fault (core dumped) That's not good. Can you backtrace it? (I.e. compite ldt_gdt_32 with -g and load the core dumb in gdb?) My best guesses are either a signal delivery failure (although that shouldn't be a problem for 32-bit userspace on any kernel) or an actual LDT access fault, and the latter would be interesting. I haven't been able to reproduce this. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On 07/30/2015 02:14 PM, Andy Lutomirski wrote: On Thu, Jul 30, 2015 at 10:56 AM, Boris Ostrovsky wrote: On 07/28/2015 01:29 AM, Andy Lutomirski wrote: + +static inline void load_mm_ldt(struct mm_struct *mm) +{ + struct ldt_struct *ldt; + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); I thought this was supposed to be checking preemptible()? v6 fixes that. Check your future inbox :) I'm goint to rework the Xen bit too based on the long discussion. Is that the only failure you're seeing? Yes. ldt_gdt_32 passes on 64-bit for me With your patch: root@haswell> uname -a Linux dhcp-burlington7-2nd-B-east-10-152-55-89.usdhcp.oraclecorp.com 4.2.0-rc4 #107 SMP Thu Jul 30 11:05:19 EDT 2015 x86_64 x86_64 x86_64 GNU/Linux root@haswell> cd tmp/linux/tools/testing/selftests/x86/ root@haswell> ls -l ldt_gdt_32 -rwxr-xr-x 1 root root 25975 Jul 30 11:48 ldt_gdt_32 root@haswell> ./ldt_gdt_32 [OK]LDT entry 0 has AR 0x0040FA00 and limit 0x000A [OK]LDT entry 0 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 1 is invalid [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 1 is invalid [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D0FA00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07A00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00907A00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07200 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07000 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07400 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00507600 and limit 0x000A [OK]LDT entry 2 has AR 0x00507E00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507C00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507A00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A [RUN] Test fork [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A [OK]LDT entry 1 is invalid [OK]Child succeeded [RUN] Test size [DONE] Size test [OK]modify_ldt failure 22 [OK]LDT entry 0 has AR 0xF200 and limit 0x [OK]LDT entry 0 has AR 0x7200 and limit 0x [OK]LDT entry 0 has AR 0xF000 and limit 0x [OK]LDT entry 0 has AR 0x7200 and limit 0x [OK]LDT entry 0 has AR 0x7000 and limit 0x0001 [OK]LDT entry 0 has AR 0x7000 and limit 0x [OK]LDT entry 0 is invalid [OK]LDT entry 0 has AR 0x0040F200 and limit 0x [OK]LDT entry 0 is invalid [RUN] Cross-CPU LDT invalidation Segmentation fault (core dumped) root@haswell> dmesg | grep -i xen [2.953815] xenfs: not registering filesystem on non-xen platform [ 17.495141] IPv6: ADDRCONF(NETDEV_UP): xenbr0: link is not ready [ 20.913839] xenbr0: port 1(eth0) entered forwarding state [ 20.913907] xenbr0: port 1(eth0) entered forwarding state [ 20.914044] IPv6: ADDRCONF(NETDEV_CHANGE): xenbr0: link becomes ready On a slightly older kernel: root@haswell> uname -a Linux dhcp-burlington7-2nd-B-east-10-152-55-89.usdhcp.oraclecorp.com 4.1.0-rc2 #111 SMP Fri Jun 19 16:28:46 EDT 2015 x86_64 x86_64 x86_64 GNU/Linux root@haswell> cd tmp/linux/tools/testing/selftests/x86/ root@haswell> ls -l ldt_gdt_32 -rwxr-xr-x 1 root root 25975 Jul 30 11:48 ldt_gdt_32 root@haswell> ./ldt_gdt_32 [OK]LDT entry 0 has AR 0x0040FA00 and limit 0x000A [OK]LDT entry 0 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 1 is invalid [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 1 is invalid [OK]LDT entry 2 has AR 0x00C0FA00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D0FA00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07A00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00907A00 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07200 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07000 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00D07400 and limit 0xAFFF [OK]LDT entry 2 has AR 0x00507600 and limit 0x000A [OK]LDT entry 2 has AR 0x00507E00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507C00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507A00 and limit 0x000A [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A [RUN] Test fork [OK]LDT entry 2 has AR 0x00507800 and limit 0x000A [OK]LDT entry 1 is invalid [OK]Child succeeded [RUN] Test size [DONE] Size test [OK]modify_ldt failure 22 [OK]LDT entry 0 has AR 0xF200 and limit 0x [OK]LDT entry 0 has AR 0x7200 and limit 0x [OK]LDT entry 0 has AR 0xF000 and limit 0x [OK]LDT entry 0 has AR 0x7200 and limit 0x [OK]LDT entry 0 has AR 0x7000 and limit 0x0001 [OK]LDT entry 0 has AR 0x7000 and limit 0x [OK]LDT entry 0 is invalid [OK]LDT entry 0 has AR 0x0040F200 and limit 0x [OK]LDT entry 0 is invalid [RUN] Cross-CPU LDT invalidation [FA
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On Thu, Jul 30, 2015 at 10:56 AM, Boris Ostrovsky wrote: > On 07/28/2015 01:29 AM, Andy Lutomirski wrote: > >> + >> +static inline void load_mm_ldt(struct mm_struct *mm) >> +{ >> + struct ldt_struct *ldt; >> + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); > > > > I thought this was supposed to be checking preemptible()? v6 fixes that. Check your future inbox :) I'm goint to rework the Xen bit too based on the long discussion. Is that the only failure you're seeing? ldt_gdt_32 passes on 64-bit for me > > -boris -- Andy Lutomirski AMA Capital Management, LLC ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On 07/28/2015 01:29 AM, Andy Lutomirski wrote: + +static inline void load_mm_ldt(struct mm_struct *mm) +{ + struct ldt_struct *ldt; + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); I thought this was supposed to be checking preemptible()? -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
On Mon, Jul 27, 2015 at 10:29:39PM -0700, Andy Lutomirski wrote: > modify_ldt has questionable locking and does not synchronize > threads. Improve it: redesign the locking and synchronize all > threads' LDTs using an IPI on all modifications. > > This will dramatically slow down modify_ldt in multithreaded > programs, but there shouldn't be any multithreaded programs that > care about modify_ldt's performance in the first place. > > Cc: sta...@vger.kernel.org > Signed-off-by: Andy Lutomirski I've stared a lot at this one these days, I guess a Reviewed-by: Borislav Petkov is in order. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 2/4] x86/ldt: Make modify_ldt synchronous
modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications. This will dramatically slow down modify_ldt in multithreaded programs, but there shouldn't be any multithreaded programs that care about modify_ldt's performance in the first place. Cc: sta...@vger.kernel.org Signed-off-by: Andy Lutomirski --- arch/x86/include/asm/desc.h| 15 --- arch/x86/include/asm/mmu.h | 3 +- arch/x86/include/asm/mmu_context.h | 53 +++- arch/x86/kernel/cpu/common.c | 4 +- arch/x86/kernel/cpu/perf_event.c | 12 +- arch/x86/kernel/ldt.c | 262 - arch/x86/kernel/process_64.c | 4 +- arch/x86/kernel/step.c | 6 +- arch/x86/power/cpu.c | 3 +- 9 files changed, 209 insertions(+), 153 deletions(-) diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h index a0bf89fd2647..4e10d73cf018 100644 --- a/arch/x86/include/asm/desc.h +++ b/arch/x86/include/asm/desc.h @@ -280,21 +280,6 @@ static inline void clear_LDT(void) set_ldt(NULL, 0); } -/* - * load one particular LDT into the current CPU - */ -static inline void load_LDT_nolock(mm_context_t *pc) -{ - set_ldt(pc->ldt, pc->size); -} - -static inline void load_LDT(mm_context_t *pc) -{ - preempt_disable(); - load_LDT_nolock(pc); - preempt_enable(); -} - static inline unsigned long get_desc_base(const struct desc_struct *desc) { return (unsigned)(desc->base0 | ((desc->base1) << 16) | ((desc->base2) << 24)); diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h index 09b9620a73b4..364d27481a52 100644 --- a/arch/x86/include/asm/mmu.h +++ b/arch/x86/include/asm/mmu.h @@ -9,8 +9,7 @@ * we put the segment information here. */ typedef struct { - void *ldt; - int size; + struct ldt_struct *ldt; #ifdef CONFIG_X86_64 /* True if mm supports a task running in 32 bit compatibility mode. */ diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h index 804a3a6030ca..3fcff70c398e 100644 --- a/arch/x86/include/asm/mmu_context.h +++ b/arch/x86/include/asm/mmu_context.h @@ -34,6 +34,49 @@ static inline void load_mm_cr4(struct mm_struct *mm) {} #endif /* + * ldt_structs can be allocated, used, and freed, but they are never + * modified while live. + */ +struct ldt_struct { + /* +* Xen requires page-aligned LDTs with special permissions. This is +* needed to prevent us from installing evil descriptors such as +* call gates. On native, we could merge the ldt_struct and LDT +* allocations, but it's not worth trying to optimize. +*/ + struct desc_struct *entries; + int size; +}; + +static inline void load_mm_ldt(struct mm_struct *mm) +{ + struct ldt_struct *ldt; + DEBUG_LOCKS_WARN_ON(!irqs_disabled()); + + /* lockless_dereference synchronizes with smp_store_release */ + ldt = lockless_dereference(mm->context.ldt); + + /* +* Any change to mm->context.ldt is followed by an IPI to all +* CPUs with the mm active. The LDT will not be freed until +* after the IPI is handled by all such CPUs. This means that, +* if the ldt_struct changes before we return, the values we see +* will be safe, and the new values will be loaded before we run +* any user code. +* +* NB: don't try to convert this to use RCU without extreme care. +* We would still need IRQs off, because we don't want to change +* the local LDT after an IPI loaded a newer value than the one +* that we can see. +*/ + + if (unlikely(ldt)) + set_ldt(ldt->entries, ldt->size); + else + clear_LDT(); +} + +/* * Used for LDT copy/destruction. */ int init_new_context(struct task_struct *tsk, struct mm_struct *mm); @@ -78,12 +121,12 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next, * was called and then modify_ldt changed * prev->context.ldt but suppressed an IPI to this CPU. * In this case, prev->context.ldt != NULL, because we -* never free an LDT while the mm still exists. That -* means that next->context.ldt != prev->context.ldt, -* because mms never share an LDT. +* never set context.ldt to NULL while the mm still +* exists. That means that next->context.ldt != +* prev->context.ldt, because mms never share an LDT. */ if (unlikely(prev->context.ldt != next->context.ldt)) - load_LDT_nolock(&next->context); + load_mm_ldt(next); } #ifdef CONFIG_SMP else { @@ -106,7 +149,7 @@ static inline void