ldt handling patch

[David Holland <dholland-tech%netbsd.org@localhost>]

This (still to be tested) fixes two things:
   1. if uvm_km_alloc fails while forking, we dereference null;
   2. owing to a missing memory barrier when updating the ldt,
      the fork code can read -1 as the length of a nonempty ldt,
      leading to uvm_km_alloc failure.

Both of these problems are somewhat speculative; coypu has been seeing
crashes, but with symptoms that aren't really consistent with the
above.

(please Cc: me as I'm not actually subscribed to port-i386)

Index: pmap.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/pmap.c,v
retrieving revision 1.221
diff -u -r1.221 pmap.c
--- pmap.c	27 Aug 2016 16:07:26 -0000	1.221
+++ pmap.c	18 Sep 2016 22:20:01 -0000
@@ -2443,30 +2443,49 @@
 		return;
 	}
 
+	/*
+	 * Copy the LDT into the new process.
+	 *
+	 * Read pmap1's ldt pointer and length unlocked; if it changes
+	 * behind our back we'll retry. This will starve if there's a
+	 * stream of LDT changes in another thread but that should not
+	 * happen.
+	 */
+
  retry:
 	if (pmap1->pm_ldt != NULL) {
 		len = pmap1->pm_ldt_len;
+		/* Allocate space for the new process's LDT */
 		new_ldt = (union descriptor *)uvm_km_alloc(kernel_map, len, 0,
 		    UVM_KMF_WIRED);
+		if (new_ldt == NULL) {
+			printf("WARNING: pmap_fork: "
+			       "unable to allocate LDT space\n");
+			return;
+		}
 		mutex_enter(&cpu_lock);
+		/* Get a GDT slot for it */
 		sel = ldt_alloc(new_ldt, len);
 		if (sel == -1) {
 			mutex_exit(&cpu_lock);
 			uvm_km_free(kernel_map, (vaddr_t)new_ldt, len,
 			    UVM_KMF_WIRED);
-			printf("WARNING: pmap_fork: unable to allocate LDT\n");
+			printf("WARNING: pmap_fork: "
+			       "unable to allocate LDT selector\n");
 			return;
 		}
 	} else {
+		/* Wasn't anything there after all. */
 		len = -1;
 		new_ldt = NULL;
 		sel = -1;
 		mutex_enter(&cpu_lock);
 	}
 
- 	/* Copy the LDT, if necessary. */
+ 	/* If there's still something there now that we have cpu_lock... */
  	if (pmap1->pm_ldt != NULL) {
 		if (len != pmap1->pm_ldt_len) {
+			/* Oops, it changed. Drop what we did and try again */
 			if (len != -1) {
 				ldt_free(sel);
 				uvm_km_free(kernel_map, (vaddr_t)new_ldt,
@@ -2476,6 +2495,7 @@
 			goto retry;
 		}
 
+		/* Copy the LDT data and install it in pmap2 */
 		memcpy(new_ldt, pmap1->pm_ldt, len);
 		pmap2->pm_ldt = new_ldt;
 		pmap2->pm_ldt_len = pmap1->pm_ldt_len;
@@ -2484,7 +2504,9 @@
 	}
 
 	if (len != -1) {
+		/* There wasn't still something there, so mop up */
 		ldt_free(sel);
+		/* XXX shouldn't this not be under cpu_lock? */
 		uvm_km_free(kernel_map, (vaddr_t)new_ldt, len,
 		    UVM_KMF_WIRED);
 	}
Index: sys_machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/sys_machdep.c,v
retrieving revision 1.29
diff -u -r1.29 sys_machdep.c
--- sys_machdep.c	23 Oct 2015 18:53:26 -0000	1.29
+++ sys_machdep.c	18 Sep 2016 22:20:01 -0000
@@ -365,9 +365,11 @@
 	}
 
 	/* All changes are now globally visible.  Swap in the new LDT. */
-	pmap->pm_ldt = new_ldt;
 	pmap->pm_ldt_len = new_len;
 	pmap->pm_ldt_sel = new_sel;
+	/* membar_store_store for pmap_fork() to read these unlocked safely */
+	membar_producer();
+	pmap->pm_ldt = new_ldt;
 
 	/* Switch existing users onto new LDT. */
 	pmap_ldt_sync(pmap);
@@ -375,10 +377,13 @@
 	/* Free existing LDT (if any). */
 	if (old_ldt != NULL) {
 		ldt_free(old_sel);
+		/* exit the mutex before free */
+		mutex_exit(&cpu_lock);
 		uvm_km_free(kernel_map, (vaddr_t)old_ldt, old_len,
 		    UVM_KMF_WIRED);
+	} else {
+		mutex_exit(&cpu_lock);
 	}
-	mutex_exit(&cpu_lock);
 
 	return error;
 #endif


-- 
David A. Holland
dholland%netbsd.org@localhost