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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The Ceph bit is fine.
Acked-by: Greg Farnum <greg@xxxxxxxxxxx>


On Mon, Apr 28, 2014 at 10:50 AM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> ---
>  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-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux