Re: [PATCH] lockd: fix TEST handling when not all permissions are available.

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2026-03-24 at 21:13 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The F_GETLK fcntl can work with either read access or write access or
> both.  It can query F_RDLCK and F_WRLCK locks in either case.
> 
> However lockd currently treats F_GETLK similar to F_SETLK in that read
> access is required to query an F_RDLCK lock and write access is required
> to query a F_WRLCK lock.
> 
> This is wrong and can cause problem - e.g.  when qemu accesses a
> read-only (e.g. iso) filesystem image over NFS (though why it queries
> if it can get a write lock - I don't know.  But it does, and this works
> with local filesystems).
> 
> So we need TEST requests to be handled differently.  To do this:
> 
> - change nlm_do_fopen() to accept O_RDWR as a mode and in that case
>   succeed if either a O_RDONLY or O_WRONLY file can be opened.
> - change nlm_lookup_file() to accept a mode argument from caller,
>   instead of deducing base on lock time, and pass that on to nlm_do_fopen()
> - change nlm4svc_retrieve_args() and nlmsvc_retrieve_args() to detect
>   TEST requests and pass O_RDWR as a mode to nlm_lookup_file, passing
>   the same mode as before for other requests.  Also set
>    lock->fl.c.flc_file to whichever file is available for TEST requests.
> - change nlmsvc_testlock() to also not calculate the mode, but to use
>   whenever was stored in lock->fl.c.flc_file.
> 
> Reported-by: Tj <tj.iam.tj@proton.me>
> Link:  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1128861
> Fixes: 7f024fcd5c97 ("Keep read and write fds with each nlm_file")
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/lockd/svc4proc.c         | 13 ++++++++++---
>  fs/lockd/svclock.c          |  4 +---
>  fs/lockd/svcproc.c          | 15 ++++++++++++---
>  fs/lockd/svcsubs.c          | 26 +++++++++++++++++---------
>  include/linux/lockd/lockd.h |  2 +-
>  5 files changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 4b6f18d97734..75e020a8bfd0 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -26,6 +26,8 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	struct nlm_host		*host = NULL;
>  	struct nlm_file		*file = NULL;
>  	struct nlm_lock		*lock = &argp->lock;
> +	bool			is_test = (rqstp->rq_proc == NLMPROC_TEST ||
> +					   rqstp->rq_proc == NLMPROC_TEST_MSG);
>  	__be32			error = 0;
>  
>  	/* nfsd callbacks must have been installed for this procedure */
> @@ -46,15 +48,20 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	if (filp != NULL) {
>  		int mode = lock_to_openmode(&lock->fl);
>  
> +		if (is_test)
> +			mode = O_RDWR;
> +
>  		lock->fl.c.flc_flags = FL_POSIX;
>  
> -		error = nlm_lookup_file(rqstp, &file, lock);
> +		error = nlm_lookup_file(rqstp, &file, lock, mode);
>  		if (error)
>  			goto no_locks;
>  		*filp = file;
> -
>  		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.c.flc_file = file->f_file[mode];
> +		if (is_test)
> +			lock->fl.c.flc_file = nlmsvc_file_file(file);
> +		else
> +			lock->fl.c.flc_file = file->f_file[mode];
>  		lock->fl.c.flc_pid = current->tgid;
>  		lock->fl.fl_start = (loff_t)lock->lock_start;
>  		lock->fl.fl_end = lock->lock_len ?
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 255a847ca0b6..adfd8c072898 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -614,7 +614,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		struct nlm_lock *conflock)
>  {
>  	int			error;
> -	int			mode;
>  	__be32			ret;
>  
>  	dprintk("lockd: nlmsvc_testlock(%s/%ld, ty=%d, %Ld-%Ld)\n",
> @@ -632,14 +631,13 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		goto out;
>  	}
>  
> -	mode = lock_to_openmode(&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);
> +	error = vfs_test_lock(lock->fl.c.flc_file, &conflock->fl);
>  	if (error) {
>  		ret = nlm_lck_denied_nolocks;
>  		goto out;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 5817ef272332..d98e8d684376 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -55,6 +55,8 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	struct nlm_host		*host = NULL;
>  	struct nlm_file		*file = NULL;
>  	struct nlm_lock		*lock = &argp->lock;
> +	bool			is_test = (rqstp->rq_proc == NLMPROC_TEST ||
> +					   rqstp->rq_proc == NLMPROC_TEST_MSG);
>  	int			mode;
>  	__be32			error = 0;
>  
> @@ -70,15 +72,22 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>  
>  	/* Obtain file pointer. Not used by FREE_ALL call. */
>  	if (filp != NULL) {
> -		error = cast_status(nlm_lookup_file(rqstp, &file, lock));
> +		mode = lock_to_openmode(&lock->fl);
> +
> +		if (is_test)
> +			mode = O_RDWR;
> +
> +		error = cast_status(nlm_lookup_file(rqstp, &file, lock, mode));
>  		if (error != 0)
>  			goto no_locks;
>  		*filp = file;
>  
>  		/* Set up the missing parts of the file_lock structure */
> -		mode = lock_to_openmode(&lock->fl);
>  		lock->fl.c.flc_flags = FL_POSIX;
> -		lock->fl.c.flc_file  = file->f_file[mode];
> +		if (is_test)
> +			lock->fl.c.flc_file = nlmsvc_file_file(file);
> +		else
> +			lock->fl.c.flc_file = file->f_file[mode];
>  		lock->fl.c.flc_pid = current->tgid;
>  		lock->fl.fl_lmops = &nlmsvc_lock_operations;
>  		nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid);
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index dd0214dcb695..b92eb032849f 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -82,18 +82,28 @@ int lock_to_openmode(struct file_lock *lock)
>   *
>   * We have to make sure we have the right credential to open
>   * the file.
> + *
> + * mode can be O_RDONLY(0), O_WRONLY(1) or O_RDWR(2) meaning either

Meaning either... ?

>   */
>  static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
>  			   struct nlm_file *file, int mode)
>  {
> -	struct file **fp = &file->f_file[mode];
> +	struct file **fp;
>  	__be32	nfserr;
> +	int m;
>  
> -	if (*fp)
> -		return 0;
> -	nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, mode);
> -	if (nfserr)
> -		dprintk("lockd: open failed (error %d)\n", nfserr);
> +	for (m = O_RDONLY ; m <= O_WRONLY ; m++) {
> +		if (mode != O_RDWR && mode != m)
> +			continue;
> +
> +		fp = &file->f_file[m];
> +		if (*fp)
> +			return 0;
> +		nfserr = nlmsvc_ops->fopen(rqstp, &file->f_handle, fp, m);
> +		if (!nfserr)
> +			return 0;
> +	}
> +	dprintk("lockd: open failed (error %d)\n", nfserr);
>  	return nfserr;
>  }
>  
> @@ -103,17 +113,15 @@ static __be32 nlm_do_fopen(struct svc_rqst *rqstp,
>   */
>  __be32
>  nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
> -					struct nlm_lock *lock)
> +		struct nlm_lock *lock, int mode)
>  {
>  	struct nlm_file	*file;
>  	unsigned int	hash;
>  	__be32		nfserr;
> -	int		mode;
>  
>  	nlm_debug_print_fh("nlm_lookup_file", &lock->fh);
>  
>  	hash = file_hash(&lock->fh);
> -	mode = lock_to_openmode(&lock->fl);
>  
>  	/* Lock file table */
>  	mutex_lock(&nlm_file_mutex);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 330e38776bb2..fe5cdd4d66f4 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -294,7 +294,7 @@ void		  nlmsvc_locks_init_private(struct file_lock *, struct nlm_host *, pid_t);
>   * File handling for the server personality
>   */
>  __be32		  nlm_lookup_file(struct svc_rqst *, struct nlm_file **,
> -					struct nlm_lock *);
> +				  struct nlm_lock *, int);
>  void		  nlm_release_file(struct nlm_file *);
>  void		  nlmsvc_put_lockowner(struct nlm_lockowner *);
>  void		  nlmsvc_release_lockowner(struct nlm_lock *);

Patch looks good though.

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