On Tue, 24 Mar 2026, Jeff Layton wrote:
> On Tue, 2026-03-24 at 21:13 +1100, NeilBrown wrote:
> > From: NeilBrown <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks,
NeilBrown
