diff options
author | Andreas Gruenbacher <agruen@suse.de> | 2005-01-14 23:35:57 -0800 |
---|---|---|
committer | Linus Torvalds <torvalds@ppc970.osdl.org> | 2005-01-14 23:35:57 -0800 |
commit | fd1ea9abe35beaa2402b90b83a4c47135b086613 (patch) | |
tree | fae65165516b9faed78100cd3e9bb532a01c43d4 /fs/ext2/xattr.c | |
parent | 401494b1615efb1922a380226b0bea3eccb22ff3 (diff) | |
download | history-fd1ea9abe35beaa2402b90b83a4c47135b086613.tar.gz |
[PATCH] ext3/EA: Race in ext[23] xattr sharing code
Andrew Tridgell and Stephen C. Tweedie have reported two different Oopses
caused by a race condition in the mbcache, which is responsible for
extended attribute sharing in ext2 and ext3. Stephen tracked down the bug;
I did the fix.
Explanation:
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.
Fix this by converting the mbcache into a readers-writer style lock, and
protect all block accesses in ext2/ext3 by the mbcache entry lock. This
ensures that destroying blocks is an exclusive operation that may not
overlap xattr block reuse, while allowing multiple "re-users". Write
access to the xattr block's buffer is protected by the buffer lock.
Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Diffstat (limited to 'fs/ext2/xattr.c')
-rw-r--r-- | fs/ext2/xattr.c | 55 |
1 files changed, 31 insertions, 24 deletions
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c index d3cbbdd8b726b2..7560d9eec5c162 100644 --- a/fs/ext2/xattr.c +++ b/fs/ext2/xattr.c @@ -95,7 +95,6 @@ static int ext2_xattr_set2(struct inode *, struct buffer_head *, static int ext2_xattr_cache_insert(struct buffer_head *); static struct buffer_head *ext2_xattr_cache_find(struct inode *, struct ext2_xattr_header *); -static void ext2_xattr_cache_remove(struct buffer_head *); static void ext2_xattr_rehash(struct ext2_xattr_header *, struct ext2_xattr_entry *); @@ -494,15 +493,22 @@ bad_block: ext2_error(sb, "ext2_xattr_set", /* Here we know that we can set the new attribute. */ if (header) { + struct mb_cache_entry *ce; + /* assert(header == HDR(bh)); */ + ce = mb_cache_entry_get(ext2_xattr_cache, bh->b_bdev, + bh->b_blocknr); lock_buffer(bh); if (header->h_refcount == cpu_to_le32(1)) { ea_bdebug(bh, "modifying in-place"); - ext2_xattr_cache_remove(bh); + if (ce) + mb_cache_entry_free(ce); /* keep the buffer locked while modifying it. */ } else { int offset; + if (ce) + mb_cache_entry_release(ce); unlock_buffer(bh); ea_bdebug(bh, "cloning"); header = kmalloc(bh->b_size, GFP_KERNEL); @@ -707,13 +713,19 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh, error = 0; if (old_bh && old_bh != new_bh) { + struct mb_cache_entry *ce; + /* * If there was an old block and we are no longer using it, * release the old block. */ + ce = mb_cache_entry_get(ext2_xattr_cache, old_bh->b_bdev, + old_bh->b_blocknr); lock_buffer(old_bh); if (HDR(old_bh)->h_refcount == cpu_to_le32(1)) { /* Free the old block. */ + if (ce) + mb_cache_entry_free(ce); ea_bdebug(old_bh, "freeing"); ext2_free_blocks(inode, old_bh->b_blocknr, 1); /* We let our caller release old_bh, so we @@ -724,6 +736,8 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh, /* Decrement the refcount only. */ HDR(old_bh)->h_refcount = cpu_to_le32( le32_to_cpu(HDR(old_bh)->h_refcount) - 1); + if (ce) + mb_cache_entry_release(ce); DQUOT_FREE_BLOCK(inode, 1); mark_buffer_dirty(old_bh); ea_bdebug(old_bh, "refcount now=%d", @@ -748,6 +762,7 @@ void ext2_xattr_delete_inode(struct inode *inode) { struct buffer_head *bh = NULL; + struct mb_cache_entry *ce; down_write(&EXT2_I(inode)->xattr_sem); if (!EXT2_I(inode)->i_file_acl) @@ -767,15 +782,19 @@ ext2_xattr_delete_inode(struct inode *inode) EXT2_I(inode)->i_file_acl); goto cleanup; } + ce = mb_cache_entry_get(ext2_xattr_cache, bh->b_bdev, bh->b_blocknr); lock_buffer(bh); if (HDR(bh)->h_refcount == cpu_to_le32(1)) { - ext2_xattr_cache_remove(bh); + if (ce) + mb_cache_entry_free(ce); ext2_free_blocks(inode, EXT2_I(inode)->i_file_acl, 1); get_bh(bh); bforget(bh); } else { HDR(bh)->h_refcount = cpu_to_le32( le32_to_cpu(HDR(bh)->h_refcount) - 1); + if (ce) + mb_cache_entry_release(ce); mark_buffer_dirty(bh); if (IS_SYNC(inode)) sync_dirty_buffer(bh); @@ -892,11 +911,19 @@ ext2_xattr_cache_find(struct inode *inode, struct ext2_xattr_header *header) if (!header->h_hash) return NULL; /* never share */ ea_idebug(inode, "looking for cached blocks [%x]", (int)hash); +again: ce = mb_cache_entry_find_first(ext2_xattr_cache, 0, inode->i_sb->s_bdev, hash); while (ce) { - struct buffer_head *bh = sb_bread(inode->i_sb, ce->e_block); + struct buffer_head *bh; + if (IS_ERR(ce)) { + if (PTR_ERR(ce) == -EAGAIN) + goto again; + break; + } + + bh = sb_bread(inode->i_sb, ce->e_block); if (!bh) { ext2_error(inode->i_sb, "ext2_xattr_cache_find", "inode %ld: block %ld read error", @@ -923,26 +950,6 @@ ext2_xattr_cache_find(struct inode *inode, struct ext2_xattr_header *header) return NULL; } -/* - * ext2_xattr_cache_remove() - * - * Remove the cache entry of a block from the cache. Called when a - * block becomes invalid. - */ -static void -ext2_xattr_cache_remove(struct buffer_head *bh) -{ - struct mb_cache_entry *ce; - - ce = mb_cache_entry_get(ext2_xattr_cache, bh->b_bdev, bh->b_blocknr); - if (ce) { - ea_bdebug(bh, "removing (%d cache entries remaining)", - atomic_read(&ext2_xattr_cache->c_entry_count)-1); - mb_cache_entry_free(ce); - } else - ea_bdebug(bh, "no cache entry"); -} - #define NAME_HASH_SHIFT 5 #define VALUE_HASH_SHIFT 16 |