Attachment 28134 Details for Bug 65424

[patch] Proposed fix

xattr-race-minimal-fix.diff (text/plain), 7.10 KB, created by Andreas Gruenbacher on 2005-02-02 21:44:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andreas Gruenbacher

Created: 2005-02-02 21:44:05 UTC

Size: 7.10 KB

patch

obsolete

>From: Andreas Gruenbacher <agruen@suse.de>
>Subject: Minimal fix for mbcache race in ext2 and ext3
>
>The mbcache caches the locations and content hashes of xattr blocks.
>There are two access strategies: [1] xattr block disposal via
>mb_cache_entry_get(), [2] xattr block reuse (sharing) via
>mb_cache_entry_find_{first,next}().  There is no locking between the two
>methods, so between one mb_cache_entry_find_x and the next, a
>mb_cache_entry_get might come in, unhash the cache entry, and change the
>journaling state of the xattr buffer.  Subsequently, two things can
>happen: [a] the next mb_cache_entry_find_x may try to follow the mbcache
>hash chain starting from the entry that has become unhashed, which now
>is a stale pointer, [b] the block may have become deallocated, and then
>we try to reuse it.
>
>With default acls, a third case has been observed: mb_cache_entry_find_x
>returns a block that an inode reuses, at the same time the block gets
>deallocated and immediately re-allocated, and the inode ends up with
>changed attributes.
>
>The "real" fix for this bug is to convert the mbcache into a cache with
>readers/writer style locking semantics, and to use those new mbcache
>locking guarantees to protect from the mb_cache_entry_find_x race. This
>is relatively intrusive, so we take a minimal approach, and use a global
>lock instead.
>
>Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
>
>Index: linux-2.4.21/fs/ext3/xattr.c
>===================================================================
>--- linux-2.4.21.orig/fs/ext3/xattr.c
>+++ linux-2.4.21/fs/ext3/xattr.c
>@@ -43,12 +43,10 @@
>  *
>  * Locking strategy
>  * ----------------
>- * EXT3_I(inode)->i_file_acl is protected by EXT3_I(inode)->xattr_sem.
>- * EA blocks are only changed if they are exclusive to an inode, so
>- * holding xattr_sem also means that nothing but the EA block's reference
>- * count will change. Multiple writers to an EA block are synchronized
>- * by the bh lock. No more than a single bh lock is held at any time,
>- * which avoids deadlocks.
>+ * As a minimal fix of a race with the mbcache, we are using a global
>+ * readers/writer semaphore (http://linux.bkbits.net:8080/linux-2.6/
>+ * cset@41e9a8dde680BhQtidQ5PNNkGXHKMQ?nav=index.html|ChangeSet@-8w
>+ * explains the problem).
>  */
> 
> #include <linux/fs.h>
>@@ -84,6 +82,8 @@
> # define ea_bdebug(f...)
> #endif
> 
>+static DECLARE_RWSEM(ext3_xattr_sem);
>+
> static int ext3_xattr_set_handle2(handle_t *, struct inode *,
> 				  struct buffer_head *,
> 				  struct ext3_xattr_header *);
>@@ -280,7 +280,7 @@ ext3_xattr_get(struct inode *inode, int 
> 
> 	if (name == NULL)
> 		return -EINVAL;
>-	down_read(&EXT3_I(inode)->xattr_sem);
>+	down_read(&ext3_xattr_sem);
> 	error = -ENODATA;
> 	if (!EXT3_I(inode)->i_file_acl)
> 		goto cleanup;
>@@ -353,7 +353,7 @@ found:
> 
> cleanup:
> 	brelse(bh);
>-	up_read(&EXT3_I(inode)->xattr_sem);
>+	up_read(&ext3_xattr_sem);
> 
> 	return error;
> }
>@@ -380,7 +380,7 @@ ext3_xattr_list(struct inode *inode, cha
> 	ea_idebug(inode, "buffer=%p, buffer_size=%ld",
> 		  buffer, (long)buffer_size);
> 
>-	down_read(&EXT3_I(inode)->xattr_sem);
>+	down_read(&ext3_xattr_sem);
> 	error = 0;
> 	if (!EXT3_I(inode)->i_file_acl)
> 		goto cleanup;
>@@ -441,7 +441,7 @@ bad_block:	ext3_error(inode->i_sb, "ext3
> 
> cleanup:
> 	brelse(bh);
>-	up_read(&EXT3_I(inode)->xattr_sem);
>+	up_read(&ext3_xattr_sem);
> 
> 	return error;
> }
>@@ -517,7 +517,7 @@ ext3_xattr_set_handle(handle_t *handle, 
> 	name_len = strlen(name);
> 	if (name_len > 255 || value_len > sb->s_blocksize)
> 		return -ERANGE;
>-	down_write(&EXT3_I(inode)->xattr_sem);
>+	down_write(&ext3_xattr_sem);
> 	if (EXT3_I(inode)->i_file_acl) {
> 		/* The inode already has an extended attribute block. */
> 		bh = sb_bread(sb, EXT3_I(inode)->i_file_acl);
>@@ -740,7 +740,7 @@ cleanup:
> 	brelse(bh);
> 	if (!(bh && header == HDR(bh)))
> 		kfree(header);
>-	up_write(&EXT3_I(inode)->xattr_sem);
>+	up_write(&ext3_xattr_sem);
> 
> 	return error;
> }
>@@ -911,7 +911,7 @@ ext3_xattr_delete_inode(handle_t *handle
> {
> 	struct buffer_head *bh = NULL;
> 
>-	down_write(&EXT3_I(inode)->xattr_sem);
>+	down_write(&ext3_xattr_sem);
> 	if (!EXT3_I(inode)->i_file_acl)
> 		goto cleanup;
> 	bh = sb_bread(inode->i_sb, EXT3_I(inode)->i_file_acl);
>@@ -954,7 +954,7 @@ ext3_xattr_delete_inode(handle_t *handle
> 
> cleanup:
> 	brelse(bh);
>-	up_write(&EXT3_I(inode)->xattr_sem);
>+	up_write(&ext3_xattr_sem);
> }
> 
> /*
>Index: linux-2.4.21/fs/ext2/xattr.c
>===================================================================
>--- linux-2.4.21.orig/fs/ext2/xattr.c
>+++ linux-2.4.21/fs/ext2/xattr.c
>@@ -42,12 +42,10 @@
>  *
>  * Locking strategy
>  * ----------------
>- * EXT2_I(inode)->i_file_acl is protected by EXT2_I(inode)->xattr_sem.
>- * EA blocks are only changed if they are exclusive to an inode, so
>- * holding xattr_sem also means that nothing but the EA block's reference
>- * count will change. Multiple writers to an EA block are synchronized
>- * by the bh lock. No more than a single bh lock is held at any time,
>- * which avoids deadlocks.
>+ * As a minimal fix of a race with the mbcache, we are using a global
>+ * readers/writer semaphore (http://linux.bkbits.net:8080/linux-2.6/
>+ * cset@41e9a8dde680BhQtidQ5PNNkGXHKMQ?nav=index.html|ChangeSet@-8w
>+ * explains the problem).
>  */
> 
> #include <linux/module.h>
>@@ -90,6 +88,8 @@ EXPORT_SYMBOL(ext2_xattr_set);
> # define ea_bdebug(f...)
> #endif
> 
>+static DECLARE_RWSEM(ext2_xattr_sem);
>+
> static int ext2_xattr_set2(struct inode *, struct buffer_head *,
> 			   struct ext2_xattr_header *);
> 
>@@ -285,7 +285,7 @@ ext2_xattr_get(struct inode *inode, int 
> 
> 	if (name == NULL)
> 		return -EINVAL;
>-	down_read(&EXT2_I(inode)->xattr_sem);
>+	down_read(&ext2_xattr_sem);
> 	error = -ENODATA;
> 	if (!EXT2_I(inode)->i_file_acl)
> 		goto cleanup;
>@@ -358,7 +358,7 @@ found:
> 
> cleanup:
> 	brelse(bh);
>-	up_read(&EXT2_I(inode)->xattr_sem);
>+	up_read(&ext2_xattr_sem);
> 
> 	return error;
> }
>@@ -385,7 +385,7 @@ ext2_xattr_list(struct inode *inode, cha
> 	ea_idebug(inode, "buffer=%p, buffer_size=%ld",
> 		  buffer, (long)buffer_size);
> 
>-	down_read(&EXT2_I(inode)->xattr_sem);
>+	down_read(&ext2_xattr_sem);
> 	error = 0;
> 	if (!EXT2_I(inode)->i_file_acl)
> 		goto cleanup;
>@@ -446,7 +446,7 @@ bad_block:	ext2_error(inode->i_sb, "ext2
> 
> cleanup:
> 	brelse(bh);
>-	up_read(&EXT2_I(inode)->xattr_sem);
>+	up_read(&ext2_xattr_sem);
> 
> 	return error;
> }
>@@ -518,7 +518,7 @@ ext2_xattr_set(struct inode *inode, int 
> 	name_len = strlen(name);
> 	if (name_len > 255 || value_len > sb->s_blocksize)
> 		return -ERANGE;
>-	down_write(&EXT2_I(inode)->xattr_sem);
>+	down_write(&ext2_xattr_sem);
> 	if (EXT2_I(inode)->i_file_acl) {
> 		/* The inode already has an extended attribute block. */
> 
>@@ -733,7 +733,7 @@ cleanup:
> 	brelse(bh);
> 	if (!(bh && header == HDR(bh)))
> 		kfree(header);
>-	up_write(&EXT2_I(inode)->xattr_sem);
>+	up_write(&ext2_xattr_sem);
> 
> 	return error;
> }
>@@ -866,7 +866,7 @@ ext2_xattr_delete_inode(struct inode *in
> {
> 	struct buffer_head *bh = NULL;
> 
>-	down_write(&EXT2_I(inode)->xattr_sem);
>+	down_write(&ext2_xattr_sem);
> 	if (!EXT2_I(inode)->i_file_acl)
> 		goto cleanup;
> 	bh = sb_bread(inode->i_sb, EXT2_I(inode)->i_file_acl);
>@@ -905,7 +905,7 @@ ext2_xattr_delete_inode(struct inode *in
> 
> cleanup:
> 	brelse(bh);
>-	up_write(&EXT2_I(inode)->xattr_sem);
>+	up_write(&ext2_xattr_sem);
> }
> 
> /*

Actions: View | Diff

Attachments on bug 65424: 28134 | 28230