From 8dc7c3115b611c00006eac3ee5b108296432aab7 Mon Sep 17 00:00:00 2001 From: Andy Adamson Date: Mon, 20 Mar 2006 13:44:26 -0500 Subject: [PATCH] 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 Signed-off-by: J. Bruce Fields Signed-off-by: Trond Myklebust --- fs/lockd/svclock.c | 12 +++++------- fs/locks.c | 21 +++++++++++++-------- fs/nfs/file.c | 7 +++---- fs/nfsd/nfs4state.c | 13 ++++++------- include/linux/fs.h | 2 +- 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 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd, */ 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 @@ nfs_file_write(struct kiocb *iocb, const char __user *buf, size_t count, loff_t 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 @@ nfsd4_lock(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_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 *);