[PATCH v2 0/2] lockd/locks: address issues with vfs_test_lock()

[PATCH v2 0/2] lockd/locks: address issues with vfs_test_lock()

[NeilBrown]

v2 contains fixes to problem found by Chuck (thanks) and also improves
the documentation for vfs_test_lock().

Documentation says that it returns 0 or -ERRNO, but one caller check for
a FILE_LOCK_DEFERRED which is neither of those.  So I address that in
the second patch.

Thanks,
NeilBrown


 [PATCH v2 1/2] lockd: fix vfs_test_lock() calls
 [PATCH v2 2/2] locks: ensure vfs_test_lock() never returns

[PATCH v2 1/2] lockd: fix vfs_test_lock() calls

[NeilBrown]

From: NeilBrown <neil@brown.name>

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>

---
changes since v1:
- use conflock more consistently in nlmsvc_testlock()
---
 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 109e5caae8c7..4b6f18d97734 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -97,7 +97,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");
@@ -107,7 +106,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.c.flc_owner;
 	/* Now check for conflicting locks */
 	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock,
 				       &resp->lock);
@@ -116,7 +114,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 a31dc9588eb8..d66e82851599 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -627,7 +627,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 flc_file */
+	conflock->fl.c.flc_file = lock->fl.c.flc_file;
+	conflock->fl.fl_start = lock->fl.fl_start;
+	conflock->fl.fl_end = lock->fl.fl_end;
+	conflock->fl.c.flc_owner = lock->fl.c.flc_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)
@@ -637,22 +643,19 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		goto out;
 	}
 
-	if (lock->fl.c.flc_type == F_UNLCK) {
+	if (conflock->fl.c.flc_type == F_UNLCK) {
 		ret = nlm_granted;
 		goto out;
 	}
 
 	dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
-		lock->fl.c.flc_type, (long long)lock->fl.fl_start,
-		(long long)lock->fl.fl_end);
+		conflock->fl.c.flc_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.c.flc_pid;
-	conflock->fl.c.flc_type = lock->fl.c.flc_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.c.flc_pid;
+	locks_release_private(&conflock->fl);
 
 	ret = nlm_lck_denied;
 out:
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index f53d5177f267..5817ef272332 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.c.flc_owner;
-
 	/* Now check for conflicting locks */
 	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host,
 						   &argp->lock, &resp->lock));
@@ -138,7 +135,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 04a3f0e20724..bf5e0d05a026 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2185,13 +2185,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->c.flc_file);
 	if (filp->f_op->lock)
 		return filp->f_op->lock(filp, F_GETLK, fl);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 2/2] locks: ensure vfs_test_lock() never returns FILE_LOCK_DEFERRED

[NeilBrown]

From: NeilBrown <neil@brown.name>

FILE_LOCK_DEFERRED can be returned when creating or removing a lock, but
not when testing for a lock.  This support was explicitly removed in
Commit 09802fd2a8ca ("lockd: rip out deferred lock handling from testlock codepath")

However the test in nlmsvc_testlock() suggests that it *can* be returned,
only nlm cannot handle it.

To aid clarity, remove the test and instead put a similar test and
warning in vfs_test_lock().  If the impossible happens, convert
FILE_LOCK_DEFERRED to -EIO.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/lockd/svclock.c |  4 ----
 fs/locks.c         | 17 ++++++++++++++---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index d66e82851599..dfd1a12b7887 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -635,10 +635,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	conflock->fl.c.flc_owner = lock->fl.c.flc_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)
-			WARN_ON_ONCE(1);
-
 		ret = nlm_lck_denied_nolocks;
 		goto out;
 	}
diff --git a/fs/locks.c b/fs/locks.c
index bf5e0d05a026..b5a3bb8790f3 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2199,12 +2199,23 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
  */
 int vfs_test_lock(struct file *filp, struct file_lock *fl)
 {
+	int error = 0;
+
 	WARN_ON_ONCE(fl->fl_ops || fl->fl_lmops);
 	WARN_ON_ONCE(filp != fl->c.flc_file);
 	if (filp->f_op->lock)
-		return filp->f_op->lock(filp, F_GETLK, fl);
-	posix_test_lock(filp, fl);
-	return 0;
+		error = filp->f_op->lock(filp, F_GETLK, fl);
+	else
+		posix_test_lock(filp, fl);
+
+	/*
+	 * We don't expect FILE_LOCK_DEFERRED and callers cannot
+	 * handle it.
+	 */
+	if (WARN_ON_ONCE(error == FILE_LOCK_DEFERRED))
+		error = -EIO;
+
+	return error;
 }
 EXPORT_SYMBOL_GPL(vfs_test_lock);
 
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 1/2] lockd: fix vfs_test_lock() calls

[Chuck Lever]

On Fri, Nov 21, 2025, at 8:00 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> 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>

At a guess:

Fixes: 5ea0d75037b9 ("lockd: handle test_lock deferrals")  ??

I suspect this also needs a Cc: stable.


