Re: [PATCH] lockd: set other missing fields when unlocking files

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2022-11-06 at 14:02 -0500, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> vfs_lock_file() expects the struct file_lock to be fully initialised by
> the caller. Re-exported NFSv3 has been seen to Oops if the fl_file field
> is NULL.
> 
> Fixes: aec158242b87 ("lockd: set fl_owner when unlocking files")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/lockd/svcsubs.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index e1c4617de771..3515f17eaf3f 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -176,7 +176,7 @@ nlm_delete_file(struct nlm_file *file)
>  	}
>  }
>  
> -static int nlm_unlock_files(struct nlm_file *file, fl_owner_t owner)
> +static int nlm_unlock_files(struct nlm_file *file, const struct file_lock *fl)
>  {
>  	struct file_lock lock;
>  
> @@ -184,12 +184,15 @@ static int nlm_unlock_files(struct nlm_file *file, fl_owner_t owner)
>  	lock.fl_type  = F_UNLCK;
>  	lock.fl_start = 0;
>  	lock.fl_end   = OFFSET_MAX;
> -	lock.fl_owner = owner;
> -	if (file->f_file[O_RDONLY] &&
> -	    vfs_lock_file(file->f_file[O_RDONLY], F_SETLK, &lock, NULL))
> +	lock.fl_owner = fl->fl_owner;
> +	lock.fl_pid   = fl->fl_pid;
> +	lock.fl_flags = FL_POSIX;
> +
> +	lock.fl_file = file->f_file[O_RDONLY];
> +	if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
>  		goto out_err;
> -	if (file->f_file[O_WRONLY] &&
> -	    vfs_lock_file(file->f_file[O_WRONLY], F_SETLK, &lock, NULL))
> +	lock.fl_file = file->f_file[O_WRONLY];
> +	if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
>  		goto out_err;
>  	return 0;
>  out_err:
> @@ -226,7 +229,7 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
>  		if (match(lockhost, host)) {
>  
>  			spin_unlock(&flctx->flc_lock);
> -			if (nlm_unlock_files(file, fl->fl_owner))
> +			if (nlm_unlock_files(file, fl))
>  				return 1;
>  			goto again;
>  		}

Good catch.

I wonder if we ought to roll an initializer function for file_locks to
make it harder for callers to miss setting some fields like this? One
idea: we could change vfs_lock_file to *not* take a file argument, and
insist that the caller fill out fl_file when calling it? That would make
it harder to screw this up.

In any case, let's take this patch in the interim while we consider
whether and how to clean this up.

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