Fix a bunch of problems and races with posix file locking.

- file descriptor table becomes the owner of the lock instead of the proc.
- When grabbing the lock, we check if the fd hasn't changed under our
  feet, this is more or less impossible to solve without a hack like
  this. I've banged my head against the wall, I figured out a solution,
  but implementing it correctly would cost me 12 gray hairs. Screw it,
  this is ugly, but it works.
- Wait until usecount drains before releasing the posix lock in closef.
- Add missing FREF/FRELE to sys_flock
- keep the pid in the flock struct instead of abusing the fact that we
  used to use the proc as the lock owner.

Pointed out by and discussed with Al Viro, big thanks.
miod@ ok

3 files changed, 75 insertions, 53 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 772d0006ed9..8097d04710e 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_descrip.c,v 1.79 2008/06/12 18:10:06 thib Exp $	*/
+/*	$OpenBSD: kern_descrip.c,v 1.80 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: kern_descrip.c,v 1.42 1996/03/30 22:24:38 christos Exp $	*/
 
 /*
@@ -430,22 +430,21 @@ restart:
 				error = EBADF;
 				goto out;
 			}
-			atomic_setbits_int(&p->p_flag, P_ADVLOCK);
-			error = (VOP_ADVLOCK(vp, (caddr_t)p, F_SETLK, &fl, flg));
-			goto out;
+			atomic_setbits_int(&fdp->fd_flags, FD_ADVLOCK);
+			error = VOP_ADVLOCK(vp, fdp, F_SETLK, &fl, flg);
+			break;
 
 		case F_WRLCK:
 			if ((fp->f_flag & FWRITE) == 0) {
 				error = EBADF;
 				goto out;
 			}
-			atomic_setbits_int(&p->p_flag, P_ADVLOCK);
-			error = (VOP_ADVLOCK(vp, (caddr_t)p, F_SETLK, &fl, flg));
-			goto out;
+			atomic_setbits_int(&fdp->fd_flags, FD_ADVLOCK);
+			error = VOP_ADVLOCK(vp, fdp, F_SETLK, &fl, flg);
+			break;
 
 		case F_UNLCK:
-			error = (VOP_ADVLOCK(vp, (caddr_t)p, F_UNLCK, &fl,
-				F_POSIX));
+			error = VOP_ADVLOCK(vp, fdp, F_UNLCK, &fl, F_POSIX);
 			goto out;
 
 		default:
@@ -453,6 +452,21 @@ restart:
 			goto out;
 		}
 
+		if (fp != fd_getfile(fdp, fd)) {
+			/*
+			 * We have lost the race with close() or dup2()
+			 * unlock, pretend that we'd won the race and that
+			 * lock had been removed by close()
+			 */
+			fl.l_whence = SEEK_SET;
+			fl.l_start = 0;
+			fl.l_len = 0;
+			VOP_ADVLOCK(vp, fdp, F_UNLCK, &fl, F_POSIX);
+			fl.l_type = F_UNLCK;
+		}
+		goto out;
+
+
 	case F_GETLK:
 		if (fp->f_type != DTYPE_VNODE) {
 			error = EBADF;
@@ -479,7 +493,7 @@ restart:
 			error = EINVAL;
 			break;
 		}
