Re: [PATCH 1/2] locks: fix locks_mandatory_locked to respect file-private locks

["J. Bruce Fields" <bfields@fieldses.org>]

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@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  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@redhat.com>

--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
>