lockd: fix vfs_test_lock() calls

[ 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 c.flc_* field accesses to direct fl_* fields ]
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index e318d55e4c0ef92f67f02b67d52fbc5ff7d03a47..277d665937ae4e1d7b06828c1181935e6d737e21 100644 (file)
--- 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 4e30f3c50970127ea0a5197ed03b310f2409e3eb..035f885809dd2adcc41e86dd14692c3ce97fb136 100644 (file)
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -604,7 +604,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)
@@ -614,22 +620,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 2a615032f5d0f9493d62ef130c078c670973f063..fdbe93accb8627fe69635b2510f3a060528bb4a6 100644 (file)
--- 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 250559af8064b725b7a7e470c0a11794429781fa..d923a9edff6695da1a6f6441ec5b9659d5d106b7 100644 (file)
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2229,13 +2229,22 @@ 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);
        posix_test_lock(filp, fl);