[PATCH 0/4] filelock: WARN when @filp and fl_file don't match

[PATCH 0/4] filelock: WARN when @filp and fl_file don't match

[Jeff Layton]

Eventually, I'd like to reduce the redundant arguments to the locking
APIs, but to get there we need to ensure the callers all set their file
locks sanely.

Adding the WARN_ON_ONCEs helped to find a couple of warts in lockd's
file handling. The first 3 patches fix those.

Chuck, would you be willing to take these in for v6.2? I'd like to see
the WARN_ONs added for that so we can try to clean up the file locking
APIs for v6.3.

Thanks,

Jeff Layton (4):
  lockd: set missing fl_flags field when retrieving args
  lockd: ensure we use the correct file description when unlocking
  lockd: fix file selection in nlmsvc_cancel_blocked
  filelock: WARN_ON_ONCE when ->fl_file and filp don't match

 fs/lockd/svc4proc.c |  1 +
 fs/lockd/svclock.c  | 17 ++++++++++-------
 fs/lockd/svcproc.c  |  1 +
 fs/locks.c          |  3 +++
 4 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.38.1

[PATCH 1/4] lockd: set missing fl_flags field when retrieving args

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/lockd/svc4proc.c | 1 +
 fs/lockd/svcproc.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 284b019cb652..b72023a6b4c1 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -52,6 +52,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
+		lock->fl.fl_flags = FL_POSIX;
 		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_start = (loff_t)lock->lock_start;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index e35c05e27806..32784f508c81 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -77,6 +77,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 
 		/* Set up the missing parts of the file_lock structure */
 		mode = lock_to_openmode(&lock->fl);
+		lock->fl.fl_flags = FL_POSIX;
 		lock->fl.fl_file  = file->f_file[mode];
 		lock->fl.fl_pid = current->tgid;
 		lock->fl.fl_lmops = &nlmsvc_lock_operations;
-- 
2.38.1

[PATCH 2/4] lockd: ensure we use the correct file description when unlocking

[Jeff Layton]

Shared locks are set on O_RDONLY descriptors and exclusive locks are set
on O_WRONLY ones. nlmsvc_unlock however calls vfs_lock_file twice, once
for each descriptor, but it doesn't reset fl_file. Ensure that it does.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/lockd/svclock.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 9c1aa75441e1..9eae99e08e69 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -659,11 +659,13 @@ nlmsvc_unlock(struct net *net, struct nlm_file *file, struct nlm_lock *lock)
 	nlmsvc_cancel_blocked(net, file, lock);
 
 	lock->fl.fl_type = F_UNLCK;