-		error = VOP_ADVLOCK(vp, (caddr_t)p, F_GETLK, &fl, F_POSIX);
+		error = VOP_ADVLOCK(vp, fdp, F_GETLK, &fl, F_POSIX);
 		if (error)
 			break;
 		error = (copyout((caddr_t)&fl, (caddr_t)SCARG(uap, arg),
@@ -1013,14 +1027,36 @@ fdfree(struct proc *p)
 int
 closef(struct file *fp, struct proc *p)
 {
-	struct vnode *vp;
-	struct flock lf;
+	struct filedesc *fdp;
+	int references_left;
 	int error;
 
 	if (fp == NULL)
 		return (0);
 
 	/*
+	 * Some files passed to this function could be accessed
+	 * without a FILE_IS_USABLE check (and in some cases it's perfectly
+	 * legal), we must beware of files where someone already won the
+	 * race to FIF_WANTCLOSE.
+	 */
+	if ((fp->f_iflags & FIF_WANTCLOSE) != 0 ||
+	    --fp->f_count > 0) {
+		references_left = 1;
+	} else {
+		references_left = 0;
+#ifdef DIAGNOSTIC
+		if (fp->f_count < 0)
+			panic("closef: count < 0");
+#endif
+
+		/* Wait for the last usecount to drain. */
+		fp->f_iflags |= FIF_WANTCLOSE;
+		while (fp->f_usecount > 1)
+			tsleep(&fp->f_usecount, PRIBIO, "closef", 0);
+	}
+
+	/*
 	 * POSIX record locking dictates that any close releases ALL
 	 * locks owned by this process.  This is handled by setting
 	 * a flag in the unlock to free ONLY locks obeying POSIX
@@ -1028,49 +1064,24 @@ closef(struct file *fp, struct proc *p)
 	 * If the descriptor was in a message, POSIX-style locks
 	 * aren't passed with the descriptor.
 	 */
-	if (p && (p->p_flag & P_ADVLOCK) && fp->f_type == DTYPE_VNODE) {
+	if (p && ((fdp = p->p_fd) != NULL) &&
+	    (fdp->fd_flags & FD_ADVLOCK) &&
+	    fp->f_type == DTYPE_VNODE) {
+		struct vnode *vp = fp->f_data;
+		struct flock lf;
+
 		lf.l_whence = SEEK_SET;
 		lf.l_start = 0;
 		lf.l_len = 0;
 		lf.l_type = F_UNLCK;
-		vp = (struct vnode *)fp->f_data;
-		(void) VOP_ADVLOCK(vp, (caddr_t)p, F_UNLCK, &lf, F_POSIX);
-	}
-
-	/*
-	 * Some files passed to this function could be accessed
-	 * without a FILE_IS_USABLE check (and in some cases it's perfectly
-	 * legal), we must beware of files where someone already won the
-	 * race to FIF_WANTCLOSE.
-	 */
-	if ((fp->f_iflags & FIF_WANTCLOSE) != 0) {
-		FRELE(fp);
-		return (0);
+		(void) VOP_ADVLOCK(vp, fdp, F_UNLCK, &lf, F_POSIX);
 	}
 
-	if (--fp->f_count > 0) {
+	if (references_left) {
 		FRELE(fp);
 		return (0);
 	}
 
-#ifdef DIAGNOSTIC
-	if (fp->f_count < 0)
-		panic("closef: count < 0");
-#endif
-
-	/* Wait for the last usecount to drain. */
-	fp->f_iflags |= FIF_WANTCLOSE;
-	while (fp->f_usecount > 1)
-		tsleep(&fp->f_usecount, PRIBIO, "closef", 0);
-
-	if ((fp->f_flag & FHASLOCK) && fp->f_type == DTYPE_VNODE) {
-		lf.l_whence = SEEK_SET;
-		lf.l_start = 0;
-		lf.l_len = 0;
-		lf.l_type = F_UNLCK;
-		vp = (struct vnode *)fp->f_data;
-		(void) VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, &lf, F_FLOCK);
-	}
 	if (fp->f_ops)
 		error = (*fp->f_ops->fo_close)(fp, p);
 	else
@@ -1115,6 +1126,7 @@ sys_flock(struct proc *p, void *v, register_t *retval)
 		return (EBADF);
 	if (fp->f_type != DTYPE_VNODE)
 		return (EOPNOTSUPP);
+	FREF(fp);
 	vp = (struct vnode *)fp->f_data;
 	lf.l_whence = SEEK_SET;
 	lf.l_start = 0;
@@ -1139,6 +1151,7 @@ sys_flock(struct proc *p, void *v, register_t *retval)
 	else
 		error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, &lf, F_FLOCK|F_WAIT);
 out:
+	FRELE(fp);
 	return (error);
 }
 
diff --git a/sys/kern/vfs_lockf.c b/sys/kern/vfs_lockf.c
index f628c6a1608..9afa0c0a367 100644
--- a/sys/kern/vfs_lockf.c
+++ b/sys/kern/vfs_lockf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vfs_lockf.c,v 1.12 2005/11/20 21:55:15 pedro Exp $	*/
+/*	$OpenBSD: vfs_lockf.c,v 1.13 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: vfs_lockf.c,v 1.7 1996/02/04 02:18:21 christos Exp $	*/
 
 /*
@@ -192,6 +192,7 @@ lf_advlock(struct lockf **head, off_t size, caddr_t id, int op,
 	lock->lf_next = NULL;
 	TAILQ_INIT(&lock->lf_blkhd);
 	lock->lf_flags = flags;
+	lock->lf_pid = (flags & F_POSIX) ? curproc->p_pid : -1;
 	/*
 	 * Do the requested operation.
 	 */
@@ -544,10 +545,7 @@ lf_getlock(struct lockf *lock, struct flock *fl)
 			fl->l_len = 0;
 		else
 			fl->l_len = block->lf_end - block->lf_start + 1;
-		if (block->lf_flags & F_POSIX)
-			fl->l_pid = ((struct proc *)(block->lf_id))->p_pid;
-		else
-			fl->l_pid = -1;
+		fl->l_pid = block->lf_pid;
 	} else {
 		fl->l_type = F_UNLCK;
 	}
diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index f7782056ea7..c8f33a561e1 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vfs_vnops.c,v 1.59 2008/04/08 14:46:45 thib Exp $	*/
+/*	$OpenBSD: vfs_vnops.c,v 1.60 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: vfs_vnops.c,v 1.20 1996/02/04 02:18:41 christos Exp $	*/
 
 /*
@@ -52,6 +52,7 @@
 #include <sys/tty.h>
 #include <sys/cdio.h>
 #include <sys/poll.h>
+#include <sys/filedesc.h>
 
 #include <uvm/uvm_extern.h>
 #include <miscfs/specfs/specdev.h>
@@ -482,8 +483,18 @@ vn_lock(struct vnode *vp, int flags, struct proc *p)
 int
 vn_closefile(struct file *fp, struct proc *p)
 {
-	return (vn_close(((struct vnode *)fp->f_data), fp->f_flag,
-		fp->f_cred, p));
+	struct vnode *vp = fp->f_data;
+	struct flock lf;
+	
+	if ((fp->f_flag & FHASLOCK)) {
+		lf.l_whence = SEEK_SET;
+		lf.l_start = 0;
+		lf.l_len = 0;
+		lf.l_type = F_UNLCK;
+		(void) VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, &lf, F_FLOCK);
+	}
+
+	return (vn_close(vp, fp->f_flag, fp->f_cred, p));
 }
 
 int