Commit a49a2a1b authored by NeilBrown's avatar NeilBrown Committed by Chuck Lever
Browse files

lockd: fix vfs_test_lock() calls 



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: default avatarOlga Kornievskaia <okorniev@redhat.com>
Closes: https://lore.kernel.org/all/20251021130506.45065-1-okorniev@redhat.com


Signed-off-by: default avatarNeilBrown <neil@brown.name>
Fixes: 20fa1902 ("nfs: add export operations")
Cc: stable@vger.kernel.org
Reviewed-by: default avatarJeff Layton <jlayton@kernel.org>
Signed-off-by: default avatarChuck Lever <chuck.lever@oracle.com>
parent 913f7cf7

fs/lockd/svc4proc.c

+1 −3
Original line number Diff line number Diff line
@@ -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;

fs/lockd/svclock.c

+12 −9
Original line number Diff line number Diff line
@@ -633,7 +633,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)
@@ -643,22 +649,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:

fs/lockd/svcproc.c

+1 −4
Original line number Diff line number Diff line
@@ -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;

fs/locks.c

+10 −2
Original line number Diff line number Diff line
@@ -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);