locks,lockd: fix race in nlmsvc_testlock

posix_test_lock() returns a pointer to a struct file_lock which is unprotected
and can be removed while in use by the caller.  Move the conflicting lock from
the return to a parameter, and copy the conflicting lock.

In most cases the caller ends up putting the copy of the conflicting lock on
the stack.  On i386, sizeof(struct file_lock) appears to be about 100 bytes.
We're assuming that's reasonable.

Signed-off-by: Andy Adamson <andros@citi.umich.edu>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

5 files changed, 28 insertions, 27 deletions

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index f5398097b84b..d683dd022e08 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -376,8 +376,6 @@ u32
 nlmsvc_testlock(struct nlm_file *file, struct nlm_lock *lock,
                                        struct nlm_lock *conflock)
 {
-        struct file_lock        *fl;
-
         dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
                                 file->f_file->f_dentry->d_inode->i_sb->s_id,
                                 file->f_file->f_dentry->d_inode->i_ino,
@@ -385,14 +383,14 @@ nlmsvc_testlock(struct nlm_file *file, struct nlm_lock *lock,
                                 (long long)lock->fl.fl_start,
                                 (long long)lock->fl.fl_end);
 
-        if ((fl = posix_test_lock(file->f_file, &lock->fl)) != NULL) {
+        if (posix_test_lock(file->f_file, &lock->fl, &conflock->fl)) {
                 dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
-                                fl->fl_type, (long long)fl->fl_start,
-                                (long long)fl->fl_end);
+                                conflock->fl.fl_type,
+                                (long long)conflock->fl.fl_start,
+                                (long long)conflock->fl.fl_end);
                 conflock->caller = "somehost";  /* FIXME */
                 conflock->oh.len = 0;           /* don't return OH info */
-                conflock->svid = fl->fl_pid;
-                conflock->fl = *fl;
+                conflock->svid = conflock->fl.fl_pid;
                 return nlm_lck_denied;
         }
 
diff --git a/fs/locks.c b/fs/locks.c
index cb940b142c5f..231b23c12c3d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -672,8 +672,9 @@ static int locks_block_on_timeout(struct file_lock *blocker, struct file_lock *w
         return result;
 }
 
-struct file_lock *
-posix_test_lock(struct file *filp, struct file_lock *fl)
+int
+posix_test_lock(struct file *filp, struct file_lock *fl,
+                struct file_lock *conflock)
 {
         struct file_lock *cfl;
 
@@ -684,9 +685,13 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
                 if (posix_locks_conflict(cfl, fl))
                         break;
         }
+        if (cfl) {
+                locks_copy_lock(conflock, cfl);
+                unlock_kernel();
+                return 1;
+        }
         unlock_kernel();
-
-        return (cfl);
+        return 0;
 }
 
 EXPORT_SYMBOL(posix_test_lock);
@@ -1563,7 +1568,7 @@ asmlinkage long sys_flock(unsigned int fd, unsigned int cmd)
  */
 int fcntl_getlk(struct file *filp, struct flock __user *l)
 {
-        struct file_lock *fl, file_lock;
+        struct file_lock *fl, cfl, file_lock;
         struct flock flock;
         int error;
 
@@ -1587,7 +1592,7 @@ int fcntl_getlk(struct file *filp, struct flock __user *l)
                 else
                   fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
         } else {
-                fl = posix_test_lock(filp, &file_lock);
+                fl = (posix_test_lock(filp, &file_lock, &cfl) ? &cfl : NULL);
         }
  
         flock.l_type = F_UNLCK;
@@ -1717,7 +1722,7 @@ out:
  */
 int fcntl_getlk64(struct file *filp, struct flock64 __user *l)
 {
-        struct file_lock *fl, file_lock;
+        struct file_lock *fl, cfl, file_lock;
         struct flock64 flock;
         int error;
 
@@ -1741,7 +1746,7 @@ int fcntl_getlk64(struct file *filp, struct flock64 __user *l)
                 else
                   fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
         } else {
-                fl = posix_test_lock(filp, &file_lock);
+                fl = (posix_test_lock(filp, &file_lock, &cfl) ? &cfl : NULL);
         }
  
         flock.l_type = F_UNLCK;
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 1cf07e4ad136..ee140c53dba6 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -392,15 +392,14 @@ out_swapfile:
 
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
-        struct file_lock *cfl;
+        struct file_lock cfl;
         struct inode *inode = filp->f_mapping->host;
         int status = 0;
 
         lock_kernel();
         /* Try local locking first */
-        cfl = posix_test_lock(filp, fl);
-        if (cfl != NULL) {
-                locks_copy_lock(fl, cfl);
+        if (posix_test_lock(filp, fl, &cfl)) {
+                locks_copy_lock(fl, &cfl);
                 goto out;
         }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1143cfb64549..f6ab762bea99 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2639,7 +2639,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
         struct nfs4_stateid *lock_stp;
         struct file *filp;
         struct file_lock file_lock;
-        struct file_lock *conflock;
+        struct file_lock conflock;
         int status = 0;
         unsigned int strhashval;
 
@@ -2775,11 +2775,11 @@ conflicting_lock:
         /* XXX There is a race here. Future patch needed to provide 
          * an atomic posix_lock_and_test_file
          */
-        if (!(conflock = posix_test_lock(filp, &file_lock))) {
+        if (!posix_test_lock(filp, &file_lock, &conflock)) {
                 status = nfserr_serverfault;
                 goto out;
         }
-        nfs4_set_lock_denied(conflock, &lock->lk_denied);
+        nfs4_set_lock_denied(&conflock, &lock->lk_denied);
 out:
         if (status && lock->lk_is_new && lock_sop)
                 release_stateowner(lock_sop);
@@ -2800,7 +2800,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
         struct inode *inode;
         struct file file;
         struct file_lock file_lock;
-        struct file_lock *conflicting_lock;
+        struct file_lock conflock;
         int status;
 
         if (nfs4_in_grace())
@@ -2864,10 +2864,9 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_lock
         file.f_dentry = current_fh->fh_dentry;
 
         status = nfs_ok;
-        conflicting_lock = posix_test_lock(&file, &file_lock);
-        if (conflicting_lock) {
+        if (posix_test_lock(&file, &file_lock, &conflock)) {
                 status = nfserr_denied;
-                nfs4_set_lock_denied(conflicting_lock, &lockt->lt_denied);
+                nfs4_set_lock_denied(&conflock, &lockt->lt_denied);
         }
 out:
         nfs4_unlock_state();
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b01482c721ae..8ef4dd788a83 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -754,7 +754,7 @@ extern void locks_init_lock(struct file_lock *);
 extern void locks_copy_lock(struct file_lock *, struct file_lock *);
 extern void locks_remove_posix(struct file *, fl_owner_t);
 extern void locks_remove_flock(struct file *);
-extern struct file_lock *posix_test_lock(struct file *, struct file_lock *);
+extern int posix_test_lock(struct file *, struct file_lock *, struct file_lock *);
 extern int posix_lock_file(struct file *, struct file_lock *);
 extern int posix_lock_file_wait(struct file *, struct file_lock *);
 extern int posix_unblock_lock(struct file *, struct file_lock *);