[linux-cifs-client] [patch] prevent slab corruption by fixing race codition in cifs

[Shirish Pargaonkar]

Jeff,

Hope this works.

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6084d63..9ad3258 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -349,6 +349,7 @@ struct cifsFileInfo {
 	struct mutex lock_mutex;
 	struct list_head llist; /* list of byte range locks we have. */
 	bool closePend:1;	/* file is marked to close */
+	bool closed:1;		/* file is closed */
 	bool invalidHandle:1;	/* file closed via session abend */
 	bool messageMode:1;	/* for pipes: message vs byte mode */
 	atomic_t wrtPending;   /* handle in use - defer close */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c34b7f8..f1ae25c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -698,7 +698,10 @@ int cifs_close(struct inode *inode, struct file *file)
 			msleep(timeout);
 			timeout *= 8;
 		}
-		kfree(file->private_data);
+		if (atomic_read(&pSMBFile->wrtPending) == 0)
+			kfree(file->private_data);
+		else
+			pSMBFile->closed = true;
 		file->private_data = NULL;
 	} else
 		rc = -EBADF;
@@ -1293,7 +1296,10 @@ refind_writable:
 				else { /* start over in case this was deleted */
 				       /* since the list could be modified */
 					read_lock(&GlobalSMBSeslock);
-					atomic_dec(&open_file->wrtPending);
+					if (atomic_dec_and_test(
+							&open_file->wrtPending)
+							&& open_file->closed)
+						kfree(open_file);
 					goto refind_writable;
 				}
 			}
@@ -1309,10 +1315,12 @@ refind_writable:
 			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 */
+			if (atomic_dec_and_test(&open_file->wrtPending)
+					&& open_file->closed) {
+				kfree(open_file);
 				goto refind_writable;
+			} else if (open_file->closePend) /* list could */
+				goto refind_writable;	 /* have changed */
 			/* 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
@@ -1373,7 +1381,9 @@ static int cifs_partialpagewrite(struct page
*page, unsigned from, unsigned to)
 	if (open_file) {
 		bytes_written = cifs_write(open_file->pfile, write_data,
 					   to-from, &offset);
-		atomic_dec(&open_file->wrtPending);
+		if (atomic_dec_and_test(&open_file->wrtPending)
+				&& open_file->closed)
+			kfree(open_file);
 		/* Does mm or vfs already set times? */
 		inode->i_atime = inode->i_mtime = current_fs_time(inode->i_sb);
 		if ((bytes_written > 0) && (offset))
@@ -1562,7 +1572,9 @@ retry:
 						   bytes_to_write, offset,
 						   &bytes_written, iov, n_iov,
 						   long_op);
-				atomic_dec(&open_file->wrtPending);
+				if (atomic_dec_and_test(&open_file->wrtPending)
+						&& open_file->closed)
+					kfree(open_file);
 				cifs_update_eof(cifsi, offset, bytes_written);

 				if (rc || bytes_written < bytes_to_write) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 82d8383..3b4c27d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -799,8 +799,11 @@ set_via_filehandle:

 	if (open_file == NULL)
 		CIFSSMBClose(xid, pTcon, netfid);
-	else
-		atomic_dec(&open_file->wrtPending);
+	else {
+		if (atomic_dec_and_test(&open_file->wrtPending)
+					&& open_file->closed)
+			kfree(open_file);
+	}
 out:
 	return rc;
 }
@@ -1635,7 +1638,9 @@ cifs_set_file_size(struct inode *inode, struct
iattr *attrs,
 		__u32 npid = open_file->pid;
 		rc = CIFSSMBSetFileSize(xid, pTcon, attrs->ia_size, nfid,
 					npid, false);
-		atomic_dec(&open_file->wrtPending);
+		if (atomic_dec_and_test(&open_file->wrtPending)
+					&& open_file->closed)
+			kfree(open_file);
 		cFYI(1, ("SetFSize for attrs rc = %d", rc));
 		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
 			unsigned int bytes_written;
@@ -1790,7 +1795,9 @@ cifs_setattr_unix(struct dentry *direntry,
struct iattr *attrs)
 		u16 nfid = open_file->netfid;
 		u32 npid = open_file->pid;
 		rc = CIFSSMBUnixSetFileInfo(xid, pTcon, args, nfid, npid);
-		atomic_dec(&open_file->wrtPending);
+		if (atomic_dec_and_test(&open_file->wrtPending)
+				&& open_file->closed)
+			kfree(open_file);
 	} else {
 		rc = CIFSSMBUnixSetPathInfo(xid, pTcon, full_path, args,
 				    cifs_sb->local_nls,


signed-off by: Shirish Pargaonkar <shirishpargaonkar at gmail.com>


On Mon, Aug 17, 2009 at 2:50 PM, Jeff Layton<jlayton at redhat.com> wrote:
> On Sun, 16 Aug 2009 11:38:57 -0500
> Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
>
>> This patch prevents a slab corruption like this.  During heavy stress,
>> it is possible that
>> cifs_close will free up cifsFileInfo while due to delayed writes,
>> wrtPending of that
>> cifsFileInfo gets updated (decremented), cifsFileInfo either freed or
>> freed and allocated to another process.
>>
>>
>> Slab corruption: start=ffff8101e28e3818, len=256
>> Redzone: 0x5a2cf071/0x5a2cf071.
>> Last user: [<ffffffff88276ec4>](cifs_close+0x224/0x2c2 [cifs])
>> 060: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6a 6b 6b 6b
>> Prev obj: start=ffff8101e28e3700, len=256
>> Redzone: 0x170fc2a5/0x170fc2a5.
>> Last user: [<ffffffff882784fa>](cifs_open+0x348/0x6d9 [cifs])
>> 000: b8 d3 44 e2 01 81 ff ff a0 e2 c7 e1 01 81 ff ff
>> 010: 68 f5 fa dc 01 81 ff ff 10 47 d4 dd 01 81 ff ff
>> Next obj: start=ffff8101e28e3930, len=256
>> Redzone: 0x170fc2a5/0x170fc2a5.
>> Last user: [<ffffffff882784fa>](cifs_open+0x348/0x6d9 [cifs])
>> 000: 18 b8 2a e3 01 81 ff ff b8 a3 5a e0 01 81 ff ff
>> 010: 68 f5 36 65 02 81 ff ff 40 99 a1 dc 01 81 ff ff
>
> Can you resend this patch with it inlined into the email? I can't
> reasonably comment on it with it sent as a binary attachment.
>
> Thanks,
> --
> Jeff Layton <jlayton at redhat.com>
>