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    [W:0.037 / U:58.392 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site