lkml.org 
[lkml]   [2005]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[RESEND][PATCH 4/9] Ext3: extended attribute sharing fixes and in-inode EAs
Ext3: do not use journal_release_buffer

The use of journal_release_buffer is unsafe; it can overflow the
journal: When a buffer is stolen from a transaction and later removed
from that transaction with journal_release_buffer, the buffer is not
accounted to the transaction that now "owns" the buffer, and one extra
credit appears to be available. Don't use journal_release_buffer:

We did rely on the buffer lock to synchronize xattr block accesses, and
get write access to the buffer first to get atomicity. Return the
mb_cache_entry from ext3_xattr_cache_find instead, and do the
check/update under its lock. Only get write access when we know we will
use the buffer.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.10/fs/ext3/xattr.c
===================================================================
--- linux-2.6.10.orig/fs/ext3/xattr.c
+++ linux-2.6.10/fs/ext3/xattr.c
@@ -94,9 +94,9 @@ static int ext3_xattr_set_handle2(handle
struct ext3_xattr_header *);

static int ext3_xattr_cache_insert(struct buffer_head *);
-static struct buffer_head *ext3_xattr_cache_find(handle_t *, struct inode *,
+static struct buffer_head *ext3_xattr_cache_find(struct inode *,
struct ext3_xattr_header *,
- int *);
+ struct mb_cache_entry **);
static void ext3_xattr_rehash(struct ext3_xattr_header *,
struct ext3_xattr_entry *);

