svn commit: r238952 - head/sys/kern

[John Baldwin]

Author: jhb
Date: Tue Jul 31 18:25:00 2012
New Revision: 238952
URL: http://svn.freebsd.org/changeset/base/238952

Log:
  Reorder the managament of advisory locks on open files so that the advisory
  lock is obtained before the write count is increased during open() and the
  lock is released after the write count is decreased during close().
  
  The first change closes a race where an open() that will block with O_SHLOCK
  or O_EXLOCK can increase the write count while it waits.  If the process
  holding the current lock on the file then tries to call exec() on the file
  it has locked, it can fail with ETXTBUSY even though the advisory lock is
  preventing other threads from succesfully completeing a writable open().
  
  The second change closes a race where a read-only open() with O_SHLOCK or
  O_EXLOCK may return successfully while the write count is non-zero due to
  another descriptor that had the advisory lock and was blocking the open()
  still being in the process of closing.  If the process that completed the
  open() then attempts to call exec() on the file it locked, it can fail with
  ETXTBUSY even though the other process that held a write lock has closed
  the file and released the lock.
  
  Reviewed by:  kib
  MFC after:1 month

Modified:
  head/sys/kern/vfs_syscalls.c
  head/sys/kern/vfs_vnops.c

Modified: head/sys/kern/vfs_syscalls.c
==
--- head/sys/kern/vfs_syscalls.cTue Jul 31 17:32:28 2012
(r238951)
+++ head/sys/kern/vfs_syscalls.cTue Jul 31 18:25:00 2012
(r238952)
@@ -1093,8 +1093,7 @@ kern_openat(struct thread *td, int fd, c
struct file *fp;
struct vnode *vp;
int cmode;
-   int type, indx = -1, error;
-   struct flock lf;
+   int indx = -1, error;
struct nameidata nd;
int vfslocked;
cap_rights_t rights_needed = CAP_LOOKUP;
@@ -1180,26 +1179,11 @@ kern_openat(struct thread *td, int fd, c
if (fp-f_ops == badfileops) {
KASSERT(vp-v_type != VFIFO, (Unexpected fifo.));
fp-f_seqcount = 1;
-   finit(fp, flags  FMASK, DTYPE_VNODE, vp, vnops);
+   finit(fp, (flags  FMASK) | (fp-f_flag  FHASLOCK), 
DTYPE_VNODE,
+   vp, vnops);
}
 
VOP_UNLOCK(vp, 0);
-   if (fp-f_type == DTYPE_VNODE  (flags  (O_EXLOCK | O_SHLOCK)) != 0) {
-   lf.l_whence = SEEK_SET;
-   lf.l_start = 0;
-   lf.l_len = 0;
-   if (flags  O_EXLOCK)
-   lf.l_type = F_WRLCK;
-   else
-   lf.l_type = F_RDLCK;
-   type = F_FLOCK;
-   if ((flags  FNONBLOCK) == 0)
-   type |= F_WAIT;
-   if ((error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, lf,
-   type)) != 0)
-   goto bad;
-   atomic_set_int(fp-f_flag, FHASLOCK);
-   }
if (flags  O_TRUNC) {
error = fo_truncate(fp, 0, td-td_ucred, td);
if (error)
@@ -4483,9 +4467,8 @@ sys_fhopen(td, uap)
struct mount *mp;
struct vnode *vp;
struct fhandle fhp;
-   struct flock lf;
struct file *fp;
-   int fmode, error, type;
+   int fmode, error;
int vfslocked;
int indx;
 
@@ -4542,24 +4525,9 @@ sys_fhopen(td, uap)
 #endif
fp-f_vnode = vp;
fp-f_seqcount = 1;
-   finit(fp, fmode  FMASK, DTYPE_VNODE, vp, vnops);
+   finit(fp, (fmode  FMASK) | (fp-f_flag  FHASLOCK), DTYPE_VNODE, vp,
+   vnops);
VOP_UNLOCK(vp, 0);
-   if (fmode  (O_EXLOCK | O_SHLOCK)) {
-   lf.l_whence = SEEK_SET;
-   lf.l_start = 0;
-   lf.l_len = 0;
-   if (fmode  O_EXLOCK)
-   lf.l_type = F_WRLCK;
-   else
-   lf.l_type = F_RDLCK;
-   type = F_FLOCK;
-   if ((fmode  FNONBLOCK) == 0)
-   type |= F_WAIT;
-   if ((error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, lf,
-   type)) != 0)
-   goto bad;
-   atomic_set_int(fp-f_flag, FHASLOCK);
-   }
if (fmode  O_TRUNC) {
error = fo_truncate(fp, 0, td-td_ucred, td);
if (error)

Modified: head/sys/kern/vfs_vnops.c
==
--- head/sys/kern/vfs_vnops.c   Tue Jul 31 17:32:28 2012(r238951)
+++ head/sys/kern/vfs_vnops.c   Tue Jul 31 18:25:00 2012(r238952)
@@ -229,8 +229,10 @@ int
 vn_open_vnode(struct vnode *vp, int fmode, struct ucred *cred,
 struct thread *td, struct file *fp)
 {
+   struct mount *mp;
accmode_t accmode;
-   int error;
+   struct flock lf;
+   int error, have_flock, lock_flags, type;
 

Re: svn commit: r238952 - head/sys/kern

[John Baldwin]

On Tuesday, July 31, 2012 2:25:01 pm John Baldwin wrote:
 Author: jhb
 Date: Tue Jul 31 18:25:00 2012
 New Revision: 238952
 URL: http://svn.freebsd.org/changeset/base/238952
 
 Log:
   Reorder the managament of advisory locks on open files so that the advisory
   lock is obtained before the write count is increased during open() and the
   lock is released after the write count is decreased during close().
   
   The first change closes a race where an open() that will block with O_SHLOCK
   or O_EXLOCK can increase the write count while it waits.  If the process
   holding the current lock on the file then tries to call exec() on the file
   it has locked, it can fail with ETXTBUSY even though the advisory lock is
   preventing other threads from succesfully completeing a writable open().
   
   The second change closes a race where a read-only open() with O_SHLOCK or
   O_EXLOCK may return successfully while the write count is non-zero due to
   another descriptor that had the advisory lock and was blocking the open()
   still being in the process of closing.  If the process that completed the
   open() then attempts to call exec() on the file it locked, it can fail with
   ETXTBUSY even though the other process that held a write lock has closed
   the file and released the lock.
   
   Reviewed by:kib
   MFC after:  1 month

Oops, should have included:

Tested by:  pho

I have a regression test (of sorts) for this.  If you run it on an unpatched
system it should start emitting errors within a few seconds.

#include sys/types.h
#include sys/stat.h
#include sys/wait.h
#include err.h
#include errno.h
#include fcntl.h
#include stdio.h
#include stdlib.h
#include unistd.h

static void
usage(void)
{
fprintf(stderr, Usage: flock_close_race binary [args]\n);
exit(1);
}

static void
child(const char *binary)
{
int fd;

/* Exit as soon as our parent exits. */
while (getppid() != 1) {
fd = open(binary, O_RDWR | O_EXLOCK);
if (fd  0) {
/*
 * This may get ETXTBSY since exit() will
 * close its open fd's (thus releasing the
 * lock), before it releases the vmspace (and
 * mapping of the binary).
 */
if (errno == ETXTBSY)
continue;
err(1, can't open %s, binary);
}
close(fd);
}
exit(0);
}

static void
exec_child(char **av)
{
int fd;

fd = open(av[0], O_RDONLY | O_SHLOCK);
execv(av[0], av);
err(127, execv);
}

int
main(int ac, char **av)
{
struct stat sb;
pid_t pid;

if (ac  2)
usage();
if (stat(av[1], sb) != 0)
err(1, stat(%s), av[1]);
if (!S_ISREG(sb.st_mode))
errx(1, %s not an executable, av[1]);

pid = fork();
if (pid  0)
err(1, fork);
if (pid == 0)
child(av[1]);
for (;;) {
pid = fork();
if (pid  0)
err(1, fork);
if (pid == 0)
exec_child(av + 1);
wait(NULL);
}
return (0);
}

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org