From: NeilBrown neil@brown.name
[ Upstream commit a49a2a1baa0c553c3548a1c414b6a3c005a8deba ]
Usage of vfs_test_lock() is somewhat confused. Documentation suggests it is given a "lock" but this is not the case. It is given a struct file_lock which contains some details of the sort of lock it should be looking for.
In particular passing a "file_lock" containing fl_lmops or fl_ops is meaningless and possibly confusing.
This is particularly problematic in lockd. nlmsvc_testlock() receives an initialised "file_lock" from xdr-decode, including manager ops and an owner. It then mistakenly passes this to vfs_test_lock() which might replace the owner and the ops. This can lead to confusion when freeing the lock.
The primary role of the 'struct file_lock' passed to vfs_test_lock() is to report a conflicting lock that was found, so it makes more sense for nlmsvc_testlock() to pass "conflock", which it uses for returning the conflicting lock.
With this change, freeing of the lock is not confused and code in __nlm4svc_proc_test() and __nlmsvc_proc_test() can be simplified.
Documentation for vfs_test_lock() is improved to reflect its real purpose, and a WARN_ON_ONCE() is added to avoid a similar problem in the future.
Reported-by: Olga Kornievskaia okorniev@redhat.com Closes: https://lore.kernel.org/all/20251021130506.45065-1-okorniev@redhat.com Signed-off-by: NeilBrown neil@brown.name Fixes: 20fa19027286 ("nfs: add export operations") Cc: stable@vger.kernel.org Reviewed-by: Jeff Layton jlayton@kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com [ adapted fl->c.flc_* field references to flat fl->fl_* naming convention ] Signed-off-by: Sasha Levin sashal@kernel.org --- fs/lockd/svc4proc.c | 4 +--- fs/lockd/svclock.c | 21 ++++++++++++--------- fs/lockd/svcproc.c | 5 +---- fs/locks.c | 12 ++++++++++-- 4 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c index b72023a6b4c1..28ba7b1460aa 100644 --- a/fs/lockd/svc4proc.c +++ b/fs/lockd/svc4proc.c @@ -96,7 +96,6 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) struct nlm_args *argp = rqstp->rq_argp; struct nlm_host *host; struct nlm_file *file; - struct nlm_lockowner *test_owner; __be32 rc = rpc_success;
dprintk("lockd: TEST4 called\n"); @@ -106,7 +105,6 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) if ((resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file))) return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
- test_owner = argp->lock.fl.fl_owner; /* Now check for conflicting locks */ resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie); if (resp->status == nlm_drop_reply) @@ -114,7 +112,7 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) else dprintk("lockd: TEST4 status %d\n", ntohl(resp->status));
- nlmsvc_put_lockowner(test_owner); + nlmsvc_release_lockowner(&argp->lock); nlmsvc_release_host(host); nlm_release_file(file); return rc; diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index 43aeba9de55c..ef6d8061460d 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -615,7 +615,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, }
mode = lock_to_openmode(&lock->fl); - error = vfs_test_lock(file->f_file[mode], &lock->fl); + locks_init_lock(&conflock->fl); + /* vfs_test_lock only uses start, end, and owner, but tests fl_file */ + conflock->fl.fl_file = lock->fl.fl_file; + conflock->fl.fl_start = lock->fl.fl_start; + conflock->fl.fl_end = lock->fl.fl_end; + conflock->fl.fl_owner = lock->fl.fl_owner; + error = vfs_test_lock(file->f_file[mode], &conflock->fl); if (error) { /* We can't currently deal with deferred test requests */ if (error == FILE_LOCK_DEFERRED) @@ -625,22 +631,19 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file, goto out; }
- if (lock->fl.fl_type == F_UNLCK) { + if (conflock->fl.fl_type == F_UNLCK) { ret = nlm_granted; goto out; }
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); + conflock->fl.fl_type, (long long)conflock->fl.fl_start, + (long long)conflock->fl.fl_end); conflock->caller = "somehost"; /* FIXME */ conflock->len = strlen(conflock->caller); conflock->oh.len = 0; /* don't return OH info */ - conflock->svid = lock->fl.fl_pid; - 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); + conflock->svid = conflock->fl.fl_pid; + locks_release_private(&conflock->fl);
ret = nlm_lck_denied; out: diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c index 32784f508c81..1a4459763644 100644 --- a/fs/lockd/svcproc.c +++ b/fs/lockd/svcproc.c @@ -117,7 +117,6 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) struct nlm_args *argp = rqstp->rq_argp; struct nlm_host *host; struct nlm_file *file; - struct nlm_lockowner *test_owner; __be32 rc = rpc_success;
dprintk("lockd: TEST called\n"); @@ -127,8 +126,6 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) if ((resp->status = nlmsvc_retrieve_args(rqstp, argp, &host, &file))) return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
- test_owner = argp->lock.fl.fl_owner; - /* Now check for conflicting locks */ resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie)); if (resp->status == nlm_drop_reply) @@ -137,7 +134,7 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp) dprintk("lockd: TEST status %d vers %d\n", ntohl(resp->status), rqstp->rq_vers);
- nlmsvc_put_lockowner(test_owner); + nlmsvc_release_lockowner(&argp->lock); nlmsvc_release_host(host); nlm_release_file(file); return rc; diff --git a/fs/locks.c b/fs/locks.c index ccfa441e487e..19e9fcd1d4ca 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -2124,13 +2124,21 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) /** * vfs_test_lock - test file byte range lock * @filp: The file to test lock for - * @fl: The lock to test; also used to hold result + * @fl: The byte-range in the file to test; also used to hold result * + * On entry, @fl does not contain a lock, but identifies a range (fl_start, fl_end) + * in the file (c.flc_file), and an owner (c.flc_owner) for whom existing locks + * should be ignored. c.flc_type and c.flc_flags are ignored. + * Both fl_lmops and fl_ops in @fl must be NULL. * Returns -ERRNO on failure. Indicates presence of conflicting lock by - * setting conf->fl_type to something other than F_UNLCK. + * setting fl->fl_type to something other than F_UNLCK. + * + * If vfs_test_lock() does find a lock and return it, the caller must + * use locks_free_lock() or locks_release_private() on the returned lock. */ int vfs_test_lock(struct file *filp, struct file_lock *fl) { + WARN_ON_ONCE(fl->fl_ops || fl->fl_lmops); WARN_ON_ONCE(filp != fl->fl_file); if (filp->f_op->lock) return filp->f_op->lock(filp, F_GETLK, fl);