Change the vnode locking in exec to not keep the vnode locked almost all

the time.

This could lead to problems when a process wants to do an exec on the same
vnode it's being run from and needs to copy in arguments from an uncached
page in the data segment. When that happens uvm detects a vnode deadlock
and returns an error causing execve() return EFAULT.

This fixes the regress test in regress/sys/kern/exec_self

Also, initialize scriptvp early in exec_script because it could be
used uninitialized in a failure case.

3 files changed, 24 insertions, 26 deletions

diff --git a/sys/kern/exec_elf.c b/sys/kern/exec_elf.c
index beb231e60aa..078963448ce 100644
--- a/sys/kern/exec_elf.c
+++ b/sys/kern/exec_elf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: exec_elf.c,v 1.38 2002/03/14 01:27:03 millert Exp $	*/
+/*	$OpenBSD: exec_elf.c,v 1.39 2002/08/22 22:04:42 art Exp $	*/
 
 /*
  * Copyright (c) 1996 Per Fogelstrom
@@ -312,7 +312,7 @@ ELFNAME(read_from)(struct proc *p, struct vnode *vp, u_long off, caddr_t buf,
 	size_t resid;
 
 	if ((error = vn_rdwr(UIO_READ, vp, buf, size, off, UIO_SYSSPACE,
-	    IO_NODELOCKED, p->p_ucred, &resid, p)) != 0)
+	    0, p->p_ucred, &resid, p)) != 0)
 		return error;
 	/*
 	 * See if we got all of it
diff --git a/sys/kern/exec_script.c b/sys/kern/exec_script.c
index 7f8dc558f2a..27f5ff7935b 100644
--- a/sys/kern/exec_script.c
+++ b/sys/kern/exec_script.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: exec_script.c,v 1.15 2001/11/06 19:53:20 miod Exp $	*/
+/*	$OpenBSD: exec_script.c,v 1.16 2002/08/22 22:04:42 art Exp $	*/
 /*	$NetBSD: exec_script.c,v 1.13 1996/02/04 02:15:06 christos Exp $	*/
 
 /*
@@ -78,6 +78,13 @@ exec_script_makecmds(p, epp)
 #endif
 
 	/*
+	 * remember the old vp and pnbuf for later, so we can restore
+	 * them if check_exec() fails.
+	 */
+	scriptvp = epp->ep_vp;
+	oldpnbuf = epp->ep_ndp->ni_cnd.cn_pnbuf;
+
+	/*
 	 * if the magic isn't that of a shell script, or we've already
 	 * done shell script processing for this exec, punt on it.
 	 */
@@ -160,7 +167,10 @@ check_shell:
 	 * close all open fd's when the start.  That kills this
 	 * method of implementing "safe" set-id and x-only scripts.
 	 */
-	if (VOP_ACCESS(epp->ep_vp, VREAD, p->p_ucred, p) == EACCES
+	vn_lock(scriptvp, LK_EXCLUSIVE|LK_RETRY, p);
+	error = VOP_ACCESS(scriptvp, VREAD, p->p_ucred, p);
+	VOP_UNLOCK(scriptvp, 0, p);
+	if (error == EACCES
 #ifdef SETUIDSCRIPTS
 	    || script_sbits
 #endif
@@ -178,7 +188,7 @@ check_shell:
 		epp->ep_flags |= EXEC_HASFD;
 		fp->f_type = DTYPE_VNODE;
 		fp->f_ops = &vnops;
-		fp->f_data = (caddr_t) epp->ep_vp;
+		fp->f_data = (caddr_t) scriptvp;
 		fp->f_flag = FREAD;
 		FILE_SET_MATURE(fp);
 	}
@@ -221,15 +231,6 @@ check_shell:
 	 */
 	epp->ep_hdrvalid = 0;
 
-	/*
-	 * remember the old vp and pnbuf for later, so we can restore
-	 * them if check_exec() fails.
-	 */
-	scriptvp = epp->ep_vp;
-	oldpnbuf = epp->ep_ndp->ni_cnd.cn_pnbuf;
-
-	VOP_UNLOCK(scriptvp, 0, p);
-
 	if ((error = check_exec(p, epp)) == 0) {
 		/* note that we've clobbered the header */
 		epp->ep_flags |= EXEC_DESTR;
diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index b4699ac766a..2b0c25bc328 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_exec.c,v 1.69 2002/07/25 01:21:51 nordin Exp $	*/
+/*	$OpenBSD: kern_exec.c,v 1.70 2002/08/22 22:04:42 art Exp $	*/
 /*	$NetBSD: kern_exec.c,v 1.75 1996/02/09 18:59:28 christos Exp $	*/
 
 /*-
@@ -154,9 +154,12 @@ check_exec(p, epp)
 	if ((error = VOP_OPEN(vp, FREAD, p->p_ucred, p)) != 0)
 		goto bad1;
 
+	/* unlock vp, we need it unlocked from here */
+	VOP_UNLOCK(vp, 0, p);
+
 	/* now we have the file, get the exec header */
 	error = vn_rdwr(UIO_READ, vp, epp->ep_hdr, epp->ep_hdrlen, 0,
-	    UIO_SYSSPACE, IO_NODELOCKED, p->p_ucred, &resid, p);
+	    UIO_SYSSPACE, 0, p->p_ucred, &resid, p);
 	if (error)
 		goto bad2;
 	epp->ep_hdrvalid = epp->ep_hdrlen - resid;
@@ -171,7 +174,6 @@ check_exec(p, epp)
 
 		if (execsw[i].es_check == NULL)
 			continue;
-
 		newerror = (*execsw[i].es_check)(p, epp);
 		/* make sure the first "interesting" error code is saved. */
 		if (!newerror || error == ENOEXEC)
@@ -202,10 +204,8 @@ check_exec(p, epp)
 
 bad2:
 	/*
-	 * unlock and close the vnode, free the
-	 * pathname buf, and punt.
+	 * close the vnode, free the pathname buf, and punt.
 	 */
-	VOP_UNLOCK(vp, 0, p);
 	vn_close(vp, FREAD, p->p_ucred, p);
 	FREE(ndp->ni_cnd.cn_pnbuf, M_NAMEI);
 	return (error);
@@ -578,8 +578,7 @@ sys_execve(p, v, retval)
 	uvm_km_free_wakeup(exec_map, (vaddr_t) argp, NCARGS);
 
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
-	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
-	vput(pack.ep_vp);
+	vn_close(pack.ep_vp, FREAD, cred, p);
 
 	/*
 	 * notify others that we exec'd
@@ -623,8 +622,7 @@ bad:
 		(void) fdrelease(p, pack.ep_fd);
 	}
 	/* close and put the exec'd file */
-	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
-	vput(pack.ep_vp);
+	vn_close(pack.ep_vp, FREAD, cred, p);
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
 	uvm_km_free_wakeup(exec_map, (vaddr_t) argp, NCARGS);
 
@@ -644,8 +642,7 @@ exec_abort:
 	if (pack.ep_emul_arg)
 		FREE(pack.ep_emul_arg, M_TEMP);
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
-	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
-	vput(pack.ep_vp);
+	vn_close(pack.ep_vp, FREAD, cred, p);
 	uvm_km_free_wakeup(exec_map, (vaddr_t) argp, NCARGS);
 
 free_pack_abort: