From f39b913cee67e401ad697578baca0ba34830209b Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 9 Sep 2014 16:00:51 -0400 Subject: locks: pass correct "before" pointer to locks_unlink_lock in generic_add_lease The argument to locks_unlink_lock can't be just any pointer to a pointer. It must be a pointer to the fl_next field in the previous lock in the list. Cc: # v3.15+ Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/locks.c b/fs/locks.c index cb66fb05ad4a..bb08857f90b5 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1619,7 +1619,7 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp smp_mb(); error = check_conflicting_open(dentry, arg); if (error) - locks_unlink_lock(flp); + locks_unlink_lock(before); out: if (is_deleg) mutex_unlock(&inode->i_mutex); -- cgit v1.2.3 From d0449b90f80f263e17e8b3ce31442e45121dc46c Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 22 Aug 2014 10:18:42 -0400 Subject: locks: Remove unused conf argument from lm_grant This argument is always NULL so don't pass it around. [jlayton: remove dependencies on previous patches in series] Signed-off-by: Joe Perches Signed-off-by: Jeff Layton --- fs/dlm/plock.c | 8 ++++---- fs/lockd/svclock.c | 12 +++--------- include/linux/fs.h | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c index f704458ea5f5..e0ab3a93eeff 100644 --- a/fs/dlm/plock.c +++ b/fs/dlm/plock.c @@ -30,7 +30,7 @@ struct plock_op { struct plock_xop { struct plock_op xop; - void *callback; + int (*callback)(struct file_lock *fl, int result); void *fl; void *file; struct file_lock flc; @@ -190,7 +190,7 @@ static int dlm_plock_callback(struct plock_op *op) struct file *file; struct file_lock *fl; struct file_lock *flc; - int (*notify)(void *, void *, int) = NULL; + int (*notify)(struct file_lock *fl, int result) = NULL; struct plock_xop *xop = (struct plock_xop *)op; int rv = 0; @@ -209,7 +209,7 @@ static int dlm_plock_callback(struct plock_op *op) notify = xop->callback; if (op->info.rv) { - notify(fl, NULL, op->info.rv); + notify(fl, op->info.rv); goto out; } @@ -228,7 +228,7 @@ static int dlm_plock_callback(struct plock_op *op) (unsigned long long)op->info.number, file, fl); } - rv = notify(fl, NULL, 0); + rv = notify(fl, 0); if (rv) { /* XXX: We need to cancel the fs lock here: */ log_print("dlm_plock_callback: lock granted after lock request " diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index ab798a88ec1d..2a6170133c1d 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -667,22 +667,16 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l * deferred rpc for GETLK and SETLK. */ static void -nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf, - int result) +nlmsvc_update_deferred_block(struct nlm_block *block, int result) { block->b_flags |= B_GOT_CALLBACK; if (result == 0) block->b_granted = 1; else block->b_flags |= B_TIMED_OUT; - if (conf) { - if (block->b_fl) - __locks_copy_lock(block->b_fl, conf); - } } -static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf, - int result) +static int nlmsvc_grant_deferred(struct file_lock *fl, int result) { struct nlm_block *block; int rc = -ENOENT; @@ -697,7 +691,7 @@ static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf, rc = -ENOLCK; break; } - nlmsvc_update_deferred_block(block, conf, result); + nlmsvc_update_deferred_block(block, result); } else if (result == 0) block->b_granted = 1; diff --git a/include/linux/fs.h b/include/linux/fs.h index 94187721ad41..908af4f81680 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -869,7 +869,7 @@ struct lock_manager_operations { int (*lm_compare_owner)(struct file_lock *, struct file_lock *); unsigned long (*lm_owner_key)(struct file_lock *); void (*lm_notify)(struct file_lock *); /* unblock callback */ - int (*lm_grant)(struct file_lock *, struct file_lock *, int); + int (*lm_grant)(struct file_lock *, int); void (*lm_break)(struct file_lock *); int (*lm_change)(struct file_lock **, int); }; -- cgit v1.2.3 From 3fe0fff18fe87c6a2179837de68d1174903c6367 Mon Sep 17 00:00:00 2001 From: Kinglong Mee Date: Fri, 22 Aug 2014 10:18:42 -0400 Subject: locks: Rename __locks_copy_lock() to locks_copy_conflock() Jeff advice, " Right now __locks_copy_lock is only used to copy conflocks. It would be good to rename that to something more distinct (i.e.locks_copy_conflock), to make it clear that we're generating a conflock there." v5: change order from 3/6 to 2/6 v4: new patch only renaming function name Signed-off-by: Kinglong Mee Signed-off-by: Jeff Layton --- fs/locks.c | 10 +++++----- include/linux/fs.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index bb08857f90b5..ec9becd02d3d 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -281,7 +281,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl) /* * Initialize a new lock from an existing file_lock structure. */ -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl) +void locks_copy_conflock(struct file_lock *new, struct file_lock *fl) { new->fl_owner = fl->fl_owner; new->fl_pid = fl->fl_pid; @@ -293,14 +293,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl) new->fl_ops = NULL; new->fl_lmops = NULL; } -EXPORT_SYMBOL(__locks_copy_lock); +EXPORT_SYMBOL(locks_copy_conflock); void locks_copy_lock(struct file_lock *new, struct file_lock *fl) { /* "new" must be a freshly-initialized lock */ WARN_ON_ONCE(new->fl_ops); - __locks_copy_lock(new, fl); + locks_copy_conflock(new, fl); new->fl_file = fl->fl_file; new->fl_ops = fl->fl_ops; new->fl_lmops = fl->fl_lmops; @@ -735,7 +735,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl) break; } if (cfl) { - __locks_copy_lock(fl, cfl); + locks_copy_conflock(fl, cfl); if (cfl->fl_nspid) fl->fl_pid = pid_vnr(cfl->fl_nspid); } else @@ -941,7 +941,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str if (!posix_locks_conflict(request, fl)) continue; if (conflock) - __locks_copy_lock(conflock, fl); + locks_copy_conflock(conflock, fl); error = -EAGAIN; if (!(request->fl_flags & FL_SLEEP)) goto out; diff --git a/include/linux/fs.h b/include/linux/fs.h index 908af4f81680..5ab86f44b697 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -966,7 +966,7 @@ void locks_free_lock(struct file_lock *fl); extern void locks_init_lock(struct file_lock *); extern struct file_lock * locks_alloc_lock(void); extern void locks_copy_lock(struct file_lock *, struct file_lock *); -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *); +extern void locks_copy_conflock(struct file_lock *, struct file_lock *); extern void locks_remove_posix(struct file *, fl_owner_t); extern void locks_remove_file(struct file *); extern void locks_release_private(struct file_lock *); @@ -1026,7 +1026,7 @@ static inline void locks_init_lock(struct file_lock *fl) return; } -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl) +static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl) { return; } -- cgit v1.2.3 From 5c97d7b1479982a48cf2129062b880c2555049ac Mon Sep 17 00:00:00 2001 From: Kinglong Mee Date: Fri, 22 Aug 2014 10:18:43 -0400 Subject: locks: New ops in lock_manager_operations for get/put owner NFSD or other lockmanager may increase the owner's reference, so adds two new options for copying and releasing owner. v5: change order from 2/6 to 3/6 v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner Reviewed-by: Jeff Layton Signed-off-by: Kinglong Mee Signed-off-by: Jeff Layton --- fs/locks.c | 12 ++++++++++-- include/linux/fs.h | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index ec9becd02d3d..5e83f3a99377 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl) fl->fl_ops->fl_release_private(fl); fl->fl_ops = NULL; } - fl->fl_lmops = NULL; + if (fl->fl_lmops) { + if (fl->fl_lmops->lm_put_owner) + fl->fl_lmops->lm_put_owner(fl); + fl->fl_lmops = NULL; + } } EXPORT_SYMBOL_GPL(locks_release_private); @@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl) fl->fl_ops->fl_copy_lock(new, fl); new->fl_ops = fl->fl_ops; } - if (fl->fl_lmops) + + if (fl->fl_lmops) { + if (fl->fl_lmops->lm_get_owner) + fl->fl_lmops->lm_get_owner(new, fl); new->fl_lmops = fl->fl_lmops; + } } /* diff --git a/include/linux/fs.h b/include/linux/fs.h index 5ab86f44b697..3b07ce2698de 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -868,6 +868,8 @@ struct file_lock_operations { struct lock_manager_operations { int (*lm_compare_owner)(struct file_lock *, struct file_lock *); unsigned long (*lm_owner_key)(struct file_lock *); + void (*lm_get_owner)(struct file_lock *, struct file_lock *); + void (*lm_put_owner)(struct file_lock *); void (*lm_notify)(struct file_lock *); /* unblock callback */ int (*lm_grant)(struct file_lock *, int); void (*lm_break)(struct file_lock *); -- cgit v1.2.3 From f328296e27414394f25cebaef4a111a82ce0df32 Mon Sep 17 00:00:00 2001 From: Kinglong Mee Date: Fri, 22 Aug 2014 10:18:43 -0400 Subject: locks: Copy fl_lmops information for conflock in locks_copy_conflock() Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using fl_lmops field in file_lock for checking nfsd4 lockowner. But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return of conflicting locks) causes the fl_lmops of conflock always be NULL. Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock always be NULL too. Make sure copy the private information by fl_copy_lock() in struct file_lock_operations, merge __locks_copy_lock() to fl_copy_lock(). Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are superfluous, since they are callbacks into the filesystem. There should be no need to bother the filesystem at all with info in a conflock. But, lock _ownership_ matters for conflocks and that's indicated by the fl_lmops. So you really do want to copy the fl_lmops for conflocks I think." v5: add missing calling of locks_release_private() in nlmsvc_testlock() v4: only copy fl_lmops for conflock, don't copy fl_ops Signed-off-by: Kinglong Mee Signed-off-by: Jeff Layton --- fs/lockd/svclock.c | 1 + fs/locks.c | 36 ++++++++++++++++-------------------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index 2a6170133c1d..acfa94d5b489 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -586,6 +586,7 @@ conf_lock: conflock->fl.fl_type = lock->fl.fl_type; conflock->fl.fl_start = lock->fl.fl_start; conflock->fl.fl_end = lock->fl.fl_end; + locks_release_private(&lock->fl); ret = nlm_lck_denied; out: if (block) diff --git a/fs/locks.c b/fs/locks.c index 5e83f3a99377..c3991dc80137 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl) EXPORT_SYMBOL(locks_init_lock); -static void locks_copy_private(struct file_lock *new, struct file_lock *fl) -{ - if (fl->fl_ops) { - if (fl->fl_ops->fl_copy_lock) - fl->fl_ops->fl_copy_lock(new, fl); - new->fl_ops = fl->fl_ops; - } - - if (fl->fl_lmops) { - if (fl->fl_lmops->lm_get_owner) - fl->fl_lmops->lm_get_owner(new, fl); - new->fl_lmops = fl->fl_lmops; - } -} - /* * Initialize a new lock from an existing file_lock structure. */ @@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl) new->fl_type = fl->fl_type; new->fl_start = fl->fl_start; new->fl_end = fl->fl_end; + new->fl_lmops = fl->fl_lmops; new->fl_ops = NULL; - new->fl_lmops = NULL; + + if (fl->fl_lmops) { + if (fl->fl_lmops->lm_get_owner) + fl->fl_lmops->lm_get_owner(new, fl); + } } EXPORT_SYMBOL(locks_copy_conflock); @@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl) WARN_ON_ONCE(new->fl_ops); locks_copy_conflock(new, fl); + new->fl_file = fl->fl_file; new->fl_ops = fl->fl_ops; - new->fl_lmops = fl->fl_lmops; - locks_copy_private(new, fl); + if (fl->fl_ops) { + if (fl->fl_ops->fl_copy_lock) + fl->fl_ops->fl_copy_lock(new, fl); + } } EXPORT_SYMBOL(locks_copy_lock); @@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l) if (file_lock.fl_type != F_UNLCK) { error = posix_lock_to_flock(&flock, &file_lock); if (error) - goto out; + goto rel_priv; } error = -EFAULT; if (!copy_to_user(l, &flock, sizeof(flock))) error = 0; +rel_priv: + locks_release_private(&file_lock); out: return error; } @@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l) error = -EFAULT; if (!copy_to_user(l, &flock, sizeof(flock))) error = 0; - + + locks_release_private(&file_lock); out: return error; } -- cgit v1.2.3 From b5971afa0b33361667bc88f3e0eb3fc31f778dc6 Mon Sep 17 00:00:00 2001 From: Kinglong Mee Date: Fri, 22 Aug 2014 10:18:43 -0400 Subject: NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference v5: same as the first version Reviewed-by: Jeff Layton Signed-off-by: Kinglong Mee Signed-off-by: Jeff Layton --- fs/nfsd/nfs4state.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index e773036b03d0..2d03a4188671 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses) spin_unlock(&nn->client_lock); } +static inline struct nfs4_stateowner * +nfs4_get_stateowner(struct nfs4_stateowner *sop) +{ + atomic_inc(&sop->so_count); + return sop; +} + static int same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner) { @@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open, so_strhash) { if (!so->so_is_open_owner) continue; - if (same_owner_str(so, &open->op_owner)) { - atomic_inc(&so->so_count); - return openowner(so); - } + if (same_owner_str(so, &open->op_owner)) + return openowner(nfs4_get_stateowner(so)); } return NULL; } @@ -1651,7 +1656,7 @@ __destroy_client(struct nfs4_client *clp) } while (!list_empty(&clp->cl_openowners)) { oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient); - atomic_inc(&oo->oo_owner.so_count); + nfs4_get_stateowner(&oo->oo_owner); release_openowner(oo); } nfsd4_shutdown_callback(clp); @@ -3132,8 +3137,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate, { if (!nfsd4_has_session(cstate)) { mutex_lock(&so->so_replay.rp_mutex); - cstate->replay_owner = so; - atomic_inc(&so->so_count); + cstate->replay_owner = nfs4_get_stateowner(so); } } @@ -3232,8 +3236,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, atomic_inc(&stp->st_stid.sc_count); stp->st_stid.sc_type = NFS4_OPEN_STID; INIT_LIST_HEAD(&stp->st_locks); - stp->st_stateowner = &oo->oo_owner; - atomic_inc(&stp->st_stateowner->so_count); + stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner); get_nfs4_file(fp); stp->st_stid.sc_file = fp; stp->st_access_bmap = 0; @@ -4921,10 +4924,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner, so_strhash) { if (so->so_is_open_owner) continue; - if (!same_owner_str(so, owner)) - continue; - atomic_inc(&so->so_count); - return lockowner(so); + if (same_owner_str(so, owner)) + return lockowner(nfs4_get_stateowner(so)); } return NULL; } @@ -5003,8 +5004,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo, atomic_inc(&stp->st_stid.sc_count); stp->st_stid.sc_type = NFS4_LOCK_STID; - stp->st_stateowner = &lo->lo_owner; - atomic_inc(&lo->lo_owner.so_count); + stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner); get_nfs4_file(fp); stp->st_stid.sc_file = fp; stp->st_stid.sc_free = nfs4_free_lock_stateid; @@ -5546,7 +5546,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, } } - atomic_inc(&sop->so_count); + nfs4_get_stateowner(sop); break; } spin_unlock(&clp->cl_lock); -- cgit v1.2.3 From aef9583b234a4ecdbcaf2c3024f29d4244b18e83 Mon Sep 17 00:00:00 2001 From: Kinglong Mee Date: Fri, 22 Aug 2014 10:18:44 -0400 Subject: NFSD: Get reference of lockowner when coping file_lock v5: using nfs4_get_stateowner() instead of an inline function v3: Update based on Jeff's comments v2: Fix bad using of struct file_lock_operations for handle the owner Acked-by: Jeff Layton Signed-off-by: Kinglong Mee Signed-off-by: Jeff Layton --- fs/nfsd/nfs4state.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 2d03a4188671..a91e521622c3 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4876,9 +4876,25 @@ nfs4_transform_lock_offset(struct file_lock *lock) lock->fl_end = OFFSET_MAX; } -/* Hack!: For now, we're defining this just so we can use a pointer to it - * as a unique cookie to identify our (NFSv4's) posix locks. */ +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src) +{ + struct nfs4_lockowner *lo = (struct nfs4_lockowner *)src->fl_owner; + dst->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lo->lo_owner)); +} + +static void nfsd4_fl_put_owner(struct file_lock *fl) +{ + struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner; + + if (lo) { + nfs4_put_stateowner(&lo->lo_owner); + fl->fl_owner = NULL; + } +} + static const struct lock_manager_operations nfsd_posix_mng_ops = { + .lm_get_owner = nfsd4_fl_get_owner, + .lm_put_owner = nfsd4_fl_put_owner, }; static inline void @@ -5243,7 +5259,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, status = nfserr_openmode; goto out; } - file_lock->fl_owner = (fl_owner_t)lock_sop; + + file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lock_sop->lo_owner)); file_lock->fl_pid = current->tgid; file_lock->fl_file = filp; file_lock->fl_flags = FL_POSIX; @@ -5439,7 +5456,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, } file_lock->fl_type = F_UNLCK; - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner); + file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner)); file_lock->fl_pid = current->tgid; file_lock->fl_file = filp; file_lock->fl_flags = FL_POSIX; -- cgit v1.2.3 From 09802fd2a8caea2a2147fca8d7975697c5de573d Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:18:44 -0400 Subject: lockd: rip out deferred lock handling from testlock codepath As Kinglong points out, the nlm_block->b_fl field is no longer used at all. Also, vfs_test_lock in the generic locking code will only return FILE_LOCK_DEFERRED if FL_SLEEP is set, and it isn't here. The only other place that returns that value is the DLM lock code, but it only does that in dlm_posix_lock, never in dlm_posix_get. Remove all of the deferred locking code from the testlock codepath since it doesn't appear to ever be used anyway. I do have a small concern that this might cause a behavior change in the case where you have a block already sitting on the list when the testlock request comes in, but that looks like it doesn't really work properly anyway. I think it's best to just pass that down to vfs_test_lock and let the filesystem report that instead of trying to infer what's going on with the lock by looking at an existing block. Cc: cluster-devel@redhat.com Signed-off-by: Jeff Layton Reviewed-by: Kinglong Mee --- fs/lockd/svclock.c | 55 +++++---------------------------------------- include/linux/lockd/lockd.h | 1 - 2 files changed, 6 insertions(+), 50 deletions(-) diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index acfa94d5b489..13db95f54176 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -245,7 +245,6 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host, block->b_daemon = rqstp->rq_server; block->b_host = host; block->b_file = file; - block->b_fl = NULL; file->f_count++; /* Add to file's list of blocks */ @@ -295,7 +294,6 @@ static void nlmsvc_free_block(struct kref *kref) nlmsvc_freegrantargs(block->b_call); nlmsvc_release_call(block->b_call); nlm_release_file(block->b_file); - kfree(block->b_fl); kfree(block); } @@ -508,7 +506,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, struct nlm_host *host, struct nlm_lock *lock, struct nlm_lock *conflock, struct nlm_cookie *cookie) { - struct nlm_block *block = NULL; int error; __be32 ret; @@ -519,63 +516,26 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, (long long)lock->fl.fl_start, (long long)lock->fl.fl_end); - /* Get existing block (in case client is busy-waiting) */ - block = nlmsvc_lookup_block(file, lock); - - if (block == NULL) { - struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL); - - if (conf == NULL) - return nlm_granted; - block = nlmsvc_create_block(rqstp, host, file, lock, cookie); - if (block == NULL) { - kfree(conf); - return nlm_granted; - } - block->b_fl = conf; - } - if (block->b_flags & B_QUEUED) { - dprintk("lockd: nlmsvc_testlock deferred block %p flags %d fl %p\n", - block, block->b_flags, block->b_fl); - if (block->b_flags & B_TIMED_OUT) { - nlmsvc_unlink_block(block); - ret = nlm_lck_denied; - goto out; - } - if (block->b_flags & B_GOT_CALLBACK) { - nlmsvc_unlink_block(block); - if (block->b_fl != NULL - && block->b_fl->fl_type != F_UNLCK) { - lock->fl = *block->b_fl; - goto conf_lock; - } else { - ret = nlm_granted; - goto out; - } - } - ret = nlm_drop_reply; - goto out; - } - if (locks_in_grace(SVC_NET(rqstp))) { ret = nlm_lck_denied_grace_period; goto out; } + error = vfs_test_lock(file->f_file, &lock->fl); - if (error == FILE_LOCK_DEFERRED) { - ret = nlmsvc_defer_lock_rqst(rqstp, block); - goto out; - } if (error) { + /* We can't currently deal with deferred test requests */ + if (error == FILE_LOCK_DEFERRED) + WARN_ON_ONCE(1); + ret = nlm_lck_denied_nolocks; goto out; } + if (lock->fl.fl_type == F_UNLCK) { ret = nlm_granted; goto out; } -conf_lock: dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n", lock->fl.fl_type, (long long)lock->fl.fl_start, (long long)lock->fl.fl_end); @@ -589,8 +549,6 @@ conf_lock: locks_release_private(&lock->fl); ret = nlm_lck_denied; out: - if (block) - nlmsvc_release_block(block); return ret; } @@ -661,7 +619,6 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l * This is a callback from the filesystem for VFS file lock requests. * It will be used if lm_grant is defined and the filesystem can not * respond to the request immediately. - * For GETLK request it will copy the reply to the nlm_block. * For SETLK or SETLKW request it will get the local posix lock. * In all cases it will move the block to the head of nlm_blocked q where * nlmsvc_retry_blocked() can send back a reply for SETLKW or revisit the diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h index 219d79627c05..ff82a32871b5 100644 --- a/include/linux/lockd/lockd.h +++ b/include/linux/lockd/lockd.h @@ -178,7 +178,6 @@ struct nlm_block { unsigned char b_granted; /* VFS granted lock */ struct nlm_file * b_file; /* file in question */ struct cache_req * b_cache_req; /* deferred request handling */ - struct file_lock * b_fl; /* set for GETLK */ struct cache_deferred_req * b_deferred_req; unsigned int b_flags; /* block flags */ #define B_QUEUED 1 /* lock queued */ -- cgit v1.2.3 From 699688a416524c3cea9eafaca69fc6c06c13c02e Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:18:44 -0400 Subject: locks: remove lock_may_read and lock_may_write There are no callers of these functions. Signed-off-by: Jeff Layton --- fs/locks.c | 80 ------------------------------------------------------ include/linux/fs.h | 14 ---------- 2 files changed, 94 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index c3991dc80137..5200ffd2ba9b 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -2597,86 +2597,6 @@ static int __init proc_locks_init(void) module_init(proc_locks_init); #endif -/** - * lock_may_read - checks that the region is free of locks - * @inode: the inode that is being read - * @start: the first byte to read - * @len: the number of bytes to read - * - * Emulates Windows locking requirements. Whole-file - * mandatory locks (share modes) can prohibit a read and - * byte-range POSIX locks can prohibit a read if they overlap. - * - * N.B. this function is only ever called - * from knfsd and ownership of locks is never checked. - */ -int lock_may_read(struct inode *inode, loff_t start, unsigned long len) -{ - struct file_lock *fl; - int result = 1; - - spin_lock(&inode->i_lock); - for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) { - if (IS_POSIX(fl)) { - if (fl->fl_type == F_RDLCK) - continue; - if ((fl->fl_end < start) || (fl->fl_start > (start + len))) - continue; - } else if (IS_FLOCK(fl)) { - if (!(fl->fl_type & LOCK_MAND)) - continue; - if (fl->fl_type & LOCK_READ) - continue; - } else - continue; - result = 0; - break; - } - spin_unlock(&inode->i_lock); - return result; -} - -EXPORT_SYMBOL(lock_may_read); - -/** - * lock_may_write - checks that the region is free of locks - * @inode: the inode that is being written - * @start: the first byte to write - * @len: the number of bytes to write - * - * Emulates Windows locking requirements. Whole-file - * mandatory locks (share modes) can prohibit a write and - * byte-range POSIX locks can prohibit a write if they overlap. - * - * N.B. this function is only ever called - * from knfsd and ownership of locks is never checked. - */ -int lock_may_write(struct inode *inode, loff_t start, unsigned long len) -{ - struct file_lock *fl; - int result = 1; - - spin_lock(&inode->i_lock); - for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) { - if (IS_POSIX(fl)) { - if ((fl->fl_end < start) || (fl->fl_start > (start + len))) - continue; - } else if (IS_FLOCK(fl)) { - if (!(fl->fl_type & LOCK_MAND)) - continue; - if (fl->fl_type & LOCK_WRITE) - continue; - } else - continue; - result = 0; - break; - } - spin_unlock(&inode->i_lock); - return result; -} - -EXPORT_SYMBOL(lock_may_write); - static int __init filelock_init(void) { int i; diff --git a/include/linux/fs.h b/include/linux/fs.h index 3b07ce2698de..458f733c96bd 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -985,8 +985,6 @@ extern void lease_get_mtime(struct inode *, struct timespec *time); extern int generic_setlease(struct file *, long, struct file_lock **); extern int vfs_setlease(struct file *, long, struct file_lock **); extern int lease_modify(struct file_lock **, int); -extern int lock_may_read(struct inode *, loff_t start, unsigned long count); -extern int lock_may_write(struct inode *, loff_t start, unsigned long count); #else /* !CONFIG_FILE_LOCKING */ static inline int fcntl_getlk(struct file *file, unsigned int cmd, struct flock __user *user) @@ -1117,18 +1115,6 @@ static inline int lease_modify(struct file_lock **before, int arg) { return -EINVAL; } - -static inline int lock_may_read(struct inode *inode, loff_t start, - unsigned long len) -{ - return 1; -} - -static inline int lock_may_write(struct inode *inode, loff_t start, - unsigned long len) -{ - return 1; -} #endif /* !CONFIG_FILE_LOCKING */ -- cgit v1.2.3 From 1c994a0909a556508c2cc26ab5d9e13c5ce33aa0 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Wed, 27 Aug 2014 06:49:41 -0400 Subject: locks: consolidate "nolease" routines GFS2 and NFS have setlease routines that always just return -EINVAL. Turn that into a generic routine that can live in fs/libfs.c. Cc: Cc: Steven Whitehouse Cc: Signed-off-by: Jeff Layton Acked-by: Trond Myklebust Reviewed-by: Christoph Hellwig --- fs/gfs2/file.c | 22 +--------------------- fs/libfs.c | 16 ++++++++++++++++ fs/nfs/file.c | 13 +------------ fs/nfs/internal.h | 1 - fs/nfs/nfs4file.c | 2 +- include/linux/fs.h | 1 + 6 files changed, 20 insertions(+), 35 deletions(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 26b3f952e6b1..2c02478a86b0 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -912,26 +912,6 @@ out_uninit: #ifdef CONFIG_GFS2_FS_LOCKING_DLM -/** - * gfs2_setlease - acquire/release a file lease - * @file: the file pointer - * @arg: lease type - * @fl: file lock - * - * We don't currently have a way to enforce a lease across the whole - * cluster; until we do, disable leases (by just returning -EINVAL), - * unless the administrator has requested purely local locking. - * - * Locking: called under i_lock - * - * Returns: errno - */ - -static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl) -{ - return -EINVAL; -} - /** * gfs2_lock - acquire/release a posix lock on a file * @file: the file pointer @@ -1069,7 +1049,7 @@ const struct file_operations gfs2_file_fops = { .flock = gfs2_flock, .splice_read = generic_file_splice_read, .splice_write = iter_file_splice_write, - .setlease = gfs2_setlease, + .setlease = simple_nosetlease, .fallocate = gfs2_fallocate, }; diff --git a/fs/libfs.c b/fs/libfs.c index 88e3e00e2eca..29012a303ef8 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1075,3 +1075,19 @@ struct inode *alloc_anon_inode(struct super_block *s) return inode; } EXPORT_SYMBOL(alloc_anon_inode); + +/** + * simple_nosetlease - generic helper for prohibiting leases + * @filp: file pointer + * @arg: type of lease to obtain + * @flp: new lease supplied for insertion + * + * Generic helper for filesystems that do not wish to allow leases to be set. + * All arguments are ignored and it just returns -EINVAL. + */ +int +simple_nosetlease(struct file *filp, long arg, struct file_lock **flp) +{ + return -EINVAL; +} +EXPORT_SYMBOL(simple_nosetlease); diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 524dd80d1898..8c4048ecdad1 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -891,17 +891,6 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) } EXPORT_SYMBOL_GPL(nfs_flock); -/* - * There is no protocol support for leases, so we have no way to implement - * them correctly in the face of opens by other clients. - */ -int nfs_setlease(struct file *file, long arg, struct file_lock **fl) -{ - dprintk("NFS: setlease(%pD2, arg=%ld)\n", file, arg); - return -EINVAL; -} -EXPORT_SYMBOL_GPL(nfs_setlease); - const struct file_operations nfs_file_operations = { .llseek = nfs_file_llseek, .read = new_sync_read, @@ -918,6 +907,6 @@ const struct file_operations nfs_file_operations = { .splice_read = nfs_file_splice_read, .splice_write = iter_file_splice_write, .check_flags = nfs_check_flags, - .setlease = nfs_setlease, + .setlease = simple_nosetlease, }; EXPORT_SYMBOL_GPL(nfs_file_operations); diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index 9056622d2230..94d922ebb5ac 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -346,7 +346,6 @@ int nfs_file_release(struct inode *, struct file *); int nfs_lock(struct file *, int, struct file_lock *); int nfs_flock(struct file *, int, struct file_lock *); int nfs_check_flags(int); -int nfs_setlease(struct file *, long, struct file_lock **); /* inode.c */ extern struct workqueue_struct *nfsiod_workqueue; diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index a816f0627a6c..3e987ad9ae25 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -131,5 +131,5 @@ const struct file_operations nfs4_file_operations = { .splice_read = nfs_file_splice_read, .splice_write = iter_file_splice_write, .check_flags = nfs_check_flags, - .setlease = nfs_setlease, + .setlease = simple_nosetlease, }; diff --git a/include/linux/fs.h b/include/linux/fs.h index 458f733c96bd..435e3d9ec5cf 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2599,6 +2599,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping, struct page *page, void *fsdata); extern int always_delete_dentry(const struct dentry *); extern struct inode *alloc_anon_inode(struct super_block *); +extern int simple_nosetlease(struct file *, long, struct file_lock **); extern const struct dentry_operations simple_dentry_operations; extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags); -- cgit v1.2.3 From e0b93eddfe17dcb7d644eb5d6ad02a86fc41a977 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 11:27:32 -0400 Subject: security: make security_file_set_fowner, f_setown and __f_setown void return security_file_set_fowner always returns 0, so make it f_setown and __f_setown void return functions and fix up the error handling in the callers. Cc: linux-security-module@vger.kernel.org Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- drivers/net/tun.c | 4 +--- drivers/tty/tty_io.c | 3 ++- fs/fcntl.c | 21 +++++++-------------- fs/locks.c | 2 +- fs/notify/dnotify/dnotify.c | 8 +------- include/linux/fs.h | 4 ++-- include/linux/security.h | 8 ++++---- net/socket.c | 3 ++- security/capability.c | 4 ++-- security/security.c | 4 ++-- security/selinux/hooks.c | 4 +--- security/smack/smack_lsm.c | 3 +-- 12 files changed, 26 insertions(+), 42 deletions(-) diff --git a/drivers/net/tun.c b/drivers/net/tun.c index acaaf6784179..186ce541c657 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -2152,9 +2152,7 @@ static int tun_chr_fasync(int fd, struct file *file, int on) goto out; if (on) { - ret = __f_setown(file, task_pid(current), PIDTYPE_PID, 0); - if (ret) - goto out; + __f_setown(file, task_pid(current), PIDTYPE_PID, 0); tfile->flags |= TUN_FASYNC; } else tfile->flags &= ~TUN_FASYNC; diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 8fbad3410c75..aea3b66f7bf2 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -2163,8 +2163,9 @@ static int __tty_fasync(int fd, struct file *filp, int on) } get_pid(pid); spin_unlock_irqrestore(&tty->ctrl_lock, flags); - retval = __f_setown(filp, pid, type, 0); + __f_setown(filp, pid, type, 0); put_pid(pid); + retval = 0; } out: return retval; diff --git a/fs/fcntl.c b/fs/fcntl.c index 22d1c3df61ac..99d440a4a6ba 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -98,26 +98,19 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, write_unlock_irq(&filp->f_owner.lock); } -int __f_setown(struct file *filp, struct pid *pid, enum pid_type type, +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type, int force) { - int err; - - err = security_file_set_fowner(filp); - if (err) - return err; - + security_file_set_fowner(filp); f_modown(filp, pid, type, force); - return 0; } EXPORT_SYMBOL(__f_setown); -int f_setown(struct file *filp, unsigned long arg, int force) +void f_setown(struct file *filp, unsigned long arg, int force) { enum pid_type type; struct pid *pid; int who = arg; - int result; type = PIDTYPE_PID; if (who < 0) { type = PIDTYPE_PGID; @@ -125,9 +118,8 @@ int f_setown(struct file *filp, unsigned long arg, int force) } rcu_read_lock(); pid = find_vpid(who); - result = __f_setown(filp, pid, type, force); + __f_setown(filp, pid, type, force); rcu_read_unlock(); - return result; } EXPORT_SYMBOL(f_setown); @@ -181,7 +173,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg) if (owner.pid && !pid) ret = -ESRCH; else - ret = __f_setown(filp, pid, type, 1); + __f_setown(filp, pid, type, 1); rcu_read_unlock(); return ret; @@ -302,7 +294,8 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, force_successful_syscall_return(); break; case F_SETOWN: - err = f_setown(filp, arg, 1); + f_setown(filp, arg, 1); + err = 0; break; case F_GETOWN_EX: err = f_getown_ex(filp, arg); diff --git a/fs/locks.c b/fs/locks.c index 5200ffd2ba9b..f5f648e003dd 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1776,7 +1776,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new)) new = NULL; - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); out_unlock: spin_unlock(&inode->i_lock); if (fl) diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index abc8cbcfe90e..caaaf9dfe353 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -346,13 +346,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) goto out; } - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); - if (error) { - /* if we added, we must shoot */ - if (dn_mark == new_dn_mark) - destroy = 1; - goto out; - } + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); error = attach_dn(dn, dn_mark, id, fd, filp, mask); /* !error means that we attached the dn to the dn_mark, so don't free it */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 435e3d9ec5cf..96528f73dda4 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1139,8 +1139,8 @@ extern void fasync_free(struct fasync_struct *); /* can be called from interrupts */ extern void kill_fasync(struct fasync_struct **, int, int); -extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int force); -extern int f_setown(struct file *filp, unsigned long arg, int force); +extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force); +extern void f_setown(struct file *filp, unsigned long arg, int force); extern void f_delown(struct file *filp); extern pid_t f_getown(struct file *filp); extern int send_sigurg(struct fown_struct *fown); diff --git a/include/linux/security.h b/include/linux/security.h index 623f90e5f38d..b10e7af95d3b 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1559,7 +1559,7 @@ struct security_operations { int (*file_lock) (struct file *file, unsigned int cmd); int (*file_fcntl) (struct file *file, unsigned int cmd, unsigned long arg); - int (*file_set_fowner) (struct file *file); + void (*file_set_fowner) (struct file *file); int (*file_send_sigiotask) (struct task_struct *tsk, struct fown_struct *fown, int sig); int (*file_receive) (struct file *file); @@ -1834,7 +1834,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot); int security_file_lock(struct file *file, unsigned int cmd); int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg); -int security_file_set_fowner(struct file *file); +void security_file_set_fowner(struct file *file); int security_file_send_sigiotask(struct task_struct *tsk, struct fown_struct *fown, int sig); int security_file_receive(struct file *file); @@ -2312,9 +2312,9 @@ static inline int security_file_fcntl(struct file *file, unsigned int cmd, return 0; } -static inline int security_file_set_fowner(struct file *file) +static inline void security_file_set_fowner(struct file *file) { - return 0; + return; } static inline int security_file_send_sigiotask(struct task_struct *tsk, diff --git a/net/socket.c b/net/socket.c index 95ee7d8682e7..769c9671847e 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1069,7 +1069,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg) err = -EFAULT; if (get_user(pid, (int __user *)argp)) break; - err = f_setown(sock->file, pid, 1); + f_setown(sock->file, pid, 1); + err = 0; break; case FIOGETOWN: case SIOCGPGRP: diff --git a/security/capability.c b/security/capability.c index a74fde6a7468..d68c57a62bcf 100644 --- a/security/capability.c +++ b/security/capability.c @@ -343,9 +343,9 @@ static int cap_file_fcntl(struct file *file, unsigned int cmd, return 0; } -static int cap_file_set_fowner(struct file *file) +static void cap_file_set_fowner(struct file *file) { - return 0; + return; } static int cap_file_send_sigiotask(struct task_struct *tsk, diff --git a/security/security.c b/security/security.c index e41b1a8d7644..18b35c63fc0c 100644 --- a/security/security.c +++ b/security/security.c @@ -775,9 +775,9 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg) return security_ops->file_fcntl(file, cmd, arg); } -int security_file_set_fowner(struct file *file) +void security_file_set_fowner(struct file *file) { - return security_ops->file_set_fowner(file); + security_ops->file_set_fowner(file); } int security_file_send_sigiotask(struct task_struct *tsk, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index b0e940497e23..ada0d0bf3463 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3346,14 +3346,12 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd, return err; } -static int selinux_file_set_fowner(struct file *file) +static void selinux_file_set_fowner(struct file *file) { struct file_security_struct *fsec; fsec = file->f_security; fsec->fown_sid = current_sid(); - - return 0; } static int selinux_file_send_sigiotask(struct task_struct *tsk, diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index e6ab307ce86e..69e5635d89e5 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1390,12 +1390,11 @@ static int smack_mmap_file(struct file *file, * Returns 0 * Further research may be required on this one. */ -static int smack_file_set_fowner(struct file *file) +static void smack_file_set_fowner(struct file *file) { struct smack_known *skp = smk_of_current(); file->f_security = skp->smk_known; - return 0; } /** -- cgit v1.2.3 From bfe8602436c803c6d5e271d52cd985d491a7470a Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:18:44 -0400 Subject: locks: close potential race in lease_get_mtime lease_get_mtime is called without the i_lock held, so there's no guarantee about the stability of the list. Between the time when we assign "flock" and then dereference it to check whether it's a lease and for write, the lease could be freed. Ensure that that doesn't occur by taking the i_lock before trying to check the lease. Cc: J. Bruce Fields Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index f5f648e003dd..def1ac2e87bd 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1456,8 +1456,18 @@ EXPORT_SYMBOL(__break_lease); */ void lease_get_mtime(struct inode *inode, struct timespec *time) { - struct file_lock *flock = inode->i_flock; - if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK)) + bool has_lease = false; + struct file_lock *flock; + + if (inode->i_flock) { + spin_lock(&inode->i_lock); + flock = inode->i_flock; + if (flock && IS_LEASE(flock) && (flock->fl_type == F_WRLCK)) + has_lease = true; + spin_unlock(&inode->i_lock); + } + + if (has_lease) *time = current_fs_time(inode->i_sb); else *time = inode->i_mtime; -- cgit v1.2.3 From 415b96c5a1fe31ed9deb0618e95ecbb1df3de54c Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 12:26:36 -0400 Subject: nfsd: fix potential lease memory leak in nfs4_setlease It's unlikely to ever occur, but if there were already a lease set on the file then we could end up getting back a different pointer on a successful setlease attempt than the one we allocated. If that happens, the one we allocated could leak. In practice, I don't think this will happen due to the fact that we only try to set up the lease once per nfs4_file, but this error handling is a bit more correct given the current lease API. Cc: J. Bruce Fields Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/nfsd/nfs4state.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index a91e521622c3..5bb4952faf5b 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3781,7 +3781,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag) static int nfs4_setlease(struct nfs4_delegation *dp) { struct nfs4_file *fp = dp->dl_stid.sc_file; - struct file_lock *fl; + struct file_lock *fl, *ret; struct file *filp; int status = 0; @@ -3795,11 +3795,14 @@ static int nfs4_setlease(struct nfs4_delegation *dp) return -EBADF; } fl->fl_file = filp; - status = vfs_setlease(filp, fl->fl_type, &fl); + ret = fl; + status = vfs_setlease(filp, fl->fl_type, &ret); if (status) { locks_free_lock(fl); goto out_fput; } + if (ret != fl) + locks_free_lock(fl); spin_lock(&state_lock); spin_lock(&fp->fi_lock); /* Did the lease get broken before we took the lock? */ -- cgit v1.2.3 From 0efaa7e82f02fe69c05ad28e905f31fc86e6f08e Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:18:45 -0400 Subject: locks: generic_delete_lease doesn't need a file_lock at all Ensure that it's OK to pass in a NULL file_lock double pointer on a F_UNLCK request and convert the vfs_setlease F_UNLCK callers to do just that. Finally, turn the BUG_ON in generic_setlease into a WARN_ON_ONCE with an error return. That's a problem we can handle without crashing the box if it occurs. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 34 ++++++++++++++-------------------- fs/nfsd/nfs4state.c | 2 +- include/trace/events/filelock.h | 14 +++++++------- 3 files changed, 22 insertions(+), 28 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index def1ac2e87bd..f79c74ef51ef 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1637,22 +1637,23 @@ out: return error; } -static int generic_delete_lease(struct file *filp, struct file_lock **flp) +static int generic_delete_lease(struct file *filp) { + int error = -EAGAIN; struct file_lock *fl, **before; struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; - trace_generic_delete_lease(inode, *flp); - for (before = &inode->i_flock; ((fl = *before) != NULL) && IS_LEASE(fl); before = &fl->fl_next) { - if (fl->fl_file != filp) - continue; - return (*flp)->fl_lmops->lm_change(before, F_UNLCK); + if (fl->fl_file == filp) + break; } - return -EAGAIN; + trace_generic_delete_lease(inode, fl); + if (fl) + error = fl->fl_lmops->lm_change(before, F_UNLCK); + return error; } /** @@ -1682,13 +1683,15 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp) time_out_leases(inode); - BUG_ON(!(*flp)->fl_lmops->lm_break); - switch (arg) { case F_UNLCK: - return generic_delete_lease(filp, flp); + return generic_delete_lease(filp); case F_RDLCK: case F_WRLCK: + if (!(*flp)->fl_lmops->lm_break) { + WARN_ON_ONCE(1); + return -ENOLCK; + } return generic_add_lease(filp, arg, flp); default: return -EINVAL; @@ -1744,15 +1747,6 @@ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease) } EXPORT_SYMBOL_GPL(vfs_setlease); -static int do_fcntl_delete_lease(struct file *filp) -{ - struct file_lock fl, *flp = &fl; - - lease_init(filp, F_UNLCK, flp); - - return vfs_setlease(filp, F_UNLCK, &flp); -} - static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) { struct file_lock *fl, *ret; @@ -1809,7 +1803,7 @@ out_unlock: int fcntl_setlease(unsigned int fd, struct file *filp, long arg) { if (arg == F_UNLCK) - return do_fcntl_delete_lease(filp); + return vfs_setlease(filp, F_UNLCK, NULL); return do_fcntl_add_lease(fd, filp, arg); } diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 5bb4952faf5b..89d54e505155 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -690,7 +690,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp) spin_unlock(&fp->fi_lock); if (filp) { - vfs_setlease(filp, F_UNLCK, &fl); + vfs_setlease(filp, F_UNLCK, NULL); fput(filp); } } diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h index 59d11c22f076..a0d008070962 100644 --- a/include/trace/events/filelock.h +++ b/include/trace/events/filelock.h @@ -53,15 +53,15 @@ DECLARE_EVENT_CLASS(filelock_lease, ), TP_fast_assign( - __entry->fl = fl; + __entry->fl = fl ? fl : NULL; __entry->s_dev = inode->i_sb->s_dev; __entry->i_ino = inode->i_ino; - __entry->fl_next = fl->fl_next; - __entry->fl_owner = fl->fl_owner; - __entry->fl_flags = fl->fl_flags; - __entry->fl_type = fl->fl_type; - __entry->fl_break_time = fl->fl_break_time; - __entry->fl_downgrade_time = fl->fl_downgrade_time; + __entry->fl_next = fl ? fl->fl_next : NULL; + __entry->fl_owner = fl ? fl->fl_owner : NULL; + __entry->fl_flags = fl ? fl->fl_flags : 0; + __entry->fl_type = fl ? fl->fl_type : 0; + __entry->fl_break_time = fl ? fl->fl_break_time : 0; + __entry->fl_downgrade_time = fl ? fl->fl_downgrade_time : 0; ), TP_printk("fl=0x%p dev=0x%x:0x%x ino=0x%lx fl_next=0x%p fl_owner=0x%p fl_flags=%s fl_type=%s fl_break_time=%lu fl_downgrade_time=%lu", -- cgit v1.2.3 From e51673aa5d9a8c75cc836fac687fa4dde9a76182 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 18:13:28 -0400 Subject: locks: clean up vfs_setlease kerneldoc comments Some of the latter paragraphs seem ambiguous and just plain wrong. In particular the break_lease comment makes no sense. We call break_lease (and break_deleg) from all sorts of vfs-layer functions, so there is clearly such a method. Also get rid of some of the other comments about what's needed for a full implementation. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index f79c74ef51ef..e16c2c61a44f 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1708,30 +1708,16 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease) } /** - * vfs_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @lease: file_lock to use - * - * Call this to establish a lease on the file. - * The (*lease)->fl_lmops->lm_break operation must be set; if not, - * break_lease will oops! - * - * This will call the filesystem's setlease file method, if - * defined. Note that there is no getlease method; instead, the - * filesystem setlease method should call back to setlease() to - * add a lease to the inode's lease list, where fcntl_getlease() can - * find it. Since fcntl_getlease() only reports whether the current - * task holds a lease, a cluster filesystem need only do this for - * leases held by processes on this node. - * - * There is also no break_lease method; filesystems that - * handle their own leases should break leases themselves from the - * filesystem's open, create, and (on truncate) setattr methods. - * - * Warning: the only current setlease methods exist only to disable - * leases in certain cases. More vfs changes may be required to - * allow a full filesystem lease implementation. + * vfs_setlease - sets a lease on an open file + * @filp: file pointer + * @arg: type of lease to obtain + * @lease: file_lock to use when adding a lease + * + * Call this to establish a lease on the file. The "lease" argument is not + * used for F_UNLCK requests and may be NULL. For commands that set or alter + * an existing lease, the (*lease)->fl_lmops->lm_break operation must be set; + * if not, this function will return -ENOLCK (and generate a scary-looking + * stack trace). */ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease) -- cgit v1.2.3 From 0c637be884f5eaa0ee53396ea7686ec0de03d126 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 12:05:43 -0400 Subject: nfsd: don't keep a pointer to the lease in nfs4_file Now that we don't need to pass in an actual lease pointer to vfs_setlease on unlock, we can stop tracking a pointer to the lease in the nfs4_file. Switch all of the places that check the fi_lease to check fi_deleg_file instead. We always set that at the same time so it will have the same semantics. Cc: J. Bruce Fields Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/nfsd/nfs4state.c | 13 ++++--------- fs/nfsd/state.h | 1 - 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 89d54e505155..188cd68aefb6 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -679,14 +679,10 @@ nfs4_put_stid(struct nfs4_stid *s) static void nfs4_put_deleg_lease(struct nfs4_file *fp) { struct file *filp = NULL; - struct file_lock *fl; spin_lock(&fp->fi_lock); - if (fp->fi_lease && atomic_dec_and_test(&fp->fi_delegees)) { + if (fp->fi_deleg_file && atomic_dec_and_test(&fp->fi_delegees)) swap(filp, fp->fi_deleg_file); - fl = fp->fi_lease; - fp->fi_lease = NULL; - } spin_unlock(&fp->fi_lock); if (filp) { @@ -3068,8 +3064,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh) INIT_LIST_HEAD(&fp->fi_stateids); INIT_LIST_HEAD(&fp->fi_delegations); fh_copy_shallow(&fp->fi_fhandle, fh); + fp->fi_deleg_file = NULL; fp->fi_had_conflict = false; - fp->fi_lease = NULL; fp->fi_share_deny = 0; memset(fp->fi_fds, 0, sizeof(fp->fi_fds)); memset(fp->fi_access, 0, sizeof(fp->fi_access)); @@ -3810,13 +3806,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp) if (fp->fi_had_conflict) goto out_unlock; /* Race breaker */ - if (fp->fi_lease) { + if (fp->fi_deleg_file) { status = 0; atomic_inc(&fp->fi_delegees); hash_delegation_locked(dp, fp); goto out_unlock; } - fp->fi_lease = fl; fp->fi_deleg_file = filp; atomic_set(&fp->fi_delegees, 1); hash_delegation_locked(dp, fp); @@ -3849,7 +3844,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, spin_lock(&state_lock); spin_lock(&fp->fi_lock); dp->dl_stid.sc_file = fp; - if (!fp->fi_lease) { + if (!fp->fi_deleg_file) { spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); status = nfs4_setlease(dp); diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 4a89e00d7461..64f291a25a8c 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -477,7 +477,6 @@ struct nfs4_file { atomic_t fi_access[2]; u32 fi_share_deny; struct file *fi_deleg_file; - struct file_lock *fi_lease; atomic_t fi_delegees; struct knfsd_fh fi_fhandle; bool fi_had_conflict; -- cgit v1.2.3 From e6f5c78930e409f3a6b37f5484313a416359ac7f Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:40:25 -0400 Subject: locks: plumb a "priv" pointer into the setlease routines In later patches, we're going to add a new lock_manager_operation to finish setting up the lease while still holding the i_lock. To do this, we'll need to pass a little bit of info in the fcntl setlease case (primarily an fasync structure). Plumb the extra pointer into there in advance of that. We declare this pointer as a void ** to make it clear that this is private info, and that the caller isn't required to set this unless the lm_setup specifically requires it. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- Documentation/filesystems/Locking | 2 +- Documentation/filesystems/vfs.txt | 2 +- fs/cifs/cifsfs.c | 7 ++++--- fs/libfs.c | 4 +++- fs/locks.c | 32 ++++++++++++++++++++------------ fs/nfsd/nfs4state.c | 4 ++-- include/linux/fs.h | 12 ++++++------ 7 files changed, 37 insertions(+), 26 deletions(-) diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index f1997e9da61f..3d92049ae71d 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -464,7 +464,7 @@ prototypes: size_t, unsigned int); ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int); - int (*setlease)(struct file *, long, struct file_lock **); + int (*setlease)(struct file *, long, struct file_lock **, void **); long (*fallocate)(struct file *, int, loff_t, loff_t); }; diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 61d65cc65c54..28ebd49f169f 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -826,7 +826,7 @@ struct file_operations { int (*flock) (struct file *, int, struct file_lock *); ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, size_t, unsigned int); ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int); - int (*setlease)(struct file *, long arg, struct file_lock **); + int (*setlease)(struct file *, long arg, struct file_lock **, void **); long (*fallocate)(struct file *, int mode, loff_t offset, loff_t len); int (*show_fdinfo)(struct seq_file *m, struct file *f); }; diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index ac4f260155c8..85c70d5969ac 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -800,7 +800,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence) return generic_file_llseek(file, offset, whence); } -static int cifs_setlease(struct file *file, long arg, struct file_lock **lease) +static int +cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **priv) { /* * Note that this is called by vfs setlease with i_lock held to @@ -815,7 +816,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease) /* check if file is oplocked */ if (((arg == F_RDLCK) && CIFS_CACHE_READ(CIFS_I(inode))) || ((arg == F_WRLCK) && CIFS_CACHE_WRITE(CIFS_I(inode)))) - return generic_setlease(file, arg, lease); + return generic_setlease(file, arg, lease, priv); else if (tlink_tcon(cfile->tlink)->local_lease && !CIFS_CACHE_READ(CIFS_I(inode))) /* @@ -826,7 +827,7 @@ static int cifs_setlease(struct file *file, long arg, struct file_lock **lease) * knows that the file won't be changed on the server by anyone * else. */ - return generic_setlease(file, arg, lease); + return generic_setlease(file, arg, lease, priv); else return -EAGAIN; } diff --git a/fs/libfs.c b/fs/libfs.c index 29012a303ef8..171d2846f2a3 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1081,12 +1081,14 @@ EXPORT_SYMBOL(alloc_anon_inode); * @filp: file pointer * @arg: type of lease to obtain * @flp: new lease supplied for insertion + * @priv: private data for lm_setup operation * * Generic helper for filesystems that do not wish to allow leases to be set. * All arguments are ignored and it just returns -EINVAL. */ int -simple_nosetlease(struct file *filp, long arg, struct file_lock **flp) +simple_nosetlease(struct file *filp, long arg, struct file_lock **flp, + void **priv) { return -EINVAL; } diff --git a/fs/locks.c b/fs/locks.c index e16c2c61a44f..4fa269b0bdef 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1297,7 +1297,6 @@ int lease_modify(struct file_lock **before, int arg) } return 0; } - EXPORT_SYMBOL(lease_modify); static bool past_time(unsigned long then) @@ -1543,7 +1542,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg) return ret; } -static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp) +static int +generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv) { struct file_lock *fl, **before, **my_before = NULL, *lease; struct dentry *dentry = filp->f_path.dentry; @@ -1630,11 +1630,14 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp smp_mb(); error = check_conflicting_open(dentry, arg); if (error) - locks_unlink_lock(before); + goto out_unlink; out: if (is_deleg) mutex_unlock(&inode->i_mutex); return error; +out_unlink: + locks_unlink_lock(before); + goto out; } static int generic_delete_lease(struct file *filp) @@ -1661,13 +1664,15 @@ static int generic_delete_lease(struct file *filp) * @filp: file pointer * @arg: type of lease to obtain * @flp: input - file_lock to use, output - file_lock inserted + * @priv: private data for lm_setup * * The (input) flp->fl_lmops->lm_break function is required * by break_lease(). * * Called with inode->i_lock held. */ -int generic_setlease(struct file *filp, long arg, struct file_lock **flp) +int generic_setlease(struct file *filp, long arg, struct file_lock **flp, + void **priv) { struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; @@ -1692,19 +1697,20 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp) WARN_ON_ONCE(1); return -ENOLCK; } - return generic_add_lease(filp, arg, flp); + return generic_add_lease(filp, arg, flp, priv); default: return -EINVAL; } } EXPORT_SYMBOL(generic_setlease); -static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease) +static int +__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) { if (filp->f_op->setlease) - return filp->f_op->setlease(filp, arg, lease); + return filp->f_op->setlease(filp, arg, lease, priv); else - return generic_setlease(filp, arg, lease); + return generic_setlease(filp, arg, lease, priv); } /** @@ -1712,6 +1718,7 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease) * @filp: file pointer * @arg: type of lease to obtain * @lease: file_lock to use when adding a lease + * @priv: private info for lm_setup when adding a lease * * Call this to establish a lease on the file. The "lease" argument is not * used for F_UNLCK requests and may be NULL. For commands that set or alter @@ -1720,13 +1727,14 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease) * stack trace). */ -int vfs_setlease(struct file *filp, long arg, struct file_lock **lease) +int +vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) { struct inode *inode = file_inode(filp); int error; spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, lease); + error = __vfs_setlease(filp, arg, lease, priv); spin_unlock(&inode->i_lock); return error; @@ -1751,7 +1759,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) } ret = fl; spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, &ret); + error = __vfs_setlease(filp, arg, &ret, NULL); if (error) goto out_unlock; if (ret == fl) @@ -1789,7 +1797,7 @@ out_unlock: int fcntl_setlease(unsigned int fd, struct file *filp, long arg) { if (arg == F_UNLCK) - return vfs_setlease(filp, F_UNLCK, NULL); + return vfs_setlease(filp, F_UNLCK, NULL, NULL); return do_fcntl_add_lease(fd, filp, arg); } diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 188cd68aefb6..7c803db2a027 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -686,7 +686,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp) spin_unlock(&fp->fi_lock); if (filp) { - vfs_setlease(filp, F_UNLCK, NULL); + vfs_setlease(filp, F_UNLCK, NULL, NULL); fput(filp); } } @@ -3792,7 +3792,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp) } fl->fl_file = filp; ret = fl; - status = vfs_setlease(filp, fl->fl_type, &ret); + status = vfs_setlease(filp, fl->fl_type, &fl, NULL); if (status) { locks_free_lock(fl); goto out_fput; diff --git a/include/linux/fs.h b/include/linux/fs.h index 96528f73dda4..f1870eb67b02 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -982,8 +982,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl); extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl); extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type); extern void lease_get_mtime(struct inode *, struct timespec *time); -extern int generic_setlease(struct file *, long, struct file_lock **); -extern int vfs_setlease(struct file *, long, struct file_lock **); +extern int generic_setlease(struct file *, long, struct file_lock **, void **priv); +extern int vfs_setlease(struct file *, long, struct file_lock **, void **); extern int lease_modify(struct file_lock **, int); #else /* !CONFIG_FILE_LOCKING */ static inline int fcntl_getlk(struct file *file, unsigned int cmd, @@ -1100,13 +1100,13 @@ static inline void lease_get_mtime(struct inode *inode, struct timespec *time) } static inline int generic_setlease(struct file *filp, long arg, - struct file_lock **flp) + struct file_lock **flp, void **priv) { return -EINVAL; } static inline int vfs_setlease(struct file *filp, long arg, - struct file_lock **lease) + struct file_lock **lease, void **priv) { return -EINVAL; } @@ -1494,7 +1494,7 @@ struct file_operations { int (*flock) (struct file *, int, struct file_lock *); ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int); ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int); - int (*setlease)(struct file *, long, struct file_lock **); + int (*setlease)(struct file *, long, struct file_lock **, void **); long (*fallocate)(struct file *file, int mode, loff_t offset, loff_t len); int (*show_fdinfo)(struct seq_file *m, struct file *f); @@ -2599,7 +2599,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping, struct page *page, void *fsdata); extern int always_delete_dentry(const struct dentry *); extern struct inode *alloc_anon_inode(struct super_block *); -extern int simple_nosetlease(struct file *, long, struct file_lock **); +extern int simple_nosetlease(struct file *, long, struct file_lock **, void **); extern const struct dentry_operations simple_dentry_operations; extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags); -- cgit v1.2.3 From 1c7dd2ff430fa14b45c9def54468e3a25ab8342b Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 10:55:47 -0400 Subject: locks: define a lm_setup handler for leases ...and move the fasync setup into it for fcntl lease calls. At the same time, change the semantics of how the file_lock double-pointer is handled. Up until now, on a successful lease return you got a pointer to the lock on the list. This is bad, since that pointer can no longer be relied on as valid once the inode->i_lock has been released. Change the code to instead just zero out the pointer if the lease we passed in ended up being used. Then the callers can just check to see if it's NULL after the call and free it if it isn't. The priv argument has the same semantics. The lm_setup function can zero the pointer out to signal to the caller that it should not be freed after the function returns. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 92 ++++++++++++++++++++++++++++------------------------- fs/nfsd/nfs4state.c | 6 ++-- include/linux/fs.h | 1 + 3 files changed, 51 insertions(+), 48 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 4fa269b0bdef..a237ba632e8d 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -432,9 +432,27 @@ static void lease_break_callback(struct file_lock *fl) kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG); } +static void +lease_setup(struct file_lock *fl, void **priv) +{ + struct file *filp = fl->fl_file; + struct fasync_struct *fa = *priv; + + /* + * fasync_insert_entry() returns the old entry if any. If there was no + * old entry, then it used "priv" and inserted it into the fasync list. + * Clear the pointer to indicate that it shouldn't be freed. + */ + if (!fasync_insert_entry(fa->fa_fd, filp, &fl->fl_fasync, fa)) + *priv = NULL; + + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); +} + static const struct lock_manager_operations lease_manager_ops = { .lm_break = lease_break_callback, .lm_change = lease_modify, + .lm_setup = lease_setup, }; /* @@ -1607,10 +1625,11 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr } if (my_before != NULL) { + lease = *my_before; error = lease->fl_lmops->lm_change(my_before, arg); - if (!error) - *flp = *my_before; - goto out; + if (error) + goto out; + goto out_setup; } error = -EINVAL; @@ -1631,9 +1650,15 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr error = check_conflicting_open(dentry, arg); if (error) goto out_unlink; + +out_setup: + if (lease->fl_lmops->lm_setup) + lease->fl_lmops->lm_setup(lease, priv); out: if (is_deleg) mutex_unlock(&inode->i_mutex); + if (!error && !my_before) + *flp = NULL; return error; out_unlink: locks_unlink_lock(before); @@ -1661,10 +1686,11 @@ static int generic_delete_lease(struct file *filp) /** * generic_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @flp: input - file_lock to use, output - file_lock inserted - * @priv: private data for lm_setup + * @filp: file pointer + * @arg: type of lease to obtain + * @flp: input - file_lock to use, output - file_lock inserted + * @priv: private data for lm_setup (may be NULL if lm_setup + * doesn't require it) * * The (input) flp->fl_lmops->lm_break function is required * by break_lease(). @@ -1704,29 +1730,23 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp, } EXPORT_SYMBOL(generic_setlease); -static int -__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) -{ - if (filp->f_op->setlease) - return filp->f_op->setlease(filp, arg, lease, priv); - else - return generic_setlease(filp, arg, lease, priv); -} - /** * vfs_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @lease: file_lock to use when adding a lease - * @priv: private info for lm_setup when adding a lease + * @filp: file pointer + * @arg: type of lease to obtain + * @lease: file_lock to use when adding a lease + * @priv: private info for lm_setup when adding a lease (may be + * NULL if lm_setup doesn't require it) * * Call this to establish a lease on the file. The "lease" argument is not * used for F_UNLCK requests and may be NULL. For commands that set or alter * an existing lease, the (*lease)->fl_lmops->lm_break operation must be set; * if not, this function will return -ENOLCK (and generate a scary-looking * stack trace). + * + * The "priv" pointer is passed directly to the lm_setup function as-is. It + * may be NULL if the lm_setup operation doesn't require it. */ - int vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) { @@ -1734,17 +1754,18 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) int error; spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, lease, priv); + if (filp->f_op->setlease) + error = filp->f_op->setlease(filp, arg, lease, priv); + else + error = generic_setlease(filp, arg, lease, priv); spin_unlock(&inode->i_lock); - return error; } EXPORT_SYMBOL_GPL(vfs_setlease); static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) { - struct file_lock *fl, *ret; - struct inode *inode = file_inode(filp); + struct file_lock *fl; struct fasync_struct *new; int error; @@ -1757,26 +1778,9 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) locks_free_lock(fl); return -ENOMEM; } - ret = fl; - spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, &ret, NULL); - if (error) - goto out_unlock; - if (ret == fl) - fl = NULL; + new->fa_fd = fd; - /* - * fasync_insert_entry() returns the old entry if any. - * If there was no old entry, then it used 'new' and - * inserted it into the fasync list. Clear new so that - * we don't release it here. - */ - if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new)) - new = NULL; - - __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); -out_unlock: - spin_unlock(&inode->i_lock); + error = vfs_setlease(filp, arg, &fl, (void **)&new); if (fl) locks_free_lock(fl); if (new) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 7c803db2a027..5349528136e2 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3793,12 +3793,10 @@ static int nfs4_setlease(struct nfs4_delegation *dp) fl->fl_file = filp; ret = fl; status = vfs_setlease(filp, fl->fl_type, &fl, NULL); - if (status) { + if (fl) locks_free_lock(fl); + if (status) goto out_fput; - } - if (ret != fl) - locks_free_lock(fl); spin_lock(&state_lock); spin_lock(&fp->fi_lock); /* Did the lease get broken before we took the lock? */ diff --git a/include/linux/fs.h b/include/linux/fs.h index f1870eb67b02..9a6d56154dd5 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -874,6 +874,7 @@ struct lock_manager_operations { int (*lm_grant)(struct file_lock *, int); void (*lm_break)(struct file_lock *); int (*lm_change)(struct file_lock **, int); + void (*lm_setup)(struct file_lock *, void **); }; struct lock_manager { -- cgit v1.2.3 From f82b4b6780afabce9d9a91c84fae17ec3d63b9d7 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 22 Aug 2014 18:50:48 -0400 Subject: locks: move i_lock acquisition into generic_*_lease handlers Now that we have a saner internal API for managing leases, we no longer need to mandate that the inode->i_lock be held over most of the lease code. Push it down into generic_add_lease and generic_delete_lease. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- Documentation/filesystems/Locking | 6 ++++-- Documentation/filesystems/vfs.txt | 5 +++-- fs/locks.c | 21 +++++++++------------ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index 3d92049ae71d..4af288e38f13 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -472,8 +472,6 @@ locking rules: All may block except for ->setlease. No VFS locks held on entry except for ->setlease. -->setlease has the file_list_lock held and must not sleep. - ->llseek() locking has moved from llseek to the individual llseek implementations. If your fs is not using generic_file_llseek, you need to acquire and release the appropriate locks in your ->llseek(). @@ -496,6 +494,10 @@ components. And there are other reasons why the current interface is a mess... ->read on directories probably must go away - we should just enforce -EISDIR in sys_read() and friends. +->setlease operations should call generic_setlease() before or after setting +the lease within the individual filesystem to record the result of the +operation + --------------------------- dquot_operations ------------------------------- prototypes: int (*write_dquot) (struct dquot *); diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 28ebd49f169f..8be1ea3bdd5a 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -895,8 +895,9 @@ otherwise noted. splice_read: called by the VFS to splice data from file to a pipe. This method is used by the splice(2) system call - setlease: called by the VFS to set or release a file lock lease. - setlease has the file_lock_lock held and must not sleep. + setlease: called by the VFS to set or release a file lock lease. setlease + implementations should call generic_setlease to record or remove + the lease in the inode after setting it. fallocate: called by the VFS to preallocate blocks or punch a hole. diff --git a/fs/locks.c b/fs/locks.c index a237ba632e8d..eb463257f867 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1330,6 +1330,8 @@ static void time_out_leases(struct inode *inode) struct file_lock **before; struct file_lock *fl; + lockdep_assert_held(&inode->i_lock); + before = &inode->i_flock; while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) { trace_time_out_leases(inode, fl); @@ -1590,6 +1592,8 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr return -EINVAL; } + spin_lock(&inode->i_lock); + time_out_leases(inode); error = check_conflicting_open(dentry, arg); if (error) goto out; @@ -1655,6 +1659,7 @@ out_setup: if (lease->fl_lmops->lm_setup) lease->fl_lmops->lm_setup(lease, priv); out: + spin_unlock(&inode->i_lock); if (is_deleg) mutex_unlock(&inode->i_mutex); if (!error && !my_before) @@ -1672,6 +1677,7 @@ static int generic_delete_lease(struct file *filp) struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; + spin_lock(&inode->i_lock); for (before = &inode->i_flock; ((fl = *before) != NULL) && IS_LEASE(fl); before = &fl->fl_next) { @@ -1681,6 +1687,7 @@ static int generic_delete_lease(struct file *filp) trace_generic_delete_lease(inode, fl); if (fl) error = fl->fl_lmops->lm_change(before, F_UNLCK); + spin_unlock(&inode->i_lock); return error; } @@ -1694,8 +1701,6 @@ static int generic_delete_lease(struct file *filp) * * The (input) flp->fl_lmops->lm_break function is required * by break_lease(). - * - * Called with inode->i_lock held. */ int generic_setlease(struct file *filp, long arg, struct file_lock **flp, void **priv) @@ -1712,8 +1717,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp, if (error) return error; - time_out_leases(inode); - switch (arg) { case F_UNLCK: return generic_delete_lease(filp); @@ -1750,16 +1753,10 @@ EXPORT_SYMBOL(generic_setlease); int vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) { - struct inode *inode = file_inode(filp); - int error; - - spin_lock(&inode->i_lock); if (filp->f_op->setlease) - error = filp->f_op->setlease(filp, arg, lease, priv); + return filp->f_op->setlease(filp, arg, lease, priv); else - error = generic_setlease(filp, arg, lease, priv); - spin_unlock(&inode->i_lock); - return error; + return generic_setlease(filp, arg, lease, priv); } EXPORT_SYMBOL_GPL(vfs_setlease); -- cgit v1.2.3 From c45198eda2794bb72601c9f96266d8b95db66dd5 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 1 Sep 2014 07:12:07 -0400 Subject: locks: move freeing of leases outside of i_lock There was only one place where we still could free a file_lock while holding the i_lock -- lease_modify. Add a new list_head argument to the lm_change operation, pass in a private list when calling it, and fix those callers to dispose of the list once the lock has been dropped. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- Documentation/filesystems/Locking | 3 +-- fs/locks.c | 34 ++++++++++++++++++++++------------ fs/nfsd/nfs4state.c | 6 +++--- include/linux/fs.h | 7 ++++--- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index 4af288e38f13..94d93b1f8b53 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -469,8 +469,7 @@ prototypes: }; locking rules: - All may block except for ->setlease. - No VFS locks held on entry except for ->setlease. + All may block. ->llseek() locking has moved from llseek to the individual llseek implementations. If your fs is not using generic_file_llseek, you diff --git a/fs/locks.c b/fs/locks.c index eb463257f867..c0f789dfa655 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1292,7 +1292,7 @@ static void lease_clear_pending(struct file_lock *fl, int arg) } /* We already had a lease on this file; just change its type */ -int lease_modify(struct file_lock **before, int arg) +int lease_modify(struct file_lock **before, int arg, struct list_head *dispose) { struct file_lock *fl = *before; int error = assign_type(fl, arg); @@ -1311,7 +1311,7 @@ int lease_modify(struct file_lock **before, int arg) printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync); fl->fl_fasync = NULL; } - locks_delete_lock(before, NULL); + locks_delete_lock(before, dispose); } return 0; } @@ -1325,7 +1325,7 @@ static bool past_time(unsigned long then) return time_after(jiffies, then); } -static void time_out_leases(struct inode *inode) +static void time_out_leases(struct inode *inode, struct list_head *dispose) { struct file_lock **before; struct file_lock *fl; @@ -1336,9 +1336,9 @@ static void time_out_leases(struct inode *inode) while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) { trace_time_out_leases(inode, fl); if (past_time(fl->fl_downgrade_time)) - lease_modify(before, F_RDLCK); + lease_modify(before, F_RDLCK, dispose); if (past_time(fl->fl_break_time)) - lease_modify(before, F_UNLCK); + lease_modify(before, F_UNLCK, dispose); if (fl == *before) /* lease_modify may have freed fl */ before = &fl->fl_next; } @@ -1373,6 +1373,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) int i_have_this_lease = 0; bool lease_conflict = false; int want_write = (mode & O_ACCMODE) != O_RDONLY; + LIST_HEAD(dispose); new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK); if (IS_ERR(new_fl)) @@ -1381,7 +1382,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) spin_lock(&inode->i_lock); - time_out_leases(inode); + time_out_leases(inode, &dispose); flock = inode->i_flock; if ((flock == NULL) || !IS_LEASE(flock)) @@ -1436,6 +1437,7 @@ restart: locks_insert_block(flock, new_fl); trace_break_lease_block(inode, new_fl); spin_unlock(&inode->i_lock); + locks_dispose_list(&dispose); error = wait_event_interruptible_timeout(new_fl->fl_wait, !new_fl->fl_next, break_time); spin_lock(&inode->i_lock); @@ -1443,7 +1445,7 @@ restart: locks_delete_block(new_fl); if (error >= 0) { if (error == 0) - time_out_leases(inode); + time_out_leases(inode, &dispose); /* * Wait for the next conflicting lease that has not been * broken yet @@ -1458,6 +1460,7 @@ restart: out: spin_unlock(&inode->i_lock); + locks_dispose_list(&dispose); locks_free_lock(new_fl); return error; } @@ -1522,9 +1525,10 @@ int fcntl_getlease(struct file *filp) struct file_lock *fl; struct inode *inode = file_inode(filp); int type = F_UNLCK; + LIST_HEAD(dispose); spin_lock(&inode->i_lock); - time_out_leases(file_inode(filp)); + time_out_leases(file_inode(filp), &dispose); for (fl = file_inode(filp)->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) { if (fl->fl_file == filp) { @@ -1533,6 +1537,7 @@ int fcntl_getlease(struct file *filp) } } spin_unlock(&inode->i_lock); + locks_dispose_list(&dispose); return type; } @@ -1570,6 +1575,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr struct inode *inode = dentry->d_inode; bool is_deleg = (*flp)->fl_flags & FL_DELEG; int error; + LIST_HEAD(dispose); lease = *flp; trace_generic_add_lease(inode, lease); @@ -1593,7 +1599,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr } spin_lock(&inode->i_lock); - time_out_leases(inode); + time_out_leases(inode, &dispose); error = check_conflicting_open(dentry, arg); if (error) goto out; @@ -1630,7 +1636,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr if (my_before != NULL) { lease = *my_before; - error = lease->fl_lmops->lm_change(my_before, arg); + error = lease->fl_lmops->lm_change(my_before, arg, &dispose); if (error) goto out; goto out_setup; @@ -1660,6 +1666,7 @@ out_setup: lease->fl_lmops->lm_setup(lease, priv); out: spin_unlock(&inode->i_lock); + locks_dispose_list(&dispose); if (is_deleg) mutex_unlock(&inode->i_mutex); if (!error && !my_before) @@ -1676,8 +1683,10 @@ static int generic_delete_lease(struct file *filp) struct file_lock *fl, **before; struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; + LIST_HEAD(dispose); spin_lock(&inode->i_lock); + time_out_leases(inode, &dispose); for (before = &inode->i_flock; ((fl = *before) != NULL) && IS_LEASE(fl); before = &fl->fl_next) { @@ -1686,8 +1695,9 @@ static int generic_delete_lease(struct file *filp) } trace_generic_delete_lease(inode, fl); if (fl) - error = fl->fl_lmops->lm_change(before, F_UNLCK); + error = fl->fl_lmops->lm_change(before, F_UNLCK, &dispose); spin_unlock(&inode->i_lock); + locks_dispose_list(&dispose); return error; } @@ -2372,7 +2382,7 @@ void locks_remove_file(struct file *filp) while ((fl = *before) != NULL) { if (fl->fl_file == filp) { if (IS_LEASE(fl)) { - lease_modify(before, F_UNLCK); + lease_modify(before, F_UNLCK, &dispose); continue; } diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 5349528136e2..604ab6decd28 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3427,11 +3427,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl) spin_unlock(&fp->fi_lock); } -static -int nfsd_change_deleg_cb(struct file_lock **onlist, int arg) +static int +nfsd_change_deleg_cb(struct file_lock **onlist, int arg, struct list_head *dispose) { if (arg & F_UNLCK) - return lease_modify(onlist, arg); + return lease_modify(onlist, arg, dispose); else return -EAGAIN; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 9a6d56154dd5..f419f718e447 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -873,7 +873,7 @@ struct lock_manager_operations { void (*lm_notify)(struct file_lock *); /* unblock callback */ int (*lm_grant)(struct file_lock *, int); void (*lm_break)(struct file_lock *); - int (*lm_change)(struct file_lock **, int); + int (*lm_change)(struct file_lock **, int, struct list_head *); void (*lm_setup)(struct file_lock *, void **); }; @@ -985,7 +985,7 @@ extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int t extern void lease_get_mtime(struct inode *, struct timespec *time); extern int generic_setlease(struct file *, long, struct file_lock **, void **priv); extern int vfs_setlease(struct file *, long, struct file_lock **, void **); -extern int lease_modify(struct file_lock **, int); +extern int lease_modify(struct file_lock **, int, struct list_head *); #else /* !CONFIG_FILE_LOCKING */ static inline int fcntl_getlk(struct file *file, unsigned int cmd, struct flock __user *user) @@ -1112,7 +1112,8 @@ static inline int vfs_setlease(struct file *filp, long arg, return -EINVAL; } -static inline int lease_modify(struct file_lock **before, int arg) +static inline int lease_modify(struct file_lock **before, int arg, + struct list_head *dispose) { return -EINVAL; } -- cgit v1.2.3 From 843c6b2f4cef384af8e0de6b7ac7191675030e3a Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 1 Sep 2014 14:27:43 -0400 Subject: locks: remove i_have_this_lease check from __break_lease I think that the intent of this code was to ensure that a process won't deadlock if it has one fd open with a lease on it and then breaks that lease by opening another fd. In that case it'll treat the __break_lease call as if it were non-blocking. This seems wrong -- the process could (for instance) be multithreaded and managing different fds via different threads. I also don't see any mention of this limitation in the (somewhat sketchy) documentation. Remove the check and the non-blocking behavior when i_have_this_lease is true. Signed-off-by: Jeff Layton --- fs/locks.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index c0f789dfa655..4e8cf5da2868 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1370,7 +1370,6 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) struct file_lock *new_fl, *flock; struct file_lock *fl; unsigned long break_time; - int i_have_this_lease = 0; bool lease_conflict = false; int want_write = (mode & O_ACCMODE) != O_RDONLY; LIST_HEAD(dispose); @@ -1391,8 +1390,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) { if (leases_conflict(fl, new_fl)) { lease_conflict = true; - if (fl->fl_owner == current->files) - i_have_this_lease = 1; + break; } } if (!lease_conflict) @@ -1422,7 +1420,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) fl->fl_lmops->lm_break(fl); } - if (i_have_this_lease || (mode & O_NONBLOCK)) { + if (mode & O_NONBLOCK) { trace_break_lease_noblock(inode, new_fl); error = -EWOULDBLOCK; goto out; -- cgit v1.2.3 From 03d12ddf845a4eb874ffa558d65a548aee9b715b Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 1 Sep 2014 14:53:41 -0400 Subject: locks: __break_lease cleanup in preparation of allowing direct removal of leases Eliminate an unneeded "flock" variable. We can use "fl" as a loop cursor everywhere. Add a any_leases_conflict helper function as well to consolidate a bit of code. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 4e8cf5da2868..7d627ac0ed87 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1351,6 +1351,20 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker) return locks_conflict(breaker, lease); } +static bool +any_leases_conflict(struct inode *inode, struct file_lock *breaker) +{ + struct file_lock *fl; + + lockdep_assert_held(&inode->i_lock); + + for (fl = inode->i_flock ; fl && IS_LEASE(fl); fl = fl->fl_next) { + if (leases_conflict(fl, breaker)) + return true; + } + return false; +} + /** * __break_lease - revoke all outstanding leases on file * @inode: the inode of the file to return @@ -1367,10 +1381,9 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker) int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) { int error = 0; - struct file_lock *new_fl, *flock; + struct file_lock *new_fl; struct file_lock *fl; unsigned long break_time; - bool lease_conflict = false; int want_write = (mode & O_ACCMODE) != O_RDONLY; LIST_HEAD(dispose); @@ -1383,17 +1396,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) time_out_leases(inode, &dispose); - flock = inode->i_flock; - if ((flock == NULL) || !IS_LEASE(flock)) - goto out; - - for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) { - if (leases_conflict(fl, new_fl)) { - lease_conflict = true; - break; - } - } - if (!lease_conflict) + if (!any_leases_conflict(inode, new_fl)) goto out; break_time = 0; @@ -1403,7 +1406,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) break_time++; /* so that 0 means no break time */ } - for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) { + for (fl = inode->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) { if (!leases_conflict(fl, new_fl)) continue; if (want_write) { @@ -1412,7 +1415,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) fl->fl_flags |= FL_UNLOCK_PENDING; fl->fl_break_time = break_time; } else { - if (lease_breaking(flock)) + if (lease_breaking(inode->i_flock)) continue; fl->fl_flags |= FL_DOWNGRADE_PENDING; fl->fl_downgrade_time = break_time; @@ -1427,12 +1430,12 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) } restart: - break_time = flock->fl_break_time; + break_time = inode->i_flock->fl_break_time; if (break_time != 0) break_time -= jiffies; if (break_time == 0) break_time++; - locks_insert_block(flock, new_fl); + locks_insert_block(inode->i_flock, new_fl); trace_break_lease_block(inode, new_fl); spin_unlock(&inode->i_lock); locks_dispose_list(&dispose); @@ -1442,17 +1445,15 @@ restart: trace_break_lease_unblock(inode, new_fl); locks_delete_block(new_fl); if (error >= 0) { - if (error == 0) - time_out_leases(inode, &dispose); /* * Wait for the next conflicting lease that has not been * broken yet */ - for (flock = inode->i_flock; flock && IS_LEASE(flock); - flock = flock->fl_next) { - if (leases_conflict(new_fl, flock)) - goto restart; - } + if (error == 0) + time_out_leases(inode, &dispose); + if (any_leases_conflict(inode, new_fl)) + goto restart; + error = 0; } -- cgit v1.2.3 From 4d01b7f5e7576858b71cbaa72b541e17a229cb91 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 1 Sep 2014 15:06:54 -0400 Subject: locks: give lm_break a return value Christoph suggests: "Add a return value to lm_break so that the lock manager can tell the core code "you can delete this lease right now". That gets rid of the games with the timeout which require all kinds of race avoidance code in the users." Do that here and have the nfsd lease break routine use it when it detects that there was a race between setting up the lease and it being broken. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 17 +++++++++++++---- fs/nfsd/nfs4state.c | 17 +++++++++-------- include/linux/fs.h | 2 +- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 7d627ac0ed87..aed4a957d232 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -427,9 +427,11 @@ static int flock_to_posix_lock(struct file *filp, struct file_lock *fl, } /* default lease lock manager operations */ -static void lease_break_callback(struct file_lock *fl) +static bool +lease_break_callback(struct file_lock *fl) { kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG); + return false; } static void @@ -1382,7 +1384,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) { int error = 0; struct file_lock *new_fl; - struct file_lock *fl; + struct file_lock *fl, **before; unsigned long break_time; int want_write = (mode & O_ACCMODE) != O_RDONLY; LIST_HEAD(dispose); @@ -1406,7 +1408,9 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) break_time++; /* so that 0 means no break time */ } - for (fl = inode->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) { + for (before = &inode->i_flock; + ((fl = *before) != NULL) && IS_LEASE(fl); + before = &fl->fl_next) { if (!leases_conflict(fl, new_fl)) continue; if (want_write) { @@ -1420,9 +1424,14 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) fl->fl_flags |= FL_DOWNGRADE_PENDING; fl->fl_downgrade_time = break_time; } - fl->fl_lmops->lm_break(fl); + if (fl->fl_lmops->lm_break(fl)) + locks_delete_lock(before, &dispose); } + fl = inode->i_flock; + if (!fl || !IS_LEASE(fl)) + goto out; + if (mode & O_NONBLOCK) { trace_break_lease_noblock(inode, new_fl); error = -EWOULDBLOCK; diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 604ab6decd28..d1b851548b7a 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3391,18 +3391,20 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp) } /* Called from break_lease() with i_lock held. */ -static void nfsd_break_deleg_cb(struct file_lock *fl) +static bool +nfsd_break_deleg_cb(struct file_lock *fl) { + bool ret = false; struct nfs4_file *fp = (struct nfs4_file *)fl->fl_owner; struct nfs4_delegation *dp; if (!fp) { WARN(1, "(%p)->fl_owner NULL\n", fl); - return; + return ret; } if (fp->fi_had_conflict) { WARN(1, "duplicate break on %p\n", fp); - return; + return ret; } /* * We don't want the locks code to timeout the lease for us; @@ -3414,17 +3416,16 @@ static void nfsd_break_deleg_cb(struct file_lock *fl) spin_lock(&fp->fi_lock); fp->fi_had_conflict = true; /* - * If there are no delegations on the list, then we can't count on this - * lease ever being cleaned up. Set the fl_break_time to jiffies so that - * time_out_leases will do it ASAP. The fact that fi_had_conflict is now - * true should keep any new delegations from being hashed. + * If there are no delegations on the list, then return true + * so that the lease code will go ahead and delete it. */ if (list_empty(&fp->fi_delegations)) - fl->fl_break_time = jiffies; + ret = true; else list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) nfsd_break_one_deleg(dp); spin_unlock(&fp->fi_lock); + return ret; } static int diff --git a/include/linux/fs.h b/include/linux/fs.h index f419f718e447..ed4e1897099c 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -872,7 +872,7 @@ struct lock_manager_operations { void (*lm_put_owner)(struct file_lock *); void (*lm_notify)(struct file_lock *); /* unblock callback */ int (*lm_grant)(struct file_lock *, int); - void (*lm_break)(struct file_lock *); + bool (*lm_break)(struct file_lock *); int (*lm_change)(struct file_lock **, int, struct list_head *); void (*lm_setup)(struct file_lock *, void **); }; -- cgit v1.2.3 From 7ca76311fe6c397e9f332e5e6c79e3310d5ee98a Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 1 Sep 2014 19:04:48 -0400 Subject: locks: set fl_owner for leases to filp instead of current->files Like flock locks, leases are owned by the file description. Now that the i_have_this_lease check in __break_lease is gone, we don't actually use the fl_owner for leases for anything. So, it's now safe to set this more appropriately to the same value as the fl_file. While we're at it, fix up the comments over the fl_owner_t definition since they're rather out of date. Signed-off-by: Jeff Layton --- fs/locks.c | 2 +- include/linux/fs.h | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index aed4a957d232..314135ad820b 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -465,7 +465,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl) if (assign_type(fl, type) != 0) return -EINVAL; - fl->fl_owner = current->files; + fl->fl_owner = filp; fl->fl_pid = current->tgid; fl->fl_file = filp; diff --git a/include/linux/fs.h b/include/linux/fs.h index ed4e1897099c..bb9484ae1eef 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -851,13 +851,7 @@ static inline struct file *get_file(struct file *f) */ #define FILE_LOCK_DEFERRED 1 -/* - * The POSIX file lock owner is determined by - * the "struct files_struct" in the thread group - * (or NULL for no owner - BSD locks). - * - * Lockd stuffs a "host" pointer into this. - */ +/* legacy typedef, should eventually be removed */ typedef void *fl_owner_t; struct file_lock_operations { -- cgit v1.2.3 From 6e129d00689c4d75253d1d428e82047b0aef5891 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Thu, 4 Sep 2014 10:25:06 -0400 Subject: locks: flock_make_lock should return a struct file_lock (or PTR_ERR) Eliminate the need for a return pointer. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- fs/locks.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 314135ad820b..735b8d3fa78c 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -326,17 +326,18 @@ static inline int flock_translate_cmd(int cmd) { } /* Fill in a file_lock structure with an appropriate FLOCK lock. */ -static int flock_make_lock(struct file *filp, struct file_lock **lock, - unsigned int cmd) +static struct file_lock * +flock_make_lock(struct file *filp, unsigned int cmd) { struct file_lock *fl; int type = flock_translate_cmd(cmd); + if (type < 0) - return type; + return ERR_PTR(type); fl = locks_alloc_lock(); if (fl == NULL) - return -ENOMEM; + return ERR_PTR(-ENOMEM); fl->fl_file = filp; fl->fl_owner = filp; @@ -345,8 +346,7 @@ static int flock_make_lock(struct file *filp, struct file_lock **lock, fl->fl_type = type; fl->fl_end = OFFSET_MAX; - *lock = fl; - return 0; + return fl; } static int assign_type(struct file_lock *fl, long type) @@ -1885,9 +1885,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) !(f.file->f_mode & (FMODE_READ|FMODE_WRITE))) goto out_putf; - error = flock_make_lock(f.file, &lock, cmd); - if (error) + lock = flock_make_lock(f.file, cmd); + if (IS_ERR(lock)) { + error = PTR_ERR(lock); goto out_putf; + } + if (can_sleep) lock->fl_flags |= FL_SLEEP; -- cgit v1.2.3 From 1b2b32dcdb3df28dd103033c73cac2417fa05845 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Wed, 24 Sep 2014 08:38:44 -0400 Subject: locks: fix fcntl_setlease/getlease return when !CONFIG_FILE_LOCKING Currently they both just return 0. Fix them to return more appropriate values instead. For better or worse, most places in the kernel return -EINVAL when leases aren't available. -ENOLCK would probably have been better, but let's follow suit here in the case of F_SETLEASE. In the F_GETLEASE case, just return F_UNLCK since we know that no lease will have been set. Signed-off-by: Jeff Layton Reviewed-by: Christoph Hellwig --- include/linux/fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/fs.h b/include/linux/fs.h index bb9484ae1eef..2023306c620e 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1008,12 +1008,12 @@ static inline int fcntl_setlk64(unsigned int fd, struct file *file, #endif static inline int fcntl_setlease(unsigned int fd, struct file *filp, long arg) { - return 0; + return -EINVAL; } static inline int fcntl_getlease(struct file *filp) { - return 0; + return F_UNLCK; } static inline void locks_init_lock(struct file_lock *fl) -- cgit v1.2.3