- Hold the vnode lock throughout execve.

 - Set VV_TEXT in the top level execve code.
 - Fixup the image activators to deal with the newly locked vnode.

8 files changed, 91 insertions, 74 deletions

diff --git a/sys/alpha/osf1/imgact_osf1.c b/sys/alpha/osf1/imgact_osf1.c
index 3a640b4..1a44189 100644
--- a/sys/alpha/osf1/imgact_osf1.c
+++ b/sys/alpha/osf1/imgact_osf1.c
@@ -146,12 +146,11 @@ exec_osf1_imgact(struct image_params *imgp)
 			return(error);
 		} 
 		if (imgp->vp) {
-			vrele(imgp->vp);
+			vput(imgp->vp);
 		/* leaking in the nameizone ??? XXX */
 		}
 		imgp->vp = ndp->ni_vp;
 		error = exec_map_first_page(imgp);
-		VOP_UNLOCK(imgp->vp, 0, FIRST_THREAD_IN_PROC(imgp->proc));
 		osf_auxargs->loader = "/compat/osf1/sbin/loader";
 	}
 
@@ -188,17 +187,6 @@ exec_osf1_imgact(struct image_params *imgp)
 	imgp->interpreted = 0;
 	imgp->proc->p_sysent = &osf1_sysvec;
 
