Commit 14484915 authored by Andrew Morton's avatar Andrew Morton Committed by Greg Kroah-Hartman

[PATCH] ext2/3: incorrect increment of i_blocks when keeping the same xattr block

From: Andreas Gruenbacher <agruen@suse.de>

Here is a fix for extended attributes on ext2 and ext3, reported by
Stephen Tweedie <sct@redhat.com>.

From: Stephen Tweedie <sct@redhat.com>:

When you have an EA block that is shared between multiple inodes; AND you
then change an attribute in that on one inode, AND the new attribute value
is the same as the old, then xattr computes the new EA block, finds it
still in the cache, bumps the reference count on it (and the i_blocks field
on the inode, incidentally), and leaves it incremented because we haven't
changed EA block so there's no need to drop the refcount on the old block.

So *every* time you have more than one inode sharing an EA block and you
perform an identical write to an EA, you get a leak on both i_blocks and
the EA refcount.

This is a big problem for symlinks, which rely on correct i_blocks
accounting to determine the difference between fast and slow symlinks.
With the leak, you end up thinking that a fast symlink (ie.  one small
enough to be stored in the inode direct blocks) is slow, so you dereference
the ascii contents of the symlink as if they were a disk block address.
That typically results in EIO all over the place.
parent 389f24aa
...@@ -741,25 +741,24 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh, ...@@ -741,25 +741,24 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
if (header) { if (header) {
new_bh = ext2_xattr_cache_find(inode, header); new_bh = ext2_xattr_cache_find(inode, header);
if (new_bh) { if (new_bh) {
/* /* We found an identical block in the cache. */
* We found an identical block in the cache. The if (new_bh == old_bh) {
* block returned is locked. The old block will ea_bdebug(new_bh, "keeping this block");
* be released after updating the inode. } else {
*/ /* The old block is released after updating
ea_bdebug(new_bh, "%s block %lu", the inode. */
(old_bh == new_bh) ? "keeping" : "reusing", ea_bdebug(new_bh, "reusing block");
(unsigned long) new_bh->b_blocknr);
error = -EDQUOT; error = -EDQUOT;
if (DQUOT_ALLOC_BLOCK(inode, 1)) { if (DQUOT_ALLOC_BLOCK(inode, 1)) {
unlock_buffer(new_bh); unlock_buffer(new_bh);
goto cleanup; goto cleanup;
} }
HDR(new_bh)->h_refcount = cpu_to_le32(1 +
HDR(new_bh)->h_refcount = cpu_to_le32( le32_to_cpu(HDR(new_bh)->h_refcount));
le32_to_cpu(HDR(new_bh)->h_refcount) + 1);
ea_bdebug(new_bh, "refcount now=%d", ea_bdebug(new_bh, "refcount now=%d",
le32_to_cpu(HDR(new_bh)->h_refcount)); le32_to_cpu(HDR(new_bh)->h_refcount));
}
unlock_buffer(new_bh); unlock_buffer(new_bh);
} else if (old_bh && header == HDR(old_bh)) { } else if (old_bh && header == HDR(old_bh)) {
/* Keep this block. No need to lock the block as we /* Keep this block. No need to lock the block as we
......
...@@ -753,25 +753,26 @@ ext3_xattr_set_handle2(handle_t *handle, struct inode *inode, ...@@ -753,25 +753,26 @@ ext3_xattr_set_handle2(handle_t *handle, struct inode *inode,
if (header) { if (header) {
new_bh = ext3_xattr_cache_find(handle, inode, header, &credits); new_bh = ext3_xattr_cache_find(handle, inode, header, &credits);
if (new_bh) { if (new_bh) {
/* /* We found an identical block in the cache. */
* We found an identical block in the cache. The if (new_bh == old_bh)
* block returned is locked. The old block will ea_bdebug(new_bh, "keeping this block");
* be released after updating the inode. else {
*/ /* The old block is released after updating
ea_bdebug(new_bh, "%s block %lu", the inode. */
(old_bh == new_bh) ? "keeping" : "reusing", ea_bdebug(new_bh, "reusing block");
(unsigned long) new_bh->b_blocknr);
error = -EDQUOT; error = -EDQUOT;
if (DQUOT_ALLOC_BLOCK(inode, 1)) { if (DQUOT_ALLOC_BLOCK(inode, 1)) {
unlock_buffer(new_bh); unlock_buffer(new_bh);
journal_release_buffer(handle, new_bh, credits); journal_release_buffer(handle, new_bh,
credits);
goto cleanup; goto cleanup;
} }
HDR(new_bh)->h_refcount = cpu_to_le32( HDR(new_bh)->h_refcount = cpu_to_le32(1 +
le32_to_cpu(HDR(new_bh)->h_refcount) + 1); le32_to_cpu(HDR(new_bh)->h_refcount));
ea_bdebug(new_bh, "refcount now=%d", ea_bdebug(new_bh, "refcount now=%d",
le32_to_cpu(HDR(new_bh)->h_refcount)); le32_to_cpu(HDR(new_bh)->h_refcount));
}
unlock_buffer(new_bh); unlock_buffer(new_bh);
} else if (old_bh && header == HDR(old_bh)) { } else if (old_bh && header == HDR(old_bh)) {
/* Keep this block. No need to lock the block as we /* Keep this block. No need to lock the block as we
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment