linux-ext4 - Re: [PATCH] ext4: allow inode expansion for nojournal file systems

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: <AAF1FFC4-7D4B-4E62-89D2-8C7978D16C35@dilger.ca>
Date:   Tue, 25 Oct 2016 15:54:16 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Whitney <enwlinux@...il.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] ext4: allow inode expansion for nojournal file systems

On Oct 25, 2016, at 3:14 PM, Eric Whitney <enwlinux@...il.com> wrote:
> 
> Runs of xfstest ext4/022 on nojournal file systems result in failures
> because the inodes of some of its test files do not expand as expected.
> The cause is a conditional in ext4_mark_inode_dirty() that prevents inode
> expansion unless the test file system has a journal.  Remove this
> unnecessary restriction.

I think the reason we required that the filesystem be journaled to expand
the inode reserved space is to ensure that the update was an all-or-nothing
approach.  If there are in-inode xattrs that need to be moved to an external
xattr block, and that block needs allocation and such, it is possible that
a nojournal filesystem could result in the xattrs being moved and the inode
is written (w/o the xattrs), but the xattr block is not written to disk
before a crash.

That said, this check could potentially be moved later in the xattr handling,
since most xattrs will stay within the inode (when growing only a few bytes),
which is always safe.  If moving xattrs to an external block, we could sync
write the xattr block before marking the inode dirty to ensure that the xattr
is not lost in a crash (though it may be duplicated, e2fsck should handle
that?).  However, that has a potentially large performance cost.

Cheers, Andreas

> 
> Signed-off-by: Eric Whitney <enwlinux@...il.com>
> ---
> fs/ext4/inode.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9c06472..260da4d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5455,18 +5455,20 @@ int ext4_mark_inode_dirty(handle_t *handle, struct inode *inode)
> 	err = ext4_reserve_inode_write(handle, inode, &iloc);
> 	if (err)
> 		return err;
> -	if (ext4_handle_valid(handle) &&
> -	    EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize &&
> +	if (EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize &&
> 	    !ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) {
> 		/*
> -		 * We need extra buffer credits since we may write into EA block
> +		 * In nojournal mode, we can immediately attempt to expand
> +		 * the inode.  When journaled, we first need to obtain extra
> +		 * buffer credits since we may write into the EA block
> 		 * with this same handle. If journal_extend fails, then it will
> 		 * only result in a minor loss of functionality for that inode.
> 		 * If this is felt to be critical, then e2fsck should be run to
> 		 * force a large enough s_min_extra_isize.
> 		 */
> -		if ((jbd2_journal_extend(handle,
> -			     EXT4_DATA_TRANS_BLOCKS(inode->i_sb))) == 0) {
> +		if (!ext4_handle_valid(handle) ||
> +		    jbd2_journal_extend(handle,
> +			     EXT4_DATA_TRANS_BLOCKS(inode->i_sb)) == 0) {
> 			ret = ext4_expand_extra_isize(inode,
> 						      sbi->s_want_extra_isize,
> 						      iloc, handle);
> --
> 2.1.4
> 
> --
> 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


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)