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

[NeilBrown <neilb@ownmail.net>]

On Tue, 24 Mar 2026, Jeff Layton wrote:
> 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... ?

If O_RDWR is given then either a O_RDONLY file or a O_WRONLY file is provided.

I see now that isn't obvious from the text...



> 
> >   */
> >  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>

Thanks,
NeilBrown