Description of problem: Alexander Viro has reported the following problems: a) fcheck() needs rcu_read_lock() or current->files->file_lock. Both in dnotify.c and locks.c. b) locks.c (fcntl_setlk()) has subtler one - we need to make sure that if we *do* have an fcntl/close race on SMP box, the access to descriptor table and inode->i_flock won't get reordered. As it is, we get STORE inode->i_flock, LOAD descriptor table entry vs. STORE descriptor table entry, LOAD inode->i_flock with not a single lock in common on both sides. We do have BKL around the first STORE, but check in locks_remove_posix() is outside of BKL and for a good reason - we don't want BKL on common path of close(2).
Proposed patch: diff -urN linux-2.6.18.x86_64/fs/dnotify.c linux-2.6.18.x86_64-fix/fs/dnotify.c --- linux-2.6.18.x86_64/fs/dnotify.c 2006-09-19 23:42:06.000000000 -0400 +++ linux-2.6.18.x86_64-fix/fs/dnotify.c 2008-04-21 02:54:33.000000000 -0400 @@ -20,6 +20,9 @@ #include <linux/init.h> #include <linux/spinlock.h> #include <linux/slab.h> +#ifndef __GENKSYMS__ +#include <linux/file.h> +#endif int dir_notify_enable __read_mostly = 1; @@ -66,6 +69,7 @@ struct dnotify_struct **prev; struct inode *inode; fl_owner_t id = current->files; + struct file *f; int error = 0; if ((arg & ~DN_MULTISHOT) == 0) { @@ -92,6 +96,15 @@ prev = &odn->dn_next; } + rcu_read_lock(); + f = fcheck(fd); + rcu_read_unlock(); + /* we'd lost the race with close(), sod off silently */ + /* note that inode->i_lock prevents reordering problems + * between accesses to descriptor table and ->i_dnotify */ + if (f != filp) + goto out_free; + error = f_setown(filp, current->pid, 0); if (error) goto out_free; diff -urN linux-2.6.18.x86_64/fs/locks.c linux-2.6.18.x86_64-fix/fs/locks.c --- linux-2.6.18.x86_64/fs/locks.c 2008-04-21 03:32:42.000000000 -0400 +++ linux-2.6.18.x86_64-fix/fs/locks.c 2008-04-21 03:42:31.000000000 -0400 @@ -1683,6 +1683,7 @@ struct file_lock *file_lock = locks_alloc_lock(); struct flock flock; struct inode *inode; + struct file *f; int error; if (file_lock == NULL) @@ -1757,7 +1758,15 @@ * Attempt to detect a close/fcntl race and recover by * releasing the lock that was just acquired. */ - if (!error && fcheck(fd) != filp && flock.l_type != F_UNLCK) { + /* + * we need that spin_lock here - it prevents reordering between + * update of inode->i_flock and check for it done in close(). + * rcu_read_lock() wouldn't do. + */ + spin_lock(¤t->files->file_lock); + f = fcheck(fd); + spin_unlock(¤t->files->file_lock); + if (!error && f != filp && flock.l_type != F_UNLCK) { flock.l_type = F_UNLCK; goto again; } @@ -1826,6 +1835,7 @@ struct file_lock *file_lock = locks_alloc_lock(); struct flock64 flock; struct inode *inode; + struct file *f; int error; if (file_lock == NULL) @@ -1900,7 +1910,10 @@ * Attempt to detect a close/fcntl race and recover by * releasing the lock that was just acquired. */ - if (!error && fcheck(fd) != filp && flock.l_type != F_UNLCK) { + spin_lock(¤t->files->file_lock); + f = fcheck(fd); + spin_unlock(¤t->files->file_lock); + if (!error && f != filp && flock.l_type != F_UNLCK) { flock.l_type = F_UNLCK; goto again; }
The fcntl_setlk()/close() race also already known upstream, relevant - commit id 0b2bac2f1ea0d33a3621b27ca68b9ae760fca2e9 - link to upstream post: http://www.kernel.org/pub/linux/kernel/v2.6/ChangeLog-2.6.25.2 -> lifting embargo on this one.
kernel-2.6.24.7-92.fc8 has been pushed to the Fedora 8 stable repository. If problems still persist, please make note of it in this bug report.
kernel-2.6.25.3-18.fc9 has been pushed to the Fedora 9 stable repository. If problems still persist, please make note of it in this bug report.
kernel-2.6.23.17-88.fc7 has been pushed to the Fedora 7 stable repository. If problems still persist, please make note of it in this bug report.
This was addressed via: Red Hat Enterprise Linux version 3 (RHSA-2008:0211) Red Hat Enterprise Linux version 5 (RHSA-2008:0233) Red Hat Enterprise Linux version 4 (RHSA-2008:0237)