PERFORCE change 101488 for review

[John Baldwin]

http://perforce.freebsd.org/chv.cgi?CH=101488

Change 101488 by jhb at jhb_mutex on 2006/07/13 20:50:08

	- Add conditional VFS Giant locking to linux_uselib() and mark it
	  MPSAFE.
	- Remove a Doesn't-Ever-Happen(tm) error case.
	- Hold the vnode lock longer to protect setting VV_TEXT and note that
	  we don't clear it later on if we get an error.
	- Document where the error return values differ from
	  exec_check_permissions() and add a competing XXX to rwatson's
	  explaining that vn_open() would be a poor choice here.  This
	  code is much closer to exec_check_permissions().
	- Audit the pathname lookup for the library.

Affected files ...

.. //depot/projects/smpng/sys/compat/linux/linux_misc.c#66 edit
.. //depot/projects/smpng/sys/i386/linux/syscalls.master#35 edit
.. //depot/projects/smpng/sys/notes#79 edit

Differences ...

==== //depot/projects/smpng/sys/compat/linux/linux_misc.c#66 (text+ko) ====

@@ -229,7 +229,7 @@
 	unsigned long bss_size;
 	char *library;
 	int error;
-	int locked;
+	int locked, vfslocked;
 
 	LCONVPATHEXIST(td, args->library, &library);
 
@@ -239,32 +239,24 @@
 #endif
 
 	a_out = NULL;
+	vfslocked = 0;
 	locked = 0;
 	vp = NULL;
 
-	/*
-	 * XXX: This code should make use of vn_open(), rather than doing
-	 * all this stuff itself.
-	 */
-	NDINIT(&ni, LOOKUP, ISOPEN|FOLLOW|LOCKLEAF, UIO_SYSSPACE, library, td);
+	NDINIT(&ni, LOOKUP, ISOPEN | FOLLOW | LOCKLEAF | MPSAFE | AUDITVNODE1,
+	    UIO_SYSSPACE, library, td);
 	error = namei(&ni);
 	LFREEPATH(library);
 	if (error)
 		goto cleanup;
 
 	vp = ni.ni_vp;
-	/*
-	 * XXX - This looks like a bogus check. A LOCKLEAF namei should not
-	 * succeed without returning a vnode.
-	 */
-	if (vp == NULL) {
-		error = ENOEXEC;	/* ?? */
-		goto cleanup;
-	}
+	vfslocked = NDHASGIANT(&ni);
 	NDFREE(&ni, NDF_ONLY_PNBUF);
 
 	/*
 	 * From here on down, we have a locked vnode that must be unlocked.
+	 * XXX: The code below largely duplicates exec_check_permissions().
 	 */
 	locked++;
 
@@ -281,6 +273,7 @@
 
 	if ((vp->v_mount->mnt_flag & MNT_NOEXEC) ||
 	    ((attr.va_mode & 0111) == 0) || (attr.va_type != VREG)) {
+		/* EACCESS is what exec(2) returns. */
 		error = ENOEXEC;
 		goto cleanup;
 	}
@@ -299,6 +292,8 @@
 	/*
 	 * XXX: This should use vn_open() so that it is properly authorized,
 	 * and to reduce code redundancy all over the place here.
+	 * XXX: Not really, it duplicates far more of exec_check_permissions()
+	 * than vn_open().
 	 */
 #ifdef MAC
 	error = mac_check_vnode_open(td->td_ucred, vp, FREAD);
@@ -312,12 +307,6 @@
 	/* Pull in executable header into kernel_map */
 	error = vm_mmap(kernel_map, (vm_offset_t *)&a_out, PAGE_SIZE,
 	    VM_PROT_READ, VM_PROT_READ, 0, OBJT_VNODE, vp, 0);
-	/*
-	 * Lock no longer needed
-	 */
-	locked = 0;
-	VOP_UNLOCK(vp, 0, td);
-
 	if (error)
 		goto cleanup;
 
@@ -372,11 +361,21 @@
 	}
 	PROC_UNLOCK(td->td_proc);
 
-	mp_fixme("Unlocked vflags access.");
-	/* prevent more writers */
+	/*
+	 * Prevent more writers.
+	 * XXX: Note that if any of the VM operations fail below we don't
+	 * clear this flag.
+	 */
 	vp->v_vflag |= VV_TEXT;
 
 	/*
+	 * Lock no longer needed
+	 */
+	locked = 0;
+	VOP_UNLOCK(vp, 0, td);
+	VFS_UNLOCK_GIANT(vfslocked);
+
+	/*
 	 * Check if file_offset page aligned. Currently we cannot handle
 	 * misalinged file offsets, and so we read in the entire image
 	 * (what a waste).
@@ -453,8 +452,10 @@
 
 cleanup:
 	/* Unlock vnode if needed */
-	if (locked)
+	if (locked) {
 		VOP_UNLOCK(vp, 0, td);
+		VFS_UNLOCK_GIANT(vfslocked);
+	}
 
 	/* Release the kernel mapping. */
 	if (a_out)

==== //depot/projects/smpng/sys/i386/linux/syscalls.master#35 (text+ko) ====

@@ -165,7 +165,7 @@
 84	AUE_LSTAT	MSTD	{ int linux_lstat(char *path, struct ostat *up); }
 85	AUE_READLINK	MSTD	{ int linux_readlink(char *name, char *buf, \
 				    l_int count); }
-86	AUE_USELIB	STD	{ int linux_uselib(char *library); }
+86	AUE_USELIB	MSTD	{ int linux_uselib(char *library); }
 87	AUE_SWAPON	MNOPROTO	{ int swapon(char *name); }
 88	AUE_REBOOT	MSTD	{ int linux_reboot(l_int magic1, \
 				    l_int magic2, l_uint cmd, void *arg); }

==== //depot/projects/smpng/sys/notes#79 (text+ko) ====

@@ -90,8 +90,8 @@
 	- svr4_sys_waitsys()
 	- svr4_sys_fchroot()
 	- svr4_sys_resolvepath()
-    - linux
-	- linux_uselib()
+    + linux
+	+ linux_uselib()
     - ibcs2
 	- ibcs2_mount()
 	- ibcs2_umount()