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

7 files changed, 93 insertions, 66 deletions

diff --git a/sys/compat/hpux/hpux_file.c b/sys/compat/hpux/hpux_file.c
index cad38f912b3..a3596136e40 100644
--- a/sys/compat/hpux/hpux_file.c
+++ b/sys/compat/hpux/hpux_file.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: hpux_file.c,v 1.18 2007/03/15 10:22:30 art Exp $	*/
+/*	$OpenBSD: hpux_file.c,v 1.19 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: hpux_file.c,v 1.5 1997/04/27 21:40:48 thorpej Exp $	*/
 
 /*
@@ -282,8 +282,8 @@ hpux_sys_fcntl(p, v, retval)
 				goto out;
 			}
 
-			atomic_setbits_int(&p->p_flag, P_ADVLOCK);
-			error = VOP_ADVLOCK(vp, (caddr_t)p, F_SETLK, &fl, flg);
+			atomic_setbits_int(&fdp->fd_flags, FD_ADVLOCK);
+			error = VOP_ADVLOCK(vp, fdp, F_SETLK, &fl, flg);
 			goto out;
 
 		case F_WRLCK:
@@ -291,13 +291,12 @@ hpux_sys_fcntl(p, v, retval)
 				error = EBADF;
 				goto out;
 			}
-			atomic_setbits_int(&p->p_flag, P_ADVLOCK);
-			error = VOP_ADVLOCK(vp, (caddr_t)p, F_SETLK, &fl, flg);
+			atomic_setbits_int(&fdp->fd_flags, FD_ADVLOCK);
+			error = VOP_ADVLOCK(vp, fdp, F_SETLK, &fl, flg);
 			goto out;
 
 		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:
@@ -328,8 +327,7 @@ hpux_sys_fcntl(p, v, retval)
 		if (fl.l_whence == SEEK_CUR)
 			fl.l_start += fp->f_offset;
 
-		if ((error =
-		    VOP_ADVLOCK(vp, (caddr_t)p, F_GETLK, &fl, F_POSIX)))
+		if ((error = VOP_ADVLOCK(vp, fdp, F_GETLK, &fl, F_POSIX)))
 			goto out;
 
 		hfl.hl_start = fl.l_start;
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
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index 121429a6bce..792129b1cb8 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: filedesc.h,v 1.19 2004/07/22 06:11:10 tedu Exp $	*/
+/*	$OpenBSD: filedesc.h,v 1.20 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: filedesc.h,v 1.14 1996/04/09 20:55:28 cgd Exp $	*/
 
 /*
@@ -74,6 +74,8 @@ struct filedesc {
 	struct	klist *fd_knlist;	/* list of attached knotes */
 	u_long	fd_knhashmask;		/* size of knhash */
 	struct	klist *fd_knhash;	/* hash table for attached knotes */
+
+	int fd_flags;			/* flags on the file descriptor table */
 };
 
 /*
@@ -102,6 +104,11 @@ struct filedesc0 {
 #define	UF_EXCLOSE 	0x01		/* auto-close on exec */
 
 /*
+ * Flags on the file descriptor table.
+ */
+#define FD_ADVLOCK	0x01		/* May hold a POSIX adv. lock. */
+
+/*
  * Storage required per open file descriptor.
  */
 #define OFILESIZE (sizeof(struct file *) + sizeof(char))
diff --git a/sys/sys/lockf.h b/sys/sys/lockf.h
index 1b17d451522..c6b96256d16 100644
--- a/sys/sys/lockf.h
+++ b/sys/sys/lockf.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: lockf.h,v 1.7 2005/03/10 17:26:10 tedu Exp $	*/
+/*	$OpenBSD: lockf.h,v 1.8 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: lockf.h,v 1.5 1994/06/29 06:44:33 cgd Exp $	*/
 
 /*
@@ -54,6 +54,7 @@ struct lockf {
 	struct	locklist lf_blkhd;	/* The list of blocked locks */
 	TAILQ_ENTRY(lockf) lf_block; /* A request waiting for a lock */
 	uid_t	lf_uid;		/* User ID responsible */
+	pid_t	lf_pid;		/* POSIX - owner pid */
 };
 
 /* Maximum length of sleep chains to traverse to try and detect deadlock. */
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index ba1ae9e9975..fe13e39a683 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.h,v 1.103 2008/05/05 15:37:41 thib Exp $	*/
+/*	$OpenBSD: proc.h,v 1.104 2008/09/19 12:24:55 art Exp $	*/
 /*	$NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $	*/
 
 /*-
@@ -269,7 +269,6 @@ struct proc {
 #define P_ZOMBIE(p)	((p)->p_stat == SZOMB || (p)->p_stat == SDEAD)
 
 /* These flags are kept in p_flag. */
-#define	P_ADVLOCK	0x000001	/* Proc may hold a POSIX adv. lock. */
 #define	P_CONTROLT	0x000002	/* Has a controlling terminal. */
 #define	P_INMEM		0x000004	/* Loaded into memory. UNUSED */
 #define	P_NOCLDSTOP	0x000008	/* No SIGCHLD when children stop. */