-	if (file->f_file[O_RDONLY])
-		error = vfs_lock_file(file->f_file[O_RDONLY], F_SETLK,
+	lock->fl.fl_file = file->f_file[O_RDONLY];
+	if (lock->fl.fl_file)
+		error = vfs_lock_file(lock->fl.fl_file, F_SETLK,
 					&lock->fl, NULL);
-	if (file->f_file[O_WRONLY])
-		error = vfs_lock_file(file->f_file[O_WRONLY], F_SETLK,
+	lock->fl.fl_file = file->f_file[O_WRONLY];
+	if (lock->fl.fl_file)
+		error |= vfs_lock_file(lock->fl.fl_file, F_SETLK,
 					&lock->fl, NULL);
 
 	return (error < 0)? nlm_lck_denied_nolocks : nlm_granted;
-- 
2.38.1

[PATCH 3/4] lockd: fix file selection in nlmsvc_cancel_blocked

[Jeff Layton]

We currently do a lock_to_openmode call based on the arguments from the
NLM_UNLOCK call, but that will always set the fl_type of the lock to
F_UNLCK, the the O_RDONLY descriptor is always chosen.

Fix it to use the file_lock from the block instead.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/lockd/svclock.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 9eae99e08e69..4e30f3c50970 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -699,9 +699,10 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
 	block = nlmsvc_lookup_block(file, lock);
 	mutex_unlock(&file->f_mutex);
 	if (block != NULL) {
-		mode = lock_to_openmode(&lock->fl);
-		vfs_cancel_lock(block->b_file->f_file[mode],
-				&block->b_call->a_args.lock.fl);
+		struct file_lock *fl = &block->b_call->a_args.lock.fl;
+
+		mode = lock_to_openmode(fl);
+		vfs_cancel_lock(block->b_file->f_file[mode], fl);
 		status = nlmsvc_unlink_block(block);
 		nlmsvc_release_block(block);
 	}
-- 
2.38.1

[PATCH 4/4] filelock: WARN_ON_ONCE when ->fl_file and filp don't match

[Jeff Layton]

vfs_lock_file, vfs_test_lock and vfs_cancel_lock all take both a struct
file argument and a file_lock. The file_lock has a fl_file field in it
howevever and it _must_ match the file passed in.

While most of the locks.c routines use the separately-passed file
argument, some filesystems rely on fl_file being filled out correctly.

I'm working on a patch series to remove the redundant argument from
these routines, but for now, let's ensure that the callers always set
this properly by issuing a WARN_ON_ONCE if they ever don't match.

Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Trond Myklebust <trondmy@hammerspace.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/locks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/locks.c b/fs/locks.c
index 607f94a0e789..5876c8ff0edc 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2146,6 +2146,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
  */
 int vfs_test_lock(struct file *filp, struct file_lock *fl)
 {
+	WARN_ON_ONCE(filp != fl->fl_file);
 	if (filp->f_op->lock)
 		return filp->f_op->lock(filp, F_GETLK, fl);
 	posix_test_lock(filp, fl);
@@ -2295,6 +2296,7 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
  */
 int vfs_lock_file(struct file *filp, unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
 {
+	WARN_ON_ONCE(filp != fl->fl_file);
 	if (filp->f_op->lock)
 		return filp->f_op->lock(filp, cmd, fl);
 	else
@@ -2663,6 +2665,7 @@ void locks_remove_file(struct file *filp)
  */
 int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
 {
+	WARN_ON_ONCE(filp != fl->fl_file);
 	if (filp->f_op->lock)
 		return filp->f_op->lock(filp, F_CANCELLK, fl);
 	return 0;
-- 
2.38.1

Re: [PATCH 4/4] filelock: WARN_ON_ONCE when ->fl_file and filp don't match

[Jeff Layton]

On Fri, 2022-11-11 at 14:36 -0500, Jeff Layton wrote:
> vfs_lock_file, vfs_test_lock and vfs_cancel_lock all take both a struct
> file argument and a file_lock. The file_lock has a fl_file field in it
> howevever and it _must_ match the file passed in.
> 
> While most of the locks.c routines use the separately-passed file
> argument, some filesystems rely on fl_file being filled out correctly.
> 
> I'm working on a patch series to remove the redundant argument from
> these routines, but for now, let's ensure that the callers always set
> this properly by issuing a WARN_ON_ONCE if they ever don't match.
> 
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Trond Myklebust <trondmy@hammerspace.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/locks.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 607f94a0e789..5876c8ff0edc 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2146,6 +2146,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>   */
>  int vfs_test_lock(struct file *filp, struct file_lock *fl)
>  {
> +	WARN_ON_ONCE(filp != fl->fl_file);
>  	if (filp->f_op->lock)
>  		return filp->f_op->lock(filp, F_GETLK, fl);
>  	posix_test_lock(filp, fl);
> @@ -2295,6 +2296,7 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
>   */
>  int vfs_lock_file(struct file *filp, unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
>  {
> +	WARN_ON_ONCE(filp != fl->fl_file);
>  	if (filp->f_op->lock)
>  		return filp->f_op->lock(filp, cmd, fl);
>  	else
> @@ -2663,6 +2665,7 @@ void locks_remove_file(struct file *filp)
>   */
>  int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
>  {
> +	WARN_ON_ONCE(filp != fl->fl_file);
>  	if (filp->f_op->lock)
>  		return filp->f_op->lock(filp, F_CANCELLK, fl);
>  	return 0;

Oops, I meant to cc linux-fsdevel here too. I'll plan to do that on the
next posting (assuming that I need to do one).

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/4] lockd: fix file selection in nlmsvc_cancel_blocked

[Chuck Lever III]

> On Nov 11, 2022, at 2:36 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> We currently do a lock_to_openmode call based on the arguments from the
> NLM_UNLOCK call, but that will always set the fl_type of the lock to
> F_UNLCK, the the O_RDONLY descriptor is always chosen.

Except for the above sentence, these all look sane to me.
I can apply them to nfsd's for-next once they've seen some
review on fsdevel, as you mentioned in the other thread.


> Fix it to use the file_lock from the block instead.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/lockd/svclock.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 9eae99e08e69..4e30f3c50970 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -699,9 +699,10 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> 	block = nlmsvc_lookup_block(file, lock);
> 	mutex_unlock(&file->f_mutex);
> 	if (block != NULL) {
> -		mode = lock_to_openmode(&lock->fl);
> -		vfs_cancel_lock(block->b_file->f_file[mode],
> -				&block->b_call->a_args.lock.fl);
> +		struct file_lock *fl = &block->b_call->a_args.lock.fl;
> +
> +		mode = lock_to_openmode(fl);
> +		vfs_cancel_lock(block->b_file->f_file[mode], fl);
> 		status = nlmsvc_unlink_block(block);
> 		nlmsvc_release_block(block);
> 	}
> -- 
> 2.38.1
> 

--
Chuck Lever

Re: [PATCH 3/4] lockd: fix file selection in nlmsvc_cancel_blocked

[Jeff Layton]

On Fri, 2022-11-11 at 20:29 +0000, Chuck Lever III wrote:
> 
> > On Nov 11, 2022, at 2:36 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > We currently do a lock_to_openmode call based on the arguments from the
> > NLM_UNLOCK call, but that will always set the fl_type of the lock to
> > F_UNLCK, the the O_RDONLY descriptor is always chosen.
> 
> Except for the above sentence, these all look sane to me.
> I can apply them to nfsd's for-next once they've seen some
> review on fsdevel, as you mentioned in the other thread.
> 
> 

Thanks. That should say "and the O_RDONLY...". Fixed in my tree.

I'll go ahead and resend with fsdevel included.

> > Fix it to use the file_lock from the block instead.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/lockd/svclock.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index 9eae99e08e69..4e30f3c50970 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -699,9 +699,10 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> > 	block = nlmsvc_lookup_block(file, lock);
> > 	mutex_unlock(&file->f_mutex);
> > 	if (block != NULL) {
> > -		mode = lock_to_openmode(&lock->fl);
> > -		vfs_cancel_lock(block->b_file->f_file[mode],
> > -				&block->b_call->a_args.lock.fl);
> > +		struct file_lock *fl = &block->b_call->a_args.lock.fl;
> > +
> > +		mode = lock_to_openmode(fl);
> > +		vfs_cancel_lock(block->b_file->f_file[mode], fl);
> > 		status = nlmsvc_unlink_block(block);
> > 		nlmsvc_release_block(block);
> > 	}
> > -- 
> > 2.38.1
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 0/4] filelock: WARN when @filp and fl_file don't match

[Jeff Layton]

Eventually, I'd like to reduce the redundant arguments to the locking
APIs, but to get there we need to ensure the callers all set their file
locks sanely.

Adding the WARN_ON_ONCEs helped to find a couple of warts in lockd's
file handling. The first 3 patches fix those.

I'd like to see these included in v6.2, with an eye toward a locking API
cleanup in v6.3.

Thanks,

Jeff Layton (4):
  lockd: set missing fl_flags field when retrieving args
  lockd: ensure we use the correct file description when unlocking
  lockd: fix file selection in nlmsvc_cancel_blocked
  filelock: WARN_ON_ONCE when ->fl_file and filp don't match

 fs/lockd/svc4proc.c |  1 +
 fs/lockd/svclock.c  | 17 ++++++++++-------
 fs/lockd/svcproc.c  |  1 +
 fs/locks.c          |  3 +++
 4 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.38.1

Re: [PATCH 3/4] lockd: fix file selection in nlmsvc_cancel_blocked

[Jeff Layton]

On Fri, 2022-11-11 at 16:52 -0500, Jeff Layton wrote:
> On Fri, 2022-11-11 at 20:29 +0000, Chuck Lever III wrote:
> > 
> > > On Nov 11, 2022, at 2:36 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > We currently do a lock_to_openmode call based on the arguments from the
> > > NLM_UNLOCK call, but that will always set the fl_type of the lock to
> > > F_UNLCK, the the O_RDONLY descriptor is always chosen.
> > 
> > Except for the above sentence, these all look sane to me.
> > I can apply them to nfsd's for-next once they've seen some
> > review on fsdevel, as you mentioned in the other thread.
> > 
> > 
> 
> Thanks. That should say "and the O_RDONLY...". Fixed in my tree.
> 
> I'll go ahead and resend with fsdevel included.
> 

I reposted the series Friday afternoon.

What might be best is for you to carry the first 3 patches in the nfsd
tree, and I'll take the filelock: patch into the locks-next branch,
along with the other filelock API cleanups.

Sound OK?

> > > Fix it to use the file_lock from the block instead.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > fs/lockd/svclock.c | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > > index 9eae99e08e69..4e30f3c50970 100644
> > > --- a/fs/lockd/svclock.c
> > > +++ b/fs/lockd/svclock.c
> > > @@ -699,9 +699,10 @@ nlmsvc_cancel_blocked(struct net *net, struct nlm_file *file, struct nlm_lock *l
> > > 	block = nlmsvc_lookup_block(file, lock);
> > > 	mutex_unlock(&file->f_mutex);
> > > 	if (block != NULL) {
> > > -		mode = lock_to_openmode(&lock->fl);
> > > -		vfs_cancel_lock(block->b_file->f_file[mode],
> > > -				&block->b_call->a_args.lock.fl);
> > > +		struct file_lock *fl = &block->b_call->a_args.lock.fl;
> > > +
> > > +		mode = lock_to_openmode(fl);
> > > +		vfs_cancel_lock(block->b_file->f_file[mode], fl);
> > > 		status = nlmsvc_unlink_block(block);
> > > 		nlmsvc_release_block(block);
> > > 	}
> > > -- 
> > > 2.38.1
> > > 
> > 
> > --
> > Chuck Lever
> > 
> > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/4] lockd: fix file selection in nlmsvc_cancel_blocked

[Chuck Lever III]

> On Nov 14, 2022, at 1:38 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Fri, 2022-11-11 at 16:52 -0500, Jeff Layton wrote:
>> On Fri, 2022-11-11 at 20:29 +0000, Chuck Lever III wrote:
>>> 
>>>> On Nov 11, 2022, at 2:36 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>> 
>>>> We currently do a lock_to_openmode call based on the arguments from the
>>>> NLM_UNLOCK call, but that will always set the fl_type of the lock to
>>>> F_UNLCK, the the O_RDONLY descriptor is always chosen.
>>> 
>>> Except for the above sentence, these all look sane to me.
>>> I can apply them to nfsd's for-next once they've seen some
>>> review on fsdevel, as you mentioned in the other thread.
>>> 
>>> 
>> 
>> Thanks. That should say "and the O_RDONLY...". Fixed in my tree.
>> 
>> I'll go ahead and resend with fsdevel included.
>> 
> 
> I reposted the series Friday afternoon.
> 
> What might be best is for you to carry the first 3 patches in the nfsd
> tree, and I'll take the filelock: patch into the locks-next branch,
> along with the other filelock API cleanups.
> 
> Sound OK?

1/4 through 3/4 have been applied and pushed. Thanks!

--
Chuck Lever