Actual code shows several problems in ia32 LDT handling:

- When a LDT entry changes, the old one is freed while it is still
  referenced by gdt and ldtr.  This can lead to disruptive behaviours in
  particular on SMP machines.
- When a LDT entry changes, it is assumed that the only one entity sharing
  the same LDT are threads in the same proc.  It doesn't take in account
  edge cases where two processes share the same VM (rfork'ed ones, for
  example).

This patch addresses these two problems and addictionally it fixes the
usage of refcount switching back it to the old manually-grown refcount
(since in this case would be faster).

Diagnosed by: tegge
Tested by: pho (a former version)
Reviewed by: kib
Approved by: jeff (mentor)
Approved by: re

2 files changed, 39 insertions, 34 deletions

diff --git a/sys/i386/i386/sys_machdep.c b/sys/i386/i386/sys_machdep.c
index 31fad62..450018d 100644
--- a/sys/i386/i386/sys_machdep.c
+++ b/sys/i386/i386/sys_machdep.c
@@ -42,7 +42,6 @@ __FBSDID("$FreeBSD$");
 #include <sys/mutex.h>
 #include <sys/priv.h>
 #include <sys/proc.h>
-#include <sys/refcount.h>
 #include <sys/smp.h>
 #include <sys/sysproto.h>
 
@@ -66,14 +65,15 @@ __FBSDID("$FreeBSD$");
 #define NEW_MAX_LD(num)  ((num + LD_PER_PAGE) & ~(LD_PER_PAGE-1))
 #define SIZE_FROM_LARGEST_LD(num) (NEW_MAX_LD(num) << 3)
 
+#ifdef SMP
+#define	NULL_LDT_BASE	((caddr_t)NULL)
 
+static void set_user_ldt_rv(struct vmspace *vmsp);
+#endif
 
 static int i386_set_ldt_data(struct thread *, int start, int num,
 	union descriptor *descs);
 static int i386_ldt_grow(struct thread *td, int len);
-#ifdef SMP
-static void set_user_ldt_rv(struct thread *);
-#endif
 
 #ifndef _SYS_SYSPROTO_H_
 struct sysarch_args {
@@ -378,10 +378,12 @@ set_user_ldt(struct mdproc *mdp)
 
 #ifdef SMP
 static void
-set_user_ldt_rv(struct thread *td)
+set_user_ldt_rv(struct vmspace *vmsp)
 {
+	struct thread *td;
 
-	if (td->td_proc != curthread->td_proc)
+	td = curthread;
+	if (vmsp != td->td_proc->p_vmspace)
 		return;
 
 	set_user_ldt(&td->td_proc->p_md);
@@ -408,7 +410,7 @@ user_ldt_alloc(struct mdproc *mdp, int len)
 		FREE(new_ldt, M_SUBPROC);
 		return NULL;
 	}
-	refcount_init(&new_ldt->ldt_refcnt, 1);
+	new_ldt->ldt_refcnt = 1;
 	new_ldt->ldt_active = 0;
 
 	mtx_lock_spin(&dt_lock);
@@ -446,12 +448,13 @@ user_ldt_free(struct thread *td)
 	}
 
 	mdp->md_ldt = NULL;
