[RFC PATCH v2] cifs: Fix cifsInodeInfo lock_sem deadlock with multiple readers

[Dave Wysochanski <dwysocha@redhat.com>]

This patch applies on 5.4-rc4 plus Pavel's patch:
"CIFS: Fix retry mid list corruption on reconnects"

v2 Changes
* In the original version I had erroneously moved lock_sem
inside cifs_find_lock_conflict but the lock_sem should be
taken in the caller of that function not inside it.
* I have started to run some basic locking tests with 
disruptions of the server and they run ok so far.


There's a deadlock that is possible that can easily be seen with
multiple readers open/read/close of the same file.  The deadlock
is due to a reader calling down_read(lock_sem) and holding
it across the full IO, even if a network or server disruption
occurs and the session has to be reconnected.  Upon reconnect,
cifs_relock_file is called where down_read(lock_sem) is called
a second time.  Normally this is not a problem, but if there is
another process that calls down_write(lock_sem) in between the
first and second reader call to down_read(lock_sem), this will
cause a deadlock.  The caller of down_write (often either
_cifsFileInfo_put that is just removing and freeing cifsLockInfo
structures from the list of locks, or cifs_new_fileinfo, which
is just attaching cifs_fid_locks to cifsInodeInfo->llist), will
block due to the reader's first down_read(lock_sem) that obtains
the semaphore (read IO in flight).  And then when the server
comes back up, the reader that holds calls down_read(lock_sem)
a second time, and this time is blocked too because of the
blocked in down_write (rw_semaphores would starve writers if
this was not the case).  Interestingly enough, the callers of
down_write in the simple test case was not adding a
conflicting lock at all, just either opening or closing the
file, and modifying the list of locks attached to cifsInodeInfo,
this ends up tripping up the reader process and causing the
deadlock.

The root of the problem is that lock_sem both protects the
cifsInodeInfo fields (such as the lllist - the list of locks),
but is also being re-used to prevent a conflicting lock added
while IO is in flight.  Add a new semaphore that tracks
just the IO in flight, and must be obtained before adding
a new lock.  While this does add another layer of complexity
and a semaphore ordering that must be obeyed to avoid new
deadlocks, it does clealy solve the underlying problem of
the deadlock and double call to down_read by the same thread.

Due to the removal of the possibility of a thread calling
down_read(lock_sem) twice, this patch also reverts
560d388950ce ("CIFS: silence lockdep splat in cifs_relock_file()")

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/cifs/cifsfs.c   |  1 +
 fs/cifs/cifsglob.h |  1 +
 fs/cifs/file.c     | 32 +++++++++++++++++++++++++-------
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c049c7b3aa87..10f614324e4e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1336,6 +1336,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
 
 	inode_init_once(&cifsi->vfs_inode);
 	init_rwsem(&cifsi->lock_sem);