@@ -500,33 +500,24 @@ bad_block: ext3_error(sb, "ext3_xattr_s

if (header) {
struct mb_cache_entry *ce;
- int credits = 0;

/* assert(header == HDR(bh)); */
- if (header->h_refcount != cpu_to_le32(1))
- goto skip_get_write_access;
- /* ext3_journal_get_write_access() requires an unlocked bh,
- which complicates things here. */
- error = ext3_journal_get_write_access_credits(handle, bh,
- &credits);
- if (error)
- goto cleanup;
ce = mb_cache_entry_get(ext3_xattr_cache, bh->b_bdev,
bh->b_blocknr);
- lock_buffer(bh);
if (header->h_refcount == cpu_to_le32(1)) {
if (ce)
mb_cache_entry_free(ce);
ea_bdebug(bh, "modifying in-place");
+ error = ext3_journal_get_write_access(handle, bh);
+ if (error)
+ goto cleanup;
+ lock_buffer(bh);
/* keep the buffer locked while modifying it. */
} else {
int offset;

if (ce)
mb_cache_entry_release(ce);
- unlock_buffer(bh);
- journal_release_buffer(handle, bh, credits);
- skip_get_write_access:
ea_bdebug(bh, "cloning");
header = kmalloc(bh->b_size, GFP_KERNEL);
error = -ENOMEM;
@@ -622,17 +613,18 @@ bad_block: ext3_error(sb, "ext3_xattr_s
}

skip_replace:
- if (IS_LAST_ENTRY(ENTRY(header+1))) {
- /* This block is now empty. */
- if (bh && header == HDR(bh))
- unlock_buffer(bh); /* we were modifying in-place. */
- error = ext3_xattr_set_handle2(handle, inode, bh, NULL);
- } else {
+ if (!IS_LAST_ENTRY(ENTRY(header+1)))
ext3_xattr_rehash(header, here);
- if (bh && header == HDR(bh))
- unlock_buffer(bh); /* we were modifying in-place. */
- error = ext3_xattr_set_handle2(handle, inode, bh, header);
+ if (bh && header == HDR(bh)) {
+ /* we were modifying in-place. */
+ unlock_buffer(bh);
+ error = ext3_journal_dirty_metadata(handle, bh);
+ if (error)
+ goto cleanup;
}
+ error = ext3_xattr_set_handle2(handle, inode, bh,
+ IS_LAST_ENTRY(ENTRY(header+1)) ?
+ NULL : header);

cleanup:
brelse(bh);
@@ -653,10 +645,11 @@ ext3_xattr_set_handle2(handle_t *handle,
{
struct super_block *sb = inode->i_sb;
struct buffer_head *new_bh = NULL;
- int credits = 0, error;
+ struct mb_cache_entry *ce = NULL;
+ int error;

if (header) {
- new_bh = ext3_xattr_cache_find(handle, inode, header, &credits);
+ new_bh = ext3_xattr_cache_find(inode, header, &ce);
if (new_bh) {
/* We found an identical block in the cache. */
if (new_bh == old_bh)
@@ -667,19 +660,26 @@ ext3_xattr_set_handle2(handle_t *handle,
ea_bdebug(new_bh, "reusing block");

error = -EDQUOT;
- if (DQUOT_ALLOC_BLOCK(inode, 1)) {
- unlock_buffer(new_bh);
- journal_release_buffer(handle, new_bh,
- credits);
+ if (DQUOT_ALLOC_BLOCK(inode, 1))
goto cleanup;
- }
+ error = ext3_journal_get_write_access(handle, new_bh);
+ if (error)
+ goto cleanup;
+ lock_buffer(new_bh);
HDR(new_bh)->h_refcount = cpu_to_le32(1 +
le32_to_cpu(HDR(new_bh)->h_refcount));
ea_bdebug(new_bh, "refcount now=%d",
le32_to_cpu(HDR(new_bh)->h_refcount));
+ unlock_buffer(new_bh);
+ error = ext3_journal_dirty_metadata(handle, new_bh);
+ if (error)
+ goto cleanup;
}
- unlock_buffer(new_bh);
+ mb_cache_entry_release(ce);
+ ce = NULL;
} else if (old_bh && header == HDR(old_bh)) {
+ /* We were modifying this block in-place. */
+
/* Keep this block. No need to lock the block as we
* don't need to change the reference count. */
new_bh = old_bh;
@@ -715,10 +715,10 @@ getblk_failed:
ext3_xattr_cache_insert(new_bh);

ext3_xattr_update_super_block(handle, sb);
+ error = ext3_journal_dirty_metadata(handle, new_bh);
+ if (error)
+ goto cleanup;
}
- error = ext3_journal_dirty_metadata(handle, new_bh);
- if (error)
- goto cleanup;
}

/* Update the inode. */
@@ -730,22 +730,14 @@ getblk_failed:

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.
*/
- error = ext3_journal_get_write_access(handle, old_bh);
- if (error)
- goto cleanup;
ce = mb_cache_entry_get(ext3_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");
ext3_free_blocks(handle, inode, old_bh->b_blocknr, 1);

@@ -754,21 +746,29 @@ getblk_failed:
duplicate the handle before. */
get_bh(old_bh);
ext3_forget(handle, 1, inode, old_bh,old_bh->b_blocknr);
+ if (ce) {
+ mb_cache_entry_free(ce);
+ ce = NULL;
+ }
} else {
+ error = ext3_journal_get_write_access(handle, old_bh);
+ if (error)
+ goto cleanup;
/* Decrement the refcount only. */
+ lock_buffer(old_bh);
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);
ext3_journal_dirty_metadata(handle, old_bh);
ea_bdebug(old_bh, "refcount now=%d",
le32_to_cpu(HDR(old_bh)->h_refcount));
+ unlock_buffer(old_bh);
}
- unlock_buffer(old_bh);
}

cleanup:
+ if (ce)
+ mb_cache_entry_release(ce);
brelse(new_bh);

return error;
@@ -819,7 +819,7 @@ void
ext3_xattr_delete_inode(handle_t *handle, struct inode *inode)
{
struct buffer_head *bh = NULL;
- struct mb_cache_entry *ce;
+ struct mb_cache_entry *ce = NULL;

down_write(&EXT3_I(inode)->xattr_sem);
if (!EXT3_I(inode)->i_file_acl)
@@ -838,31 +838,33 @@ ext3_xattr_delete_inode(handle_t *handle
EXT3_I(inode)->i_file_acl);
goto cleanup;
}
- if (ext3_journal_get_write_access(handle, bh) != 0)
- goto cleanup;
ce = mb_cache_entry_get(ext3_xattr_cache, bh->b_bdev, bh->b_blocknr);
- lock_buffer(bh);
if (HDR(bh)->h_refcount == cpu_to_le32(1)) {
- if (ce)
+ if (ce) {
mb_cache_entry_free(ce);
+ ce = NULL;
+ }
ext3_free_blocks(handle, inode, EXT3_I(inode)->i_file_acl, 1);
get_bh(bh);
ext3_forget(handle, 1, inode, bh, EXT3_I(inode)->i_file_acl);
} else {
+ if (ext3_journal_get_write_access(handle, bh) != 0)
+ goto cleanup;
+ lock_buffer(bh);
HDR(bh)->h_refcount = cpu_to_le32(
le32_to_cpu(HDR(bh)->h_refcount) - 1);
- if (ce)
- mb_cache_entry_release(ce);
ext3_journal_dirty_metadata(handle, bh);
if (IS_SYNC(inode))
handle->h_sync = 1;
DQUOT_FREE_BLOCK(inode, 1);
+ unlock_buffer(bh);
}
ea_bdebug(bh, "refcount now=%d", le32_to_cpu(HDR(bh)->h_refcount) - 1);
- unlock_buffer(bh);
EXT3_I(inode)->i_file_acl = 0;

cleanup:
+ if (ce)
+ mb_cache_entry_release(ce);
brelse(bh);
up_write(&EXT3_I(inode)->xattr_sem);
}
@@ -960,8 +962,8 @@ ext3_xattr_cmp(struct ext3_xattr_header
* not found or an error occurred.
*/
static struct buffer_head *
-ext3_xattr_cache_find(handle_t *handle, struct inode *inode,
- struct ext3_xattr_header *header, int *credits)
+ext3_xattr_cache_find(struct inode *inode, struct ext3_xattr_header *header,
+ struct mb_cache_entry **pce)
{
__u32 hash = le32_to_cpu(header->h_hash);
struct mb_cache_entry *ce;
@@ -985,27 +987,17 @@ again:
ext3_error(inode->i_sb, "ext3_xattr_cache_find",
"inode %ld: block %ld read error",
inode->i_ino, (unsigned long) ce->e_block);
- } else if (ext3_journal_get_write_access_credits(
- handle, bh, credits) == 0) {
- /* ext3_journal_get_write_access() requires an unlocked
- * bh, which complicates things here. */
- lock_buffer(bh);
- if (le32_to_cpu(HDR(bh)->h_refcount) >
- EXT3_XATTR_REFCOUNT_MAX) {
- ea_idebug(inode, "block %ld refcount %d>%d",
- (unsigned long) ce->e_block,
- le32_to_cpu(HDR(bh)->h_refcount),
+ } else if (le32_to_cpu(HDR(bh)->h_refcount) >
+ EXT3_XATTR_REFCOUNT_MAX) {
+ ea_idebug(inode, "block %ld refcount %d>%d",
+ (unsigned long) ce->e_block,
+ le32_to_cpu(HDR(bh)->h_refcount),
EXT3_XATTR_REFCOUNT_MAX);
- } else if (!ext3_xattr_cmp(header, HDR(bh))) {
- mb_cache_entry_release(ce);
- /* buffer will be unlocked by caller */
- return bh;
- }
- unlock_buffer(bh);
- journal_release_buffer(handle, bh, *credits);
- *credits = 0;
- brelse(bh);
+ } else if (ext3_xattr_cmp(header, HDR(bh)) == 0) {
+ *pce = ce;
+ return bh;
}
+ brelse(bh);
ce = mb_cache_entry_find_next(ce, 0, inode->i_sb->s_bdev, hash);
}
return NULL;
--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:09    [from the cache]
©2003-2011 Jasper Spaans