From 39db810cb6c1e7d1f2e43ae38b437b7ee72fe815 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 24 Aug 2007 03:16:51 +0000 Subject: [CIFS] Byte range unlock request to non-Unix server can unlock too much On a mount without posix extensions enabled, when an unlock request is made, the client can release more than is intended. To reproduce, on a CIFS mount without posix extensions enabled: 1) open file 2) do fcntl lock: start=0 len=1 3) do fcntl lock: start=2 len=1 4) do fcntl unlock: start=0 len=1 ...on the unlock call the client sends an unlock request to the server for both locks. The problem is a bad test in cifs_lock. Signed-off-by: Jeff Layton Signed-off-by: Steve French --- fs/cifs/file.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 894b1f7b299..f9bd8b83f40 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -767,7 +767,8 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock) mutex_lock(&fid->lock_mutex); list_for_each_entry_safe(li, tmp, &fid->llist, llist) { if (pfLock->fl_start <= li->offset && - length >= li->length) { + (pflock->fl_start + length) >= + (li->offset + li->length)) { stored_rc = CIFSSMBLock(xid, pTcon, netfid, li->length, li->offset, -- cgit v1.2.3 From c19eb71020687e178b9fa564f4a8ac1880f87b10 Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 24 Aug 2007 03:22:48 +0000 Subject: [CIFS] fix typo in previous Signed-off-by: Steve French --- fs/cifs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index f9bd8b83f40..26fa50858aa 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -767,7 +767,7 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock) mutex_lock(&fid->lock_mutex); list_for_each_entry_safe(li, tmp, &fid->llist, llist) { if (pfLock->fl_start <= li->offset && - (pflock->fl_start + length) >= + (pfLock->fl_start + length) >= (li->offset + li->length)) { stored_rc = CIFSSMBLock(xid, pTcon, netfid, -- cgit v1.2.3 From 15745320f374aa6cbfe4836b76469159c0f49640 Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 7 Sep 2007 22:23:48 +0000 Subject: [CIFS] Fix oops in find_writable_file There was a case in which find_writable_file was not waiting long enough under heavy stress when writepages was racing with close of the file handle being used by the write. Signed-off-by: Steve French --- fs/cifs/file.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 26fa50858aa..b1807fd1ac4 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -467,7 +467,7 @@ reopen_error_exit: int cifs_close(struct inode *inode, struct file *file) { int rc = 0; - int xid; + int xid, timeout; struct cifs_sb_info *cifs_sb; struct cifsTconInfo *pTcon; struct cifsFileInfo *pSMBFile = @@ -485,9 +485,9 @@ int cifs_close(struct inode *inode, struct file *file) /* no sense reconnecting to close a file that is already closed */ if (pTcon->tidStatus != CifsNeedReconnect) { - int timeout = 2; + timeout = 2; while ((atomic_read(&pSMBFile->wrtPending) != 0) - && (timeout < 1000) ) { + && (timeout <= 2048)) { /* Give write a better chance to get to server ahead of the close. We do not want to add a wait_q here as it would @@ -522,6 +522,23 @@ int cifs_close(struct inode *inode, struct file *file) list_del(&pSMBFile->flist); list_del(&pSMBFile->tlist); write_unlock(&GlobalSMBSeslock); + timeout = 10; + /* We waited above to give the SMBWrite a chance to issue + on the wire (so we do not get SMBWrite returning EBADF + if writepages is racing with close. Note that writepages + does not specify a file handle, so it is possible for a file + to be opened twice, and the application close the "wrong" + file handle - in these cases we delay long enough to allow + the SMBWrite to get on the wire before the SMB Close. + We allow total wait here over 45 seconds, more than + oplock break time, and more than enough to allow any write + to complete on the server, or to time out on the client */ + while ((atomic_read(&pSMBFile->wrtPending) != 0) + && (timeout <= 50000)) { + cERROR(1, ("writes pending, delay free of handle")); + msleep(timeout); + timeout *= 8; + } kfree(pSMBFile->search_resume_name); kfree(file->private_data); file->private_data = NULL; @@ -1031,22 +1048,24 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode) (open_file->pfile->f_flags & O_WRONLY))) { atomic_inc(&open_file->wrtPending); read_unlock(&GlobalSMBSeslock); - if ((open_file->invalidHandle) && - (!open_file->closePend) /* BB fixme -since the second clause can not be true remove it BB */) { + if (open_file->invalidHandle) { rc = cifs_reopen_file(open_file->pfile, FALSE); /* if it fails, try another handle - might be */ /* dangerous to hold up writepages with retry */ if (rc) { - cFYI(1, - ("failed on reopen file in wp")); + cFYI(1, ("wp failed on reopen file")); read_lock(&GlobalSMBSeslock); /* can not use this handle, no write pending on this one after all */ - atomic_dec - (&open_file->wrtPending); + atomic_dec(&open_file->wrtPending); continue; } } + if (open_file->closePend) { + read_lock(&GlobalSMBSeslock); + atomic_dec(&open_file->wrtPending); + continue; + } return open_file; } } -- cgit v1.2.3 From 4efa53f0907bb4378015c129a2c11b8d3a90bce2 Mon Sep 17 00:00:00 2001 From: Steve French Date: Tue, 11 Sep 2007 05:50:53 +0000 Subject: [CIFS] lock inode open file list in close in case racing with open Harmless since it only protected turning off caching for the inode, but cleaner to lock around this in case we have a close racing with open. Signed-off-by: Shaggy CC: Cyrill Gorcunov Signed-off-by: Steve French --- fs/cifs/file.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index b1807fd1ac4..79254919386 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -545,6 +545,7 @@ int cifs_close(struct inode *inode, struct file *file) } else rc = -EBADF; + read_lock(&GlobalSMBSeslock); if (list_empty(&(CIFS_I(inode)->openFileList))) { cFYI(1, ("closing last open instance for inode %p", inode)); /* if the file is not open we do not know if we can cache info @@ -552,6 +553,7 @@ int cifs_close(struct inode *inode, struct file *file) CIFS_I(inode)->clientCanCacheRead = FALSE; CIFS_I(inode)->clientCanCacheAll = FALSE; } + read_unlock(&GlobalSMBSeslock); if ((rc == 0) && CIFS_I(inode)->write_behind_rc) rc = CIFS_I(inode)->write_behind_rc; FreeXid(xid); -- cgit v1.2.3 From 9b22b0b726c6e46048767728a0900c8c05f93c21 Mon Sep 17 00:00:00 2001 From: Steve French Date: Tue, 2 Oct 2007 01:11:08 +0000 Subject: [CIFS] Reduce chance of list corruption in find_writable_file When find_writable_file is racing with close and the session to the server goes down, Shaggy noticed that there was a chance that an open file in the list of files off the inode could have been freed by close since cifs_reconnect can block (the spinlock thus not held). This means that we have to start over at the beginning of the list in some cases. There is a 2nd change that needs to be made later (pointed out by Jeremy Allison and Shaggy) in order to prevent cifs_close ever freeing the cifs per file info when a write is pending. Although we delay close from freeing this memory for sufficiently long for all known cases, ultimately on a very, very slow write overlapping a close pending we need to allow close to return (without freeing the cifs file info) and defer freeing the memory to be the responsibility of the (sloooow) write thread (presumably have to look at every place wrtPending is decremented - and add a flag for deferred free for after wrtPending goes to zero). Acked-by: Shaggy Acked-by: Shirish Pargaonkar Signed-off-by: Steve French --- fs/cifs/file.c | 54 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 15 deletions(-) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 79254919386..780c0e3e470 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1042,6 +1042,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode) } read_lock(&GlobalSMBSeslock); +refind_writable: list_for_each_entry(open_file, &cifs_inode->openFileList, flist) { if (open_file->closePend) continue; @@ -1049,26 +1050,49 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode) ((open_file->pfile->f_flags & O_RDWR) || (open_file->pfile->f_flags & O_WRONLY))) { atomic_inc(&open_file->wrtPending); + + if (!open_file->invalidHandle) { + /* found a good writable file */ + read_unlock(&GlobalSMBSeslock); + return open_file; + } + read_unlock(&GlobalSMBSeslock); - if (open_file->invalidHandle) { - rc = cifs_reopen_file(open_file->pfile, FALSE); - /* if it fails, try another handle - might be */ - /* dangerous to hold up writepages with retry */ - if (rc) { - cFYI(1, ("wp failed on reopen file")); + /* Had to unlock since following call can block */ + rc = cifs_reopen_file(open_file->pfile, FALSE); + if (!rc) { + if (!open_file->closePend) + return open_file; + else { /* start over in case this was deleted */ + /* since the list could be modified */ read_lock(&GlobalSMBSeslock); - /* can not use this handle, no write - pending on this one after all */ atomic_dec(&open_file->wrtPending); - continue; + goto refind_writable; } } - if (open_file->closePend) { - read_lock(&GlobalSMBSeslock); - atomic_dec(&open_file->wrtPending); - continue; - } - return open_file; + + /* if it fails, try another handle if possible - + (we can not do this if closePending since + loop could be modified - in which case we + have to start at the beginning of the list + again. Note that it would be bad + to hold up writepages here (rather than + in caller) with continuous retries */ + cFYI(1, ("wp failed on reopen file")); + read_lock(&GlobalSMBSeslock); + /* can not use this handle, no write + pending on this one after all */ + atomic_dec(&open_file->wrtPending); + + if (open_file->closePend) /* list could have changed */ + goto refind_writable; + /* else we simply continue to the next entry. Thus + we do not loop on reopen errors. If we + can not reopen the file, for example if we + reconnected to a server with another client + racing to delete or lock the file we would not + make progress if we restarted before the beginning + of the loop here. */ } } read_unlock(&GlobalSMBSeslock); -- cgit v1.2.3 From 2c2130e16f0e134aa65515fd0c2436fda465b1b6 Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 12 Oct 2007 19:10:28 +0000 Subject: [CIFS] remove two sparse warnings Signed-off-by: Cyrill Gorcunov Signed-off-by: Steve French --- fs/cifs/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/cifs/file.c') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 780c0e3e470..1e7e4c06d9e 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1755,7 +1755,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping, struct page *page; struct cifs_sb_info *cifs_sb; struct cifsTconInfo *pTcon; - int bytes_read = 0; + unsigned int bytes_read = 0; unsigned int read_size, i; char *smb_read_data = NULL; struct smb_com_read_rsp *pSMBr; @@ -1849,7 +1849,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping, i += bytes_read >> PAGE_CACHE_SHIFT; cifs_stats_bytes_read(pTcon, bytes_read); - if ((int)(bytes_read & PAGE_CACHE_MASK) != bytes_read) { + if ((bytes_read & PAGE_CACHE_MASK) != bytes_read) { i++; /* account for partial page */ /* server copy of file can have smaller size -- cgit v1.2.3