From 24cbe7845ea50b636ab2218b9d648270ff55f148 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 3 Feb 2014 12:13:06 -0500 Subject: locks: close potential race between setlease and open As Al Viro points out, there is an unlikely, but possible race between opening a file and setting a lease on it. generic_add_lease is done with the i_lock held, but the inode->i_flock check in break_lease is lockless. It's possible for another task doing an open to do the entire pathwalk and call break_lease between the point where generic_add_lease checks for a conflicting open and adds the lease to the list. If this occurs, we can end up with a lease set on the file with a conflicting open. To guard against that, check again for a conflicting open after adding the lease to the i_flock list. If the above race occurs, then we can simply unwind the lease setting and return -EAGAIN. Because we take dentry references and acquire write access on the file before calling break_lease, we know that if the i_flock list is empty when the open caller goes to check it then the necessary refcounts have already been incremented. Thus the additional check for a conflicting open will see that there is one and the setlease call will fail. Cc: Bruce Fields Cc: David Howells Cc: "Paul E. McKenney" Reported-by: Al Viro Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- include/linux/fs.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'include/linux/fs.h') diff --git a/include/linux/fs.h b/include/linux/fs.h index 09f553c59813..df8474408331 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1964,6 +1964,12 @@ static inline int locks_verify_truncate(struct inode *inode, static inline int break_lease(struct inode *inode, unsigned int mode) { + /* + * Since this check is lockless, we must ensure that any refcounts + * taken are done before checking inode->i_flock. Otherwise, we could + * end up racing with tasks trying to set a new lease on this file. + */ + smp_mb(); if (inode->i_flock) return __break_lease(inode, mode, FL_LEASE); return 0; -- cgit v1.2.3-11-g984f From 78ed8a13382b1354e95d0f2233577eba15cb8171 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 3 Feb 2014 12:13:08 -0500 Subject: locks: rename locks_remove_flock to locks_remove_file This function currently removes leases in addition to flock locks and in a later patch we'll have it deal with file-private locks too. Rename it to locks_remove_file to indicate that it removes locks that are associated with a particular struct file, and not just flock locks. Acked-by: J. Bruce Fields Signed-off-by: Jeff Layton --- include/linux/fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include/linux/fs.h') diff --git a/include/linux/fs.h b/include/linux/fs.h index df8474408331..7527d96913d3 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1012,7 +1012,7 @@ extern struct file_lock * locks_alloc_lock(void); extern void locks_copy_lock(struct file_lock *, struct file_lock *); extern void __locks_copy_lock(struct file_lock *, const struct file_lock *); extern void locks_remove_posix(struct file *, fl_owner_t); -extern void locks_remove_flock(struct file *); +extern void locks_remove_file(struct file *); extern void locks_release_private(struct file_lock *); extern void posix_test_lock(struct file *, struct file_lock *); extern int posix_lock_file(struct file *, struct file_lock *, struct file_lock *); @@ -1083,7 +1083,7 @@ static inline void locks_remove_posix(struct file *filp, fl_owner_t owner) return; } -static inline void locks_remove_flock(struct file *filp) +static inline void locks_remove_file(struct file *filp) { return; } -- cgit v1.2.3-11-g984f From c918d42a27a9be0d78be490997d16d79cd5b9193 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 3 Feb 2014 12:13:09 -0500 Subject: locks: make /proc/locks show IS_FILE_PVT locks as type "FLPVT" In a later patch, we'll be adding a new type of lock that's owned by the struct file instead of the files_struct. Those sorts of locks will be flagged with a new FL_FILE_PVT flag. Report these types of locks as "FLPVT" in /proc/locks to distinguish them from "classic" POSIX locks. Acked-by: J. Bruce Fields Signed-off-by: Jeff Layton --- include/linux/fs.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/linux/fs.h') diff --git a/include/linux/fs.h b/include/linux/fs.h index 7527d96913d3..5ddeb8de5e77 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -888,6 +888,7 @@ static inline int file_check_writeable(struct file *filp) #define FL_SLEEP 128 /* A blocking lock */ #define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */ #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ +#define FL_FILE_PVT 1024 /* lock is private to the file */ /* * Special return value from posix_lock_file() and vfs_lock_file() for -- cgit v1.2.3-11-g984f From c1e62b8fc355e0c3706f1ae0dacb72d1c514dc80 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 3 Feb 2014 12:13:09 -0500 Subject: locks: pass the cmd value to fcntl_getlk/getlk64 Once we introduce file private locks, we'll need to know what cmd value was used, as that affects the ownership and whether a conflict would arise. Signed-off-by: Jeff Layton --- include/linux/fs.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'include/linux/fs.h') diff --git a/include/linux/fs.h b/include/linux/fs.h index 5ddeb8de5e77..ae91dce8a547 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -993,12 +993,12 @@ struct file_lock { extern void send_sigio(struct fown_struct *fown, int fd, int band); #ifdef CONFIG_FILE_LOCKING -extern int fcntl_getlk(struct file *, struct flock __user *); +extern int fcntl_getlk(struct file *, unsigned int, struct flock __user *); extern int fcntl_setlk(unsigned int, struct file *, unsigned int, struct flock __user *); #if BITS_PER_LONG == 32 -extern int fcntl_getlk64(struct file *, struct flock64 __user *); +extern int fcntl_getlk64(struct file *, unsigned int, struct flock64 __user *); extern int fcntl_setlk64(unsigned int, struct file *, unsigned int, struct flock64 __user *); #endif @@ -1031,7 +1031,8 @@ extern int lease_modify(struct file_lock **, int); extern int lock_may_read(struct inode *, loff_t start, unsigned long count); extern int lock_may_write(struct inode *, loff_t start, unsigned long count); #else /* !CONFIG_FILE_LOCKING */ -static inline int fcntl_getlk(struct file *file, struct flock __user *user) +static inline int fcntl_getlk(struct file *file, unsigned int cmd, + struct flock __user *user) { return -EINVAL; } @@ -1043,7 +1044,8 @@ static inline int fcntl_setlk(unsigned int fd, struct file *file, } #if BITS_PER_LONG == 32 -static inline int fcntl_getlk64(struct file *file, struct flock64 __user *user) +static inline int fcntl_getlk64(struct file *file, unsigned int cmd, + struct flock64 __user *user) { return -EINVAL; } -- cgit v1.2.3-11-g984f From d7a06983a01a33605191c0766857b832ac32a2b6 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 10 Mar 2014 09:54:15 -0400 Subject: locks: fix locks_mandatory_locked to respect file-private locks 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 Signed-off-by: Jeff Layton Acked-by: J. Bruce Fields --- include/linux/fs.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'include/linux/fs.h') diff --git a/include/linux/fs.h b/include/linux/fs.h index ae91dce8a547..4aa81e6ae067 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; } @@ -2030,7 +2035,7 @@ static inline int mandatory_lock(struct inode *inode) return 0; } -static inline int locks_verify_locked(struct inode *inode) +static inline int locks_verify_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)) -- cgit v1.2.3-11-g984f