-	mp_fixme("Unlocked writecount and v_vflag access.");
-	if ((eap->tsize != 0 || eap->dsize != 0) &&
-	    imgp->vp->v_writecount != 0) {
-#ifdef DIAGNOSTIC
-		if (imgp->vp->v_vflag & VV_TEXT)
-			panic("exec: a VV_TEXT vnode has writecount != 0\n");
-#endif
-		return ETXTBSY;
-	}
-	imgp->vp->v_vflag |= VV_TEXT;
-
 	/* set up text segment */
 	if ((error = vm_mmap(&vmspace->vm_map, &taddr, tsize,
 	    VM_PROT_READ|VM_PROT_EXECUTE, VM_PROT_ALL, MAP_FIXED|MAP_COPY,
diff --git a/sys/compat/pecoff/imgact_pecoff.c b/sys/compat/pecoff/imgact_pecoff.c
index 175b021..f7b540c 100644
--- a/sys/compat/pecoff/imgact_pecoff.c
+++ b/sys/compat/pecoff/imgact_pecoff.c
@@ -533,8 +533,6 @@ exec_pecoff_coff_prep_zmagic(struct image_params * imgp,
 	imgp->auxarg_size = sizeof(struct pecoff_args);
 	imgp->interpreted = 0;
 
-	mp_fixme("Unlocked vflag access.");
-	imgp->vp->v_vflag |= VV_TEXT;
 	if (sh != NULL)
 		free(sh, M_TEMP);
 	return 0;
@@ -628,21 +626,26 @@ imgact_pecoff(struct image_params * imgp)
 	imgp->image_header;
 	struct coff_filehdr *fp;
 	int             error, peofs;
+	struct thread *td = curthread;
+
 	error = pecoff_signature(FIRST_THREAD_IN_PROC(imgp->proc),
 	    imgp->vp, dp);
 	if (error) {
 		return -1;
 	}
+	VOP_UNLOCK(imgp->vp, 0, td);
+
 	peofs = dp->d_peofs + sizeof(signature) - 1;
 	fp = malloc(PECOFF_HDR_SIZE, M_TEMP, M_WAITOK);
 	error = pecoff_read_from(FIRST_THREAD_IN_PROC(imgp->proc),
 	     imgp->vp, peofs, (caddr_t) fp, PECOFF_HDR_SIZE);
-	if (error) {
-		free(fp, M_TEMP);
-		return error;
-	}
+	if (error)
+		goto fail;
+
 	error = exec_pecoff_coff_makecmds(imgp, fp, peofs);
+fail:   
 	free(fp, M_TEMP);
+        vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY, td);
 	return error;
 }
 
diff --git a/sys/compat/svr4/imgact_svr4.c b/sys/compat/svr4/imgact_svr4.c
index c8b0e1f8..3104291 100644
--- a/sys/compat/svr4/imgact_svr4.c
+++ b/sys/compat/svr4/imgact_svr4.c
@@ -67,6 +67,7 @@ exec_svr4_imgact(imgp)
     vm_offset_t buffer;
     unsigned long bss_size;
     int error;
+    struct thread *td = curthread;
 
     if (((a_out->a_magic >> 16) & 0xff) != 0x64)
 	return -1;
@@ -111,10 +112,12 @@ exec_svr4_imgact(imgp)
 	a_out->a_data + bss_size > imgp->proc->p_rlimit[RLIMIT_DATA].rlim_cur)
 	return (ENOMEM);
 
+    VOP_UNLOCK(imgp->vp, 0, td);
+
     /* copy in arguments and/or environment from old process */
     error = exec_extract_strings(imgp);
     if (error)
-	return (error);
+	goto fail;
 
     /*
      * Destroy old process VM and create a new one (with a new stack)
@@ -139,14 +142,14 @@ exec_svr4_imgact(imgp)
 		    	    a_out->a_text + a_out->a_data + bss_size, FALSE,
 			    VM_PROT_ALL, VM_PROT_ALL, 0);
 	if (error)
-	    return error;
+	    goto fail;
 
 	error = vm_mmap(kernel_map, &buffer,
 			round_page(a_out->a_text + a_out->a_data + file_offset),
 			VM_PROT_READ, VM_PROT_READ, 0,
 			(caddr_t) imgp->vp, trunc_page(file_offset));
 	if (error)
-	    return error;
+	    goto fail;
 
 	error = copyout((caddr_t)(buffer + file_offset), (caddr_t)vmaddr, 
 			a_out->a_text + a_out->a_data);
@@ -155,7 +158,7 @@ exec_svr4_imgact(imgp)
 		      buffer + round_page(a_out->a_text + a_out->a_data + file_offset));
 
 	if (error)
-	    return error;
+	    goto fail;
 
 	/*
 	 * remove write enable on the 'text' part
@@ -166,7 +169,7 @@ exec_svr4_imgact(imgp)
 		   	       VM_PROT_EXECUTE|VM_PROT_READ,
 		   	       TRUE);
 	if (error)
-	    return error;
+	    goto fail;
     }
     else {
 #ifdef DEBUG
@@ -183,7 +186,7 @@ exec_svr4_imgact(imgp)
 	    		MAP_PRIVATE | MAP_FIXED,
 	    		(caddr_t)imgp->vp, file_offset);
 	if (error)
-	    return (error);
+	    goto fail;
     
 #ifdef DEBUG
 	printf("imgact: startaddr=%08lx, length=%08lx\n", (u_long)vmaddr,
@@ -198,7 +201,7 @@ exec_svr4_imgact(imgp)
 			       VM_PROT_ALL,
 			       FALSE);
 	if (error)
-	    return (error);
+	    goto fail;
     
 	/*
 	 * Allocate anon demand-zeroed area for uninitialized data
@@ -208,16 +211,13 @@ exec_svr4_imgact(imgp)
 	    error = vm_map_find(&vmspace->vm_map, NULL, 0, &vmaddr, 
 				bss_size, FALSE, VM_PROT_ALL, VM_PROT_ALL, 0);
 	    if (error)
-		return (error);
+		goto fail;
 #ifdef DEBUG
 	    printf("imgact: bssaddr=%08lx, length=%08lx\n",
 	        (u_long)vmaddr, bss_size);
 #endif
 
 	}
-	/* Indicate that this file should not be modified */
-	mp_fixme("Unlocked vflag access.");
-	imgp->vp->v_vflag |= VV_TEXT;
     }
     /* Fill in process VM information */
     vmspace->vm_tsize = round_page(a_out->a_text) >> PAGE_SHIFT;
@@ -230,7 +230,9 @@ exec_svr4_imgact(imgp)
     imgp->entry_addr = a_out->a_entry;
     
     imgp->proc->p_sysent = &svr4_sysvec;
