Re: [PATCH] locks: ensure that fl_owner is always initialized properly in flock and lease codepaths

["J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>]

Looks fine to me, ACK.--b.

On Mon, Apr 28, 2014 at 01:50:13PM -0400, Jeff Layton wrote:
> Currently, the fl_owner isn't set for flock locks. Some filesystems use
> byte-range locks to simulate flock locks and there is a common idiom in
> those that does:
> 
>     fl->fl_owner = (fl_owner_t)filp;
>     fl->fl_start = 0;
>     fl->fl_end = OFFSET_MAX;
> 
> Since flock locks are generally "owned" by the open file description,
> move this into the common flock lock setup code. The fl_start and fl_end
> fields are already set appropriately, so remove the unneeded setting of
> that in flock ops in those filesystems as well.
> 
> Finally, the lease code also sets the fl_owner as if they were owned by
> the process and not the open file description. This is incorrect as
> leases have the same ownership semantics as flock locks. Set them the
> same way. The lease code doesn't actually use the fl_owner value for
> anything, so this is more for consistency's sake than a bugfix.
> 
> Reported-by: Trond Myklebust <trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
> ---
>  drivers/staging/lustre/lustre/llite/file.c | 17 ++++++-----------
>  fs/9p/vfs_file.c                           |  3 ---
>  fs/afs/flock.c                             |  4 ----
>  fs/ceph/locks.c                            | 10 ++--------
>  fs/fuse/file.c                             |  1 -
>  fs/locks.c                                 |  4 +++-
>  fs/nfs/file.c                              |  4 ----
>  7 files changed, 11 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> index 8e844a6371e0..760ccd83f1f7 100644
> --- a/drivers/staging/lustre/lustre/llite/file.c
> +++ b/drivers/staging/lustre/lustre/llite/file.c
> @@ -2691,20 +2691,15 @@ int ll_file_flock(struct file *file, int cmd, struct file_lock *file_lock)
>  
>  	ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_FLOCK, 1);
>  
> -	if (file_lock->fl_flags & FL_FLOCK) {
> +	if (file_lock->fl_flags & FL_FLOCK)
>  		LASSERT((cmd == F_SETLKW) || (cmd == F_SETLK));
> -		/* flocks are whole-file locks */
> -		flock.l_flock.end = OFFSET_MAX;
> -		/* For flocks owner is determined by the local file descriptor*/
> -		flock.l_flock.owner = (unsigned long)file_lock->fl_file;
> -	} else if (file_lock->fl_flags & FL_POSIX) {
> -		flock.l_flock.owner = (unsigned long)file_lock->fl_owner;
> -		flock.l_flock.start = file_lock->fl_start;
> -		flock.l_flock.end = file_lock->fl_end;
> -	} else {
> +	else if (!(file_lock->fl_flags & FL_POSIX))
>  		return -EINVAL;
> -	}
> +
> +	flock.l_flock.owner = (unsigned long)file_lock->fl_owner;
>  	flock.l_flock.pid = file_lock->fl_pid;
> +	flock.l_flock.start = file_lock->fl_start;
> +	flock.l_flock.end = file_lock->fl_end;
>  
>  	/* Somewhat ugly workaround for svc lockd.
>  	 * lockd installs custom fl_lmops->lm_compare_owner that checks
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index d8223209d4b1..59e3fe3d56c0 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -352,9 +352,6 @@ static int v9fs_file_flock_dotl(struct file *filp, int cmd,
>  		invalidate_mapping_pages(&inode->i_data, 0, -1);
>  	}
>  	/* Convert flock to posix lock */
> -	fl->fl_owner = (fl_owner_t)filp;
> -	fl->fl_start = 0;
> -	fl->fl_end = OFFSET_MAX;
>  	fl->fl_flags |= FL_POSIX;
>  	fl->fl_flags ^= FL_FLOCK;
>  
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index a8cf2cff836c..4baf1d2b39e4 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -555,10 +555,6 @@ int afs_flock(struct file *file, int cmd, struct file_lock *fl)
>  		return -ENOLCK;
>  
>  	/* we're simulating flock() locks using posix locks on the server */
> -	fl->fl_owner = (fl_owner_t) file;
> -	fl->fl_start = 0;
> -	fl->fl_end = OFFSET_MAX;
> -
>  	if (fl->fl_type == F_UNLCK)
>  		return afs_do_unlk(file, fl);
>  	return afs_do_setlk(file, fl);
> diff --git a/fs/ceph/locks.c b/fs/ceph/locks.c
> index d94ba0df9f4d..db8c1ca15d72 100644
> --- a/fs/ceph/locks.c
> +++ b/fs/ceph/locks.c
> @@ -52,10 +52,7 @@ static int ceph_lock_message(u8 lock_type, u16 operation, struct file *file,
>  	else
>  		length = fl->fl_end - fl->fl_start + 1;
>  
> -	if (lock_type == CEPH_LOCK_FCNTL)
> -		owner = secure_addr(fl->fl_owner);
> -	else
> -		owner = secure_addr(fl->fl_file);
> +	owner = secure_addr(fl->fl_owner);
>  
>  	dout("ceph_lock_message: rule: %d, op: %d, owner: %llx, pid: %llu, "
>  	     "start: %llu, length: %llu, wait: %d, type: %d", (int)lock_type,
> @@ -313,10 +310,7 @@ int lock_to_ceph_filelock(struct file_lock *lock,
>  	cephlock->length = cpu_to_le64(lock->fl_end - lock->fl_start + 1);
>  	cephlock->client = cpu_to_le64(0);
>  	cephlock->pid = cpu_to_le64((u64)lock->fl_pid);
> -	if (lock->fl_flags & FL_POSIX)
> -		cephlock->owner = cpu_to_le64(secure_addr(lock->fl_owner));
> -	else
> -		cephlock->owner = cpu_to_le64(secure_addr(lock->fl_file));
> +	cephlock->owner = cpu_to_le64(secure_addr(lock->fl_owner));
>  
>  	switch (lock->fl_type) {
>  	case F_RDLCK:
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 13f8bdec5110..0b25fec89558 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2281,7 +2281,6 @@ static int fuse_file_flock(struct file *file, int cmd, struct file_lock *fl)
>  		struct fuse_file *ff = file->private_data;
>  
>  		/* emulate flock with POSIX locks */
> -		fl->fl_owner = (fl_owner_t) file;
>  		ff->flock = true;
>  		err = fuse_setlk(file, fl, 1);
>  	}
> diff --git a/fs/locks.c b/fs/locks.c
> index e663aeac579e..2129ba8ac062 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -322,6 +322,7 @@ static int flock_make_lock(struct file *filp, struct file_lock **lock,
>  		return -ENOMEM;
>  
>  	fl->fl_file = filp;
> +	fl->fl_owner = (fl_owner_t)filp;
>  	fl->fl_pid = current->tgid;
>  	fl->fl_flags = FL_FLOCK;
>  	fl->fl_type = type;
> @@ -439,7 +440,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl)
>  	if (assign_type(fl, type) != 0)
>  		return -EINVAL;
>  
> -	fl->fl_owner = current->files;
> +	fl->fl_owner = (fl_owner_t)filp;
>  	fl->fl_pid = current->tgid;
>  
>  	fl->fl_file = filp;
> @@ -2304,6 +2305,7 @@ void locks_remove_file(struct file *filp)
>  
>  	if (filp->f_op->flock) {
>  		struct file_lock fl = {
> +			.fl_owner = (fl_owner_t)filp,
>  			.fl_pid = current->tgid,
>  			.fl_file = filp,
>  			.fl_flags = FL_FLOCK,
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 284ca901fe16..c1edf7336315 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -916,10 +916,6 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
>  		is_local = 1;
>  
>  	/* We're simulating flock() locks using posix locks on the server */
> -	fl->fl_owner = (fl_owner_t)filp;
> -	fl->fl_start = 0;
> -	fl->fl_end = OFFSET_MAX;
> -
>  	if (fl->fl_type == F_UNLCK)
>  		return do_unlk(filp, cmd, fl, is_local);
>  	return do_setlk(filp, cmd, fl, is_local);
> -- 
> 1.9.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html