On Mon, Mar 10, 2014 at 09:36:46AM -0400, Jeff Layton wrote:
> As Trond pointed out, you can currently deadlock yourself by setting a
> file-private lock on a file that requires mandatory locking and then
> trying to do I/O on it.
>
> Avoid this problem by plumbing some knowledge of file-private locks into
> the mandatory locking code. In order to do this, we must pass down
> information about the struct file that's being used to
> locks_verify_locked.
>
> Reported-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/locks.c | 7 ++++---
> fs/namei.c | 2 +-
> include/linux/fs.h | 20 ++++++++++----------
> mm/mmap.c | 2 +-
> mm/nommu.c | 2 +-
> 5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 6fdf26a79cc8..b2b3e97b64d4 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1155,13 +1155,14 @@ EXPORT_SYMBOL(posix_lock_file_wait);
>
> /**
> * locks_mandatory_locked - Check for an active lock
> - * @inode: the file to check
> + * @file: the file to check
> *
> * Searches the inode's list of locks to find any POSIX locks which conflict.
> * This function is called from locks_verify_locked() only.
> */
> -int locks_mandatory_locked(struct inode *inode)
> +int locks_mandatory_locked(struct file *file)
> {
> + struct inode *inode = file_inode(file);
> fl_owner_t owner = current->files;
> struct file_lock *fl;
>
> @@ -1172,7 +1173,7 @@ int locks_mandatory_locked(struct inode *inode)
> for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) {
> if (!IS_POSIX(fl))
> continue;
> - if (fl->fl_owner != owner)
> + if (fl->fl_owner != owner && fl->fl_owner != (fl_owner_t)file)
I had to think about that logic for a moment.
If this is our traditional lock, then fl_owner will point to our file
table. And if it's our file-private lock, it will point to our file.
If it points to neither, it's a potentially conflicting lock.
NFS locks will also always be treated as conflicting (since fl_owner
points to some completely different kind of object in that case).
OK.
Acked-by: J. Bruce Fields <bfields@xxxxxxxxxx>
--b.
> break;
> }
> spin_unlock(&inode->i_lock);
> diff --git a/fs/namei.c b/fs/namei.c
> index d580df2e6804..dc51bac037c9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2542,7 +2542,7 @@ static int handle_truncate(struct file *filp)
> /*
> * Refuse to truncate files with mandatory locks held on them.
> */
> - error = locks_verify_locked(inode);
> + error = locks_verify_locked(filp);
> if (!error)
> error = security_path_truncate(path);
> if (!error) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ae91dce8a547..e39d83db72c0 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1912,6 +1912,11 @@ extern int current_umask(void);
> extern void ihold(struct inode * inode);
> extern void iput(struct inode *);
>
> +static inline struct inode *file_inode(struct file *f)
> +{
> + return f->f_inode;
> +}
> +
> /* /sys/fs */
> extern struct kobject *fs_kobj;
>
> @@ -1921,7 +1926,7 @@ extern struct kobject *fs_kobj;
> #define FLOCK_VERIFY_WRITE 2
>
> #ifdef CONFIG_FILE_LOCKING
> -extern int locks_mandatory_locked(struct inode *);
> +extern int locks_mandatory_locked(struct file *);
> extern int locks_mandatory_area(int, struct inode *, struct file *, loff_t, size_t);
>
> /*
> @@ -1944,10 +1949,10 @@ static inline int mandatory_lock(struct inode *ino)
> return IS_MANDLOCK(ino) && __mandatory_lock(ino);
> }
>
> -static inline int locks_verify_locked(struct inode *inode)
> +static inline int locks_verify_locked(struct file *file)
> {
> - if (mandatory_lock(inode))
> - return locks_mandatory_locked(inode);
> + if (mandatory_lock(file_inode(file)))
> + return locks_mandatory_locked(file);
> return 0;
> }
>
> @@ -2008,7 +2013,7 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
> }
>
> #else /* !CONFIG_FILE_LOCKING */
> -static inline int locks_mandatory_locked(struct inode *inode)
> +static inline int locks_mandatory_locked(struct file *file)
> {
> return 0;
> }
> @@ -2297,11 +2302,6 @@ static inline bool execute_ok(struct inode *inode)
> return (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode);
> }
>
> -static inline struct inode *file_inode(struct file *f)
> -{
> - return f->f_inode;
> -}
> -
> static inline void file_start_write(struct file *file)
> {
> if (!S_ISREG(file_inode(file)->i_mode))
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 20ff0c33274c..5932ce961218 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1299,7 +1299,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
> /*
> * Make sure there are no mandatory locks on the file.
> */
> - if (locks_verify_locked(inode))
> + if (locks_verify_locked(file))
> return -EAGAIN;
>
> vm_flags |= VM_SHARED | VM_MAYSHARE;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 8740213b1647..a554e5a451cd 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -995,7 +995,7 @@ static int validate_mmap_request(struct file *file,
> (file->f_mode & FMODE_WRITE))
> return -EACCES;
>
> - if (locks_verify_locked(file_inode(file)))
> + if (locks_verify_locked(file))
> return -EAGAIN;
>
> if (!(capabilities & BDI_CAP_MAP_DIRECT))
> --
> 1.8.5.3
>
--
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