-    return (0);
+fail:
+    vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY);
+    return (error);
 }
 
 /*
diff --git a/sys/i386/ibcs2/imgact_coff.c b/sys/i386/ibcs2/imgact_coff.c
index 61ca412..0e5363e 100644
--- a/sys/i386/ibcs2/imgact_coff.c
+++ b/sys/i386/ibcs2/imgact_coff.c
@@ -301,6 +301,7 @@ exec_coff_imgact(imgp)
 	unsigned long data_offset = 0, data_address = 0, data_size = 0;
 	unsigned long bss_size = 0;
 	caddr_t hole;
+	struct thread *td = curthread;
 
 	if (fhdr->f_magic != I386_COFF ||
 	    !(fhdr->f_flags & F_EXEC)) {
@@ -328,9 +329,11 @@ exec_coff_imgact(imgp)
 	       ((const char*)(imgp->image_header) + sizeof(struct filehdr) +
 		sizeof(struct aouthdr));
 
+	VOP_UNLOCK(imgp->vp, 0, td);
+
 	if ((error = exec_extract_strings(imgp)) != 0) {
 		DPRINTF(("%s(%d):  return %d\n", __FILE__, __LINE__, error));
-		return error;
+		goto fail;
 	}
 
 	exec_new_vmspace(imgp, VM_MIN_ADDRESS, VM_MAXUSER_ADDRESS, USRSTACK);
@@ -375,7 +378,8 @@ exec_coff_imgact(imgp)
 				    0,
 				    (caddr_t) imgp->vp,
 				    foff)) != 0) {
-	      		return ENOEXEC;
+	      		error = ENOEXEC;
+			goto fail;
 	    	}
 		if(scns[i].s_size) {
 			char *libbuf;
@@ -410,7 +414,7 @@ exec_coff_imgact(imgp)
 				  (vm_offset_t) buf + len))
 	      		panic("exec_coff_imgact vm_map_remove failed");
 	    	if (error)
-	      		return error;
+	      		goto fail;
 	  	}
 	}
 	/*
@@ -427,7 +431,7 @@ exec_coff_imgact(imgp)
 				      text_size, text_size,
 				      VM_PROT_READ | VM_PROT_EXECUTE)) != 0) {
 		DPRINTF(("%s(%d): error = %d\n", __FILE__, __LINE__, error));
-		return error;
+		goto fail;
        	}
 	/*
 	 * Map in .data and .bss now
@@ -445,7 +449,7 @@ exec_coff_imgact(imgp)
 				      VM_PROT_ALL)) != 0) {
 
 		DPRINTF(("%s(%d): error = %d\n", __FILE__, __LINE__, error));
-		return error;
+		goto fail;
 	}
 
 	imgp->interpreted = 0;
@@ -472,10 +476,10 @@ exec_coff_imgact(imgp)
 		ctob(vmspace->vm_dsize) + vmspace->vm_daddr ));
 	DPRINTF(("%s(%d):  returning successfully!\n", __FILE__, __LINE__));
 
-	/* Indicate that this file should not be modified */
-	mp_fixme("Unlocked v_flag access");
-	imgp->vp->v_vflag |= VV_TEXT;
-	return 0;
+fail:
+	vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY, td);
+
+	return error;
 }
 
 /*
diff --git a/sys/i386/linux/imgact_linux.c b/sys/i386/linux/imgact_linux.c
index 5634619..4b493d1 100644
--- a/sys/i386/linux/imgact_linux.c
+++ b/sys/i386/linux/imgact_linux.c
@@ -65,6 +65,7 @@ exec_linux_imgact(imgp)
     unsigned long virtual_offset, file_offset;
     vm_offset_t buffer;
     unsigned long bss_size;
+    struct thread *td = curthread;
     int error;
 
     if (((a_out->a_magic >> 16) & 0xff) != 0x64)
@@ -110,10 +111,12 @@ exec_linux_imgact(imgp)
 	a_out->a_data + bss_size > imgp->proc->p_rlimit[RLIMIT_DATA].rlim_cur)
 	return (ENOMEM);
 
+    VOP_UNLOCK(imgp->vp, 0, td);
+
     /* copy in arguments and/or environment from old process */
     error = exec_extract_strings(imgp);
     if (error)
-	return (error);
+	goto fail;
 
     /*
      * Destroy old process VM and create a new one (with a new stack)
@@ -138,14 +141,14 @@ exec_linux_imgact(imgp)
 		    	    a_out->a_text + a_out->a_data + bss_size, FALSE,
 			    VM_PROT_ALL, VM_PROT_ALL, 0);
 	if (error)
-	    return error;
+	    goto fail;
 
 	error = vm_mmap(kernel_map, &buffer,
 			round_page(a_out->a_text + a_out->a_data + file_offset),
 			VM_PROT_READ, VM_PROT_READ, 0,
 			(caddr_t) imgp->vp, trunc_page(file_offset));
 	if (error)
-	    return error;
+	    goto fail;
 
 	error = copyout((caddr_t)(void *)(uintptr_t)(buffer + file_offset),
 			(caddr_t)vmaddr, a_out->a_text + a_out->a_data);
@@ -154,7 +157,7 @@ exec_linux_imgact(imgp)
 		      buffer + round_page(a_out->a_text + a_out->a_data + file_offset));
 
 	if (error)
-	    return error;
+	    goto fail;
 
 	/*
 	 * remove write enable on the 'text' part
@@ -165,7 +168,7 @@ exec_linux_imgact(imgp)
 		   	       VM_PROT_EXECUTE|VM_PROT_READ,
 		   	       TRUE);
 	if (error)
-	    return error;
+	    goto fail;
     }
     else {
 #ifdef DEBUG
@@ -182,7 +185,7 @@ exec_linux_imgact(imgp)
 	    		MAP_PRIVATE | MAP_FIXED,
 	    		(caddr_t)imgp->vp, file_offset);
 	if (error)
-	    return (error);
+	    goto fail;
     
 #ifdef DEBUG
 	printf("imgact: startaddr=%08lx, length=%08lx\n",
@@ -197,7 +200,7 @@ exec_linux_imgact(imgp)
 			       VM_PROT_ALL,
 			       FALSE);
 	if (error)
-	    return (error);
+	    goto fail;
     
 	/*
 	 * Allocate anon demand-zeroed area for uninitialized data
@@ -207,7 +210,7 @@ exec_linux_imgact(imgp)
 	    error = vm_map_find(&vmspace->vm_map, NULL, 0, &vmaddr, 
 				bss_size, FALSE, VM_PROT_ALL, VM_PROT_ALL, 0);
 	    if (error)
-		return (error);
+		goto fail;
 #ifdef DEBUG
 	    printf("imgact: bssaddr=%08lx, length=%08lx\n",
 		(u_long)vmaddr, bss_size);
@@ -230,7 +233,10 @@ exec_linux_imgact(imgp)
     imgp->entry_addr = a_out->a_entry;
     
     imgp->proc->p_sysent = &linux_sysvec;
-    return (0);
+
+fail:
+    vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY, td);
+    return (error);
 }
 
 /*
diff --git a/sys/kern/imgact_aout.c b/sys/kern/imgact_aout.c
index ba2f76b..48eed90 100644
--- a/sys/kern/imgact_aout.c
+++ b/sys/kern/imgact_aout.c
@@ -239,10 +239,6 @@ exec_aout_imgact(imgp)
 
 	imgp->proc->p_sysent = &aout_sysvec;
 
-	/* Indicate that this file should not be modified */
-	mp_fixme("Unlocked vflag access.");
-	imgp->vp->v_vflag |= VV_TEXT;
-
 	return (0);
 }
 
diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
index 92e86af..7d79c1d 100644
--- a/sys/kern/imgact_elf.c
+++ b/sys/kern/imgact_elf.c
@@ -599,6 +599,7 @@ __CONCAT(exec_, __elfN(imgact))(struct image_params *imgp)
 	const char *interp = NULL;
 	Elf_Brandinfo *brand_info;
 	char *path;
+	struct thread *td = curthread;
 
 	GIANT_REQUIRED;
 
@@ -624,16 +625,7 @@ __CONCAT(exec_, __elfN(imgact))(struct image_params *imgp)
 	 * From this point on, we may have resources that need to be freed.
 	 */
 
-	/*
-	 * Yeah, I'm paranoid.  There is every reason in the world to get
-	 * VTEXT now since from here on out, there are places we can have
-	 * a context switch.  Better safe than sorry; I really don't want
-	 * the file to change while it's being loaded.
-	 *
-	 * XXX We can't really set this flag safely without the vnode lock.
-	 */
-	mp_fixme("This needs the vnode lock to be safe.");
-	imgp->vp->v_vflag |= VV_TEXT;
+	VOP_UNLOCK(imgp->vp, 0, td);
 
 	if ((error = exec_extract_strings(imgp)) != 0)
 		goto fail;
@@ -846,6 +838,7 @@ __CONCAT(exec_, __elfN(imgact))(struct image_params *imgp)
 	imgp->interpreted = 0;
 
 fail:
+	vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY, td);
 	return error;
 }
 
diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index 0d6689f..c01f4fa 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
@@ -148,6 +148,7 @@ execve(td, uap)
 #endif
 	struct vnode *textvp = NULL;
 	int credential_changing;
+	int textset;
 
 	imgp = &image_params;
 
@@ -227,15 +228,23 @@ interpret:
 	 * Check file permissions (also 'opens' file)
 	 */
 	error = exec_check_permissions(imgp);
-	if (error) {
-		VOP_UNLOCK(imgp->vp, 0, td);
+	if (error)
 		goto exec_fail_dealloc;
-	}
-	VOP_GETVOBJECT(imgp->vp, &imgp->object);
-	vm_object_reference(imgp->object);
+
+	if (VOP_GETVOBJECT(imgp->vp, &imgp->object) == 0)
+		vm_object_reference(imgp->object);
+
+	/*
+	 * Set VV_TEXT now so no one can write to the executable while we're
+	 * activating it.
+	 *
+	 * Remember if this was set before and unset it in case this is not
+	 * actually an executable image.
+	 */
+	textset = imgp->vp->v_vflag & VV_TEXT;
+	imgp->vp->v_vflag |= VV_TEXT;
 
 	error = exec_map_first_page(imgp);
-	VOP_UNLOCK(imgp->vp, 0, td);
 	if (error)
 		goto exec_fail_dealloc;
 
@@ -262,8 +271,11 @@ interpret:
 	}
 
 	if (error) {
-		if (error == -1)
+		if (error == -1) {
+			if (textset == 0)
+				imgp->vp->v_vflag &= ~VV_TEXT;
 			error = ENOEXEC;
+		}
 		goto exec_fail_dealloc;
 	}
 
@@ -273,9 +285,16 @@ interpret:
 	 */
 	if (imgp->interpreted) {
 		exec_unmap_first_page(imgp);
+		/*
+		 * VV_TEXT needs to be unset for scripts.  There is a short
+		 * period before we determine that something is a script where
+		 * VV_TEXT will be set. The vnode lock is held over this
+		 * entire period so nothing should illegitimately be blocked.
+		 */
+		imgp->vp->v_vflag &= ~VV_TEXT;
 		/* free name buffer and old vnode */
 		NDFREE(ndp, NDF_ONLY_PNBUF);
-		vrele(ndp->ni_vp);
+		vput(ndp->ni_vp);
 		vm_object_deallocate(imgp->object);
 		imgp->object = NULL;
 		/* set new name to that of the interpreter */
@@ -329,6 +348,9 @@ interpret:
 	/* close files on exec */
 	fdcloseexec(td);
 
+	/* Get a reference to the vnode prior to locking the proc */
+	VREF(ndp->ni_vp);
+
 	/*
 	 * For security and other reasons, signal handlers cannot
 	 * be shared after an exec. The new process gets a copy of the old
@@ -453,10 +475,10 @@ interpret:
 	}
 
 	/*
-	 * Store the vp for use in procfs
+	 * Store the vp for use in procfs.  This vnode was referenced prior
+	 * to locking the proc lock.
 	 */
 	textvp = p->p_textvp;
-	VREF(ndp->ni_vp);
 	p->p_textvp = ndp->ni_vp;
 
 	/*
@@ -499,6 +521,7 @@ interpret:
 done1:
 	PROC_UNLOCK(p);
 
+
 	/*
 	 * Free any resources malloc'd earlier that we didn't use.
 	 */
@@ -512,6 +535,8 @@ done1:
 	 */
 	if (textvp != NULL)
 		vrele(textvp);
+	if (ndp->ni_vp && error != 0)
+		vrele(ndp->ni_vp);
 #ifdef KTRACE
 	if (tracevp != NULL)
 		vrele(tracevp);
@@ -535,7 +560,7 @@ exec_fail_dealloc:
 
 	if (imgp->vp) {
 		NDFREE(ndp, NDF_ONLY_PNBUF);
-		vrele(imgp->vp);
+		vput(imgp->vp);
 	}
 
 	if (imgp->object)