linux-ext4 - Re: [resend PATCH] ext4: avoid deadlock on sync-mounted ext2 FS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F359D85.9070008@redhat.com>
Date:	Fri, 10 Feb 2012 16:43:17 -0600
From:	Eric Sandeen <esandeen@...hat.com>
To:	Martin Wilck <martin.wilck@...fujitsu.com>
CC:	linux-ext4@...r.kernel.org, tytso@....edu,
	adilger.kernel@...ger.ca, fmayhar@...gle.com
Subject: Re: [resend PATCH] ext4: avoid deadlock on sync-mounted ext2 FS

On 02/10/2012 04:31 AM, Martin Wilck wrote:
> I don't want to bother, but could someone have a look at this
> patch please?

You have to bother, to get attention ;)

> Martin
> 
> Processes hang forever on a sync-mounted ext2 file system that
> is mounted with the ext4 module (default in Fedora 16).
> 
> I can reproduce this reliably by mounting an ext2 partition with
> "-o sync" and opening a new file an that partition with vim. vim
> will hang in "D" state forever.
> 
> I am attaching a small patch here that solves this issue for me.
> In the sync mounted case, ext4_handle_dirty_metadata() may call
> sync_dirty_buffer(), which can't be called with buffer lock held.
> 
> My knowledge of locking in ext4 is insufficient to judge if this
> patch is correct, but it may serve as starting point for others.

This looks an awful lot like:

b2f49033d80c952a0ffc2d5647bc1a0b8a09c1b3 [PATCH] fix deadlock in ext2

from 2006!  :)

But I'm also eyeballing:

8a2bfdcbfa441d8b0e5cb9c9a7f45f77f80da465 [PATCH] ext[34]: EA block
reference count racing fix

to be sure it doesn't regress anything there.

And I'd really like to know why this only happens on ext2-with-ext4
(ext4 nojournal seems ok... hm, actually, I can't repro it on
ext2 either...)

Ahah.  Ok, makes sense, it has to be mkfs'd with a 128-byte inode, so
that selinux xattrs will go into an external block.  Now I can reproduce
it on ext2.  But still not on ext4, with 128-byte-inodes
and nojournal.  Odd.  I'd like to sort that out.

The patch also unlocks prior to mb_cache_entry_release(), and I think
that might put some of the raciness back?

-Eric


> Signed-off-by: Martin.Wilck@...fujitsu.com
> ---
>  fs/ext4/xattr.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 93a00d8..eb771ea 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -487,8 +487,10 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
>  		ext4_free_blocks(handle, inode, bh, 0, 1,
>  				 EXT4_FREE_BLOCKS_METADATA |
>  				 EXT4_FREE_BLOCKS_FORGET);
> +		unlock_buffer(bh);
>  	} else {
>  		le32_add_cpu(&BHDR(bh)->h_refcount, -1);
> +		unlock_buffer(bh);
>  		error = ext4_handle_dirty_metadata(handle, inode, bh);
>  		if (IS_SYNC(inode))
>  			ext4_handle_sync(handle);
> @@ -498,7 +500,6 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
>  		if (ce)
>  			mb_cache_entry_release(ce);
>  	}
> -	unlock_buffer(bh);
>  out:
>  	ext4_std_error(inode->i_sb, error);
>  	return;

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html