-	mtx_unlock_spin(&dt_lock);
-	if (refcount_release(&pldt->ldt_refcnt)) {
+	if (--pldt->ldt_refcnt == 0) {
+		mtx_unlock_spin(&dt_lock);
 		kmem_free(kernel_map, (vm_offset_t)pldt->ldt_base,
 			pldt->ldt_len * sizeof(union descriptor));
 		FREE(pldt, M_SUBPROC);
-	}
+	} else
+		mtx_unlock_spin(&dt_lock);
 }
 
 /*
@@ -690,9 +693,9 @@ static int
 i386_ldt_grow(struct thread *td, int len) 
 {
 	struct mdproc *mdp = &td->td_proc->p_md;
-	struct proc_ldt *pldt;
-	caddr_t old_ldt_base;
-	int old_ldt_len;
+	struct proc_ldt *new_ldt, *pldt;
+	caddr_t old_ldt_base = NULL_LDT_BASE;
+	int old_ldt_len = 0;
 
 	mtx_assert(&dt_lock, MA_OWNED);
 
@@ -703,29 +706,16 @@ i386_ldt_grow(struct thread *td, int len)
 
 	/* Allocate a user ldt. */
 	if ((pldt = mdp->md_ldt) == NULL || len > pldt->ldt_len) {
-		struct proc_ldt *new_ldt;
-
 		new_ldt = user_ldt_alloc(mdp, len);
 		if (new_ldt == NULL)
 			return (ENOMEM);
 		pldt = mdp->md_ldt;
 
 		if (pldt != NULL) {
-			if (new_ldt->ldt_len > pldt->ldt_len) {
-				old_ldt_base = pldt->ldt_base;
-				old_ldt_len = pldt->ldt_len;
-				pldt->ldt_sd = new_ldt->ldt_sd;
-				pldt->ldt_base = new_ldt->ldt_base;
-				pldt->ldt_len = new_ldt->ldt_len;
-				mtx_unlock_spin(&dt_lock);
-				kmem_free(kernel_map, (vm_offset_t)old_ldt_base,
-					old_ldt_len * sizeof(union descriptor));
-				FREE(new_ldt, M_SUBPROC);
-				mtx_lock_spin(&dt_lock);
-			} else {
+			if (new_ldt->ldt_len <= pldt->ldt_len) {
 				/*
-				 * If other threads already did the work,
-				 * do nothing.
+				 * We just lost the race for allocation, so
+				 * free the new object and return.
 				 */
 				mtx_unlock_spin(&dt_lock);
 				kmem_free(kernel_map,
@@ -735,6 +725,16 @@ i386_ldt_grow(struct thread *td, int len)
 				mtx_lock_spin(&dt_lock);
 				return (0);
 			}
+
+			/*
+			 * We have to substitute the current LDT entry for
+			 * curproc with the new one since its size grew.
+			 */
+			old_ldt_base = pldt->ldt_base;
+			old_ldt_len = pldt->ldt_len;
+			pldt->ldt_sd = new_ldt->ldt_sd;
+			pldt->ldt_base = new_ldt->ldt_base;
+			pldt->ldt_len = new_ldt->ldt_len;
 		} else
 			mdp->md_ldt = pldt = new_ldt;
 #ifdef SMP
@@ -746,11 +746,17 @@ i386_ldt_grow(struct thread *td, int len)
 		 */
 		mtx_unlock_spin(&dt_lock);
 		smp_rendezvous(NULL, (void (*)(void *))set_user_ldt_rv,
-		    NULL, td);
-		mtx_lock_spin(&dt_lock);
+		    NULL, td->td_proc->p_vmspace);
 #else
-		set_user_ldt(mdp);
+		set_user_ldt(td);
+		mtx_unlock_spin(&dt_lock);
 #endif
+		if (old_ldt_base != NULL_LDT_BASE) {
+			kmem_free(kernel_map, (vm_offset_t)old_ldt_base,
+			    old_ldt_len * sizeof(union descriptor));
+			FREE(new_ldt, M_SUBPROC);
+		}
+		mtx_lock_spin(&dt_lock);
 	}
 	return (0);
 }
diff --git a/sys/i386/i386/vm_machdep.c b/sys/i386/i386/vm_machdep.c
index 48e19ae..bbcdddf 100644
--- a/sys/i386/i386/vm_machdep.c
+++ b/sys/i386/i386/vm_machdep.c
@@ -62,7 +62,6 @@ __FBSDID("$FreeBSD$");
 #include <sys/mutex.h>
 #include <sys/pioctl.h>
 #include <sys/proc.h>
-#include <sys/refcount.h>
 #include <sys/sf_buf.h>
 #include <sys/smp.h>
 #include <sys/sched.h>
@@ -254,7 +253,7 @@ cpu_fork(td1, p2, td2, flags)
 	mtx_lock_spin(&dt_lock);
 	if (mdp2->md_ldt != NULL) {
 		if (flags & RFMEM) {
-			refcount_acquire(&mdp2->md_ldt->ldt_refcnt);
+			mdp2->md_ldt->ldt_refcnt++;
 		} else {
 			mdp2->md_ldt = user_ldt_alloc(mdp2,
 			    mdp2->md_ldt->ldt_len);