> ---
> changes since v1:
> - use conflock more consistently in nlmsvc_testlock()
> ---
>  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 109e5caae8c7..4b6f18d97734 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -97,7 +97,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");
> @@ -107,7 +106,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.c.flc_owner;
>  	/* Now check for conflicting locks */
>  	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock,
>  				       &resp->lock);
> @@ -116,7 +114,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 a31dc9588eb8..d66e82851599 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -627,7 +627,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 flc_file 
> */
> +	conflock->fl.c.flc_file = lock->fl.c.flc_file;
> +	conflock->fl.fl_start = lock->fl.fl_start;
> +	conflock->fl.fl_end = lock->fl.fl_end;
> +	conflock->fl.c.flc_owner = lock->fl.c.flc_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)
> @@ -637,22 +643,19 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct 
> nlm_file *file,
>  		goto out;
>  	}
> 
> -	if (lock->fl.c.flc_type == F_UNLCK) {
> +	if (conflock->fl.c.flc_type == F_UNLCK) {
>  		ret = nlm_granted;
>  		goto out;
>  	}
> 
>  	dprintk("lockd: conflicting lock(ty=%d, %Ld-%Ld)\n",
> -		lock->fl.c.flc_type, (long long)lock->fl.fl_start,
> -		(long long)lock->fl.fl_end);
> +		conflock->fl.c.flc_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.c.flc_pid;
> -	conflock->fl.c.flc_type = lock->fl.c.flc_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.c.flc_pid;
> +	locks_release_private(&conflock->fl);
> 
>  	ret = nlm_lck_denied;
>  out:
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index f53d5177f267..5817ef272332 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.c.flc_owner;
> -
>  	/* Now check for conflicting locks */
>  	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host,
>  						   &argp->lock, &resp->lock));
> @@ -138,7 +135,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 04a3f0e20724..bf5e0d05a026 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2185,13 +2185,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->c.flc_file);
>  	if (filp->f_op->lock)
>  		return filp->f_op->lock(filp, F_GETLK, fl);
> -- 
> 2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 0/2] lockd/locks: address issues with vfs_test_lock()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Sat, 22 Nov 2025 12:00:35 +1100, NeilBrown wrote:
> v2 contains fixes to problem found by Chuck (thanks) and also improves
> the documentation for vfs_test_lock().
> 
> Documentation says that it returns 0 or -ERRNO, but one caller check for
> a FILE_LOCK_DEFERRED which is neither of those.  So I address that in
> the second patch.
> 
> [...]

Applied to nfsd-testing, thanks!

[1/2] lockd: fix vfs_test_lock() calls
      commit: bc99611cfa1b1cf67a17718feca65e43eeed3118
[2/2] locks: ensure vfs_test_lock() never returns FILE_LOCK_DEFERRED
      commit: 569ad3b4cf6ae8d71bf73ad13079e7b88d26b339

--
Chuck Lever

Re: [PATCH v2 1/2] lockd: fix vfs_test_lock() calls

[NeilBrown]

On Sun, 23 Nov 2025, Chuck Lever wrote:
> On Fri, Nov 21, 2025, at 8:00 PM, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > 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>
> 
> At a guess:
> 
> Fixes: 5ea0d75037b9 ("lockd: handle test_lock deferrals")  ??

I think the problem is, in practice, more recent.
I think there is only a problem if the filesystem that lockd is
accessing sets fl_lmops on locks.

So maybe

Fixes: 20fa19027286 ("nfs: add export operations")

> 
> I suspect this also needs a Cc: stable.

I think that would be appropriate - yes.

NeilBrown

Re: [PATCH v2 1/2] lockd: fix vfs_test_lock() calls

[Jeff Layton]

On Sun, 2025-11-23 at 07:48 +1100, NeilBrown wrote:
> On Sun, 23 Nov 2025, Chuck Lever wrote:
> > On Fri, Nov 21, 2025, at 8:00 PM, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > 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>
> > 
> > At a guess:
> > 
> > Fixes: 5ea0d75037b9 ("lockd: handle test_lock deferrals")  ??
> 
> I think the problem is, in practice, more recent.
> I think there is only a problem if the filesystem that lockd is
> accessing sets fl_lmops on locks.
> 

...and specifically if the filesystem sets lm_get_owner/lm_put_owner.
That's only NFS/NLM, currently.

> So maybe
> 
> Fixes: 20fa19027286 ("nfs: add export operations")
> 

That's likely the place where it became a real problem, yes.

> > 
> > I suspect this also needs a Cc: stable.
> 
> I think that would be appropriate - yes.
> 
> NeilBrown

Ditto.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/2] lockd/locks: address issues with vfs_test_lock()

[Jeff Layton]

On Sat, 2025-11-22 at 12:00 +1100, NeilBrown wrote:
> v2 contains fixes to problem found by Chuck (thanks) and also improves
> the documentation for vfs_test_lock().
> 
> Documentation says that it returns 0 or -ERRNO, but one caller check for
> a FILE_LOCK_DEFERRED which is neither of those.  So I address that in
> the second patch.
> 
> Thanks,
> NeilBrown
> 
> 
>  [PATCH v2 1/2] lockd: fix vfs_test_lock() calls
>  [PATCH v2 2/2] locks: ensure vfs_test_lock() never returns

Nice cleanup.

Reviewed-by: Jeff Layton <jlayton@kernel.org>