+	init_rwsem(&cifsi->io_inflight_sem);
 }
 
 static int __init
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 50dfd9049370..40e8358dc1cc 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1392,6 +1392,7 @@ struct cifsInodeInfo {
 	bool can_cache_brlcks;
 	struct list_head llist;	/* locks helb by this inode */
 	struct rw_semaphore lock_sem;	/* protect the fields above */
+	struct rw_semaphore io_inflight_sem; /* Used to avoid lock conflicts  */
 	/* BB add in lists for dirty pages i.e. write caching info for oplock */
 	struct list_head openFileList;
 	spinlock_t	open_file_lock;	/* protects openFileList */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5ad15de2bb4f..a8b494205781 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -621,7 +621,7 @@ int cifs_open(struct inode *inode, struct file *file)
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
 	int rc = 0;
 
-	down_read_nested(&cinode->lock_sem, SINGLE_DEPTH_NESTING);
+	down_read(&cinode->lock_sem);
 	if (cinode->can_cache_brlcks) {
 		/* can cache locks - no need to relock */
 		up_read(&cinode->lock_sem);
@@ -1027,9 +1027,11 @@ int cifs_closedir(struct inode *inode, struct file *file)
 cifs_lock_add(struct cifsFileInfo *cfile, struct cifsLockInfo *lock)
 {
 	struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry));
+	down_write(&cinode->io_inflight_sem);
 	down_write(&cinode->lock_sem);
 	list_add_tail(&lock->llist, &cfile->llist->locks);
 	up_write(&cinode->lock_sem);
+	up_write(&cinode->io_inflight_sem);
 }
 
 /*
@@ -1049,6 +1051,7 @@ int cifs_closedir(struct inode *inode, struct file *file)
 
 try_again:
 	exist = false;
+	down_write(&cinode->io_inflight_sem);
 	down_write(&cinode->lock_sem);
 
 	exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length,
@@ -1057,6 +1060,7 @@ int cifs_closedir(struct inode *inode, struct file *file)
 	if (!exist && cinode->can_cache_brlcks) {
 		list_add_tail(&lock->llist, &cfile->llist->locks);
 		up_write(&cinode->lock_sem);
+		up_write(&cinode->io_inflight_sem);
 		return rc;
 	}
 
@@ -1077,6 +1081,7 @@ int cifs_closedir(struct inode *inode, struct file *file)
 	}
 
 	up_write(&cinode->lock_sem);
+	up_write(&cinode->io_inflight_sem);
 	return rc;
 }
 
@@ -1125,14 +1130,17 @@ int cifs_closedir(struct inode *inode, struct file *file)
 		return rc;
 
 try_again:
+	down_write(&cinode->io_inflight_sem);
 	down_write(&cinode->lock_sem);
 	if (!cinode->can_cache_brlcks) {
 		up_write(&cinode->lock_sem);
+		down_write(&cinode->io_inflight_sem);
 		return rc;
 	}
 
 	rc = posix_lock_file(file, flock, NULL);
 	up_write(&cinode->lock_sem);
+	up_write(&cinode->io_inflight_sem);
 	if (rc == FILE_LOCK_DEFERRED) {
 		rc = wait_event_interruptible(flock->fl_wait, !flock->fl_blocker);
 		if (!rc)
@@ -1331,6 +1339,7 @@ struct lock_to_push {
 	int rc = 0;
 
 	/* we are going to update can_cache_brlcks here - need a write access */
+	down_write(&cinode->io_inflight_sem);
 	down_write(&cinode->lock_sem);
 	if (!cinode->can_cache_brlcks) {
 		up_write(&cinode->lock_sem);
@@ -1346,6 +1355,7 @@ struct lock_to_push {
 
 	cinode->can_cache_brlcks = false;
 	up_write(&cinode->lock_sem);
+	up_write(&cinode->io_inflight_sem);
 	return rc;
 }
 
@@ -1522,6 +1532,7 @@ struct lock_to_push {
 	if (!buf)
 		return -ENOMEM;
 
+	down_write(&cinode->io_inflight_sem);
 	down_write(&cinode->lock_sem);
 	for (i = 0; i < 2; i++) {
 		cur = buf;
@@ -1593,6 +1604,7 @@ struct lock_to_push {
 	}
 
 	up_write(&cinode->lock_sem);
+	up_write(&cinode->io_inflight_sem);
 	kfree(buf);
 	return rc;
 }
@@ -3148,20 +3160,23 @@ ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
 	 * We need to hold the sem to be sure nobody modifies lock list
 	 * with a brlock that prevents writing.
 	 */
-	down_read(&cinode->lock_sem);
+	down_read(&cinode->io_inflight_sem);
 
 	rc = generic_write_checks(iocb, from);
 	if (rc <= 0)
 		goto out;
 
-	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
+	down_read(&cinode->lock_sem);
+	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from), 
 				     server->vals->exclusive_lock_type, 0,
-				     NULL, CIFS_WRITE_OP))
+				     NULL, CIFS_WRITE_OP)) {
+		up_read(&cinode->lock_sem);
 		rc = __generic_file_write_iter(iocb, from);
+	}
 	else
 		rc = -EACCES;
 out:
-	up_read(&cinode->lock_sem);
+	up_read(&cinode->io_inflight_sem);
 	inode_unlock(inode);
 
 	if (rc > 0)
@@ -3887,12 +3902,15 @@ ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to)
 	 * We need to hold the sem to be sure nobody modifies lock list
 	 * with a brlock that prevents reading.
 	 */
+	down_read(&cinode->io_inflight_sem);
 	down_read(&cinode->lock_sem);
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
 				     tcon->ses->server->vals->shared_lock_type,
-				     0, NULL, CIFS_READ_OP))
+				     0, NULL, CIFS_READ_OP)) {
+		up_read(&cinode->lock_sem);
 		rc = generic_file_read_iter(iocb, to);
-	up_read(&cinode->lock_sem);
+	}
+	up_read(&cinode->io_inflight_sem);
 	return rc;
 }
 
-- 
1.8.3.1