Date:   Tue, 23 May 2017 10:19:35 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] libext2fs: correctly subtract xattr blocks on bigalloc
 filesystems

On May 21, 2017, at 12:23 AM, Eric Biggers <ebiggers3@...il.com> wrote:
> 
> From: Eric Biggers <ebiggers@...gle.com>
> 
> ext2fs_inode_data_blocks2() calculates an inode's data block count by
> subtracting the external xattr block, if any, from the total blocks.
> But on bigalloc filesystems, the xattr "block" is actually a whole
> cluster, so ext2fs_inode_data_blocks2() would return a too-large value.
> 
> It seems this could have caused several different problems, but the one
> I encountered was that xfstest generic/399 failed in the "bigalloc"
> config because e2fsck incorrectly considered a symlink on the filesystem
> to be corrupted at the end of the test.  This happened because e2fsck
> incorrectly calculated a nonzero data block count for a "fast" symlink
> with an external xattr block and therefore treated it as a "slow"
> symlink, which failed validation.

I thought we changed this to detect "fast" inodes by i_size < 60 rather
than using the blocks count, because the blocks count was (and apparently
continues to be) unreliable for determining fast vs. slow symlinks.

However, ext4_inode_is_fast_symlink() still checks blocks count.  In
"[PATCH] ext4: fix reading new encrypted symlinks on no-journal filesystems"
we discussed whether this was safe, and it appears to be OK from my
analysis.

We just continue to hit problems when extrapolating various blocks counts
to detect fast symlinks rather than just using the same mechanism we use
at creation time, which is "len > EXT4_N_BLOCKS * 4".

Cheers, Andreas

> Fix this by updating ext2fs_inode_data_blocks2() to subtract the cluster
> size rather than the block size.
> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> lib/ext2fs/blknum.c                          |   2 +-
> tests/f_bigalloc_symlink_with_xattr/expect.1 |   7 +++++++
> tests/f_bigalloc_symlink_with_xattr/image.gz | Bin 0 -> 663 bytes
> tests/f_bigalloc_symlink_with_xattr/name     |   1 +
> tests/f_bigalloc_symlink_with_xattr/script   |   2 ++
> 5 files changed, 11 insertions(+), 1 deletion(-)
> create mode 100644 tests/f_bigalloc_symlink_with_xattr/expect.1
> create mode 100644 tests/f_bigalloc_symlink_with_xattr/image.gz
> create mode 100644 tests/f_bigalloc_symlink_with_xattr/name
> create mode 100644 tests/f_bigalloc_symlink_with_xattr/script
> 
> diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c
> index ac808490..aa0e9118 100644
> --- a/lib/ext2fs/blknum.c
> +++ b/lib/ext2fs/blknum.c
> @@ -71,7 +71,7 @@ blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs,
> 	return (inode->i_blocks |
> 		(ext2fs_has_feature_huge_file(fs->super) ?
> 		 (__u64) inode->osd2.linux2.l_i_blocks_hi << 32 : 0)) -
> -		(inode->i_file_acl ? fs->blocksize >> 9 : 0);
> +		(inode->i_file_acl ? EXT2_CLUSTER_SIZE(fs->super) >> 9 : 0);
> }
> 
> /*
> diff --git a/tests/f_bigalloc_symlink_with_xattr/expect.1 b/tests/f_bigalloc_symlink_with_xattr/expect.1
> new file mode 100644
> index 00000000..0e6c199f
> --- /dev/null
> +++ b/tests/f_bigalloc_symlink_with_xattr/expect.1
> @@ -0,0 +1,7 @@
> +Pass 1: Checking inodes, blocks, and sizes
> +Pass 2: Checking directory structure
> +Pass 3: Checking directory connectivity
> +Pass 4: Checking reference counts
> +Pass 5: Checking group summary information
> +test_filesys: 12/16 files (0.0% non-contiguous), 112/128 blocks
> +Exit status is 0
> diff --git a/tests/f_bigalloc_symlink_with_xattr/image.gz b/tests/f_bigalloc_symlink_with_xattr/image.gz
> new file mode 100644
> index 0000000000000000000000000000000000000000..cbd125dc25a59e6795363e4228c5c5a24e49eeb2
> GIT binary patch
> literal 663
> zcmb2|=3oE;CgvoGjR_(>O@...M|d61u%&K3pz<I`jg5`9<4$npQ4fK%lpp`?f7mKB
> zD~Iqbn<wa1?0j|$!y{*hw>KRQpDqo#XVdZZrqT3=(Xr31@...J44-P=E@...bLINi
> z`(}08^*)m)9&Bu6S7H3j&dZ=IW+=8GL1aN~b;H5eeTx~Y<kOUA<lerQq%$w~c3IAu
> zZ(`XRO;L_#7i@...IU4x=bU@>KfIF<y0Y7T37Pt2gNnMUX3+98T$AnOzGTkTZc7nn
> zEBXER4ukI7yGI?CbAdesCoY(X$#0Q(5x@...W)TuYkBOqqxR~AyH7qDp6rpHbVYi1
> z-PFgM&)n0T^u~I<$6V7cmFMZlV@...UTt2Pxx!d{*}`S-rnl|-eD=xS?~~`mUzh$@
> z@...1Z>Q<(@~rbFR|?ISEj(80?QCnT@...&(r5E)|E8?IJwIp3-<$R)zuq(~=kXS+
> zpZ{iNO7Z{riBFdPj<5LgaQdcLvH5?Gp7_0KVe~ogZX_2j@Yz(~_36&l&#9H~cAl*B
> z<U7CbY4G`dKQ2Y~+x-4y%&&WU+Lv|TZoNKweSY1mkLmM#vYEd5A1jmmeDOZIM~~IL
> zzWU#PUD%cX&eg&H?LX~U7q5QukJg36C;yih&##I3=lGBDC;Jom3H1&L#zNMU_DIfw
> zF*it03GP;Lo%Fv--g3+7H}%U?bbnvFyjsOF=jpfW@...J6@...?B8;Ez6(xqRS%zv
> l$5;JWSR9`6W9skuRgoQNE`u2gr4w?!8Fp-~GX^F`1^_GgBJThI
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/f_bigalloc_symlink_with_xattr/name b/tests/f_bigalloc_symlink_with_xattr/name
> new file mode 100644
> index 00000000..f612796f
> --- /dev/null
> +++ b/tests/f_bigalloc_symlink_with_xattr/name
> @@ -0,0 +1 @@
> +fast symlink with external xattr block on bigalloc filesystem
> diff --git a/tests/f_bigalloc_symlink_with_xattr/script b/tests/f_bigalloc_symlink_with_xattr/script
> new file mode 100644
> index 00000000..8ab2b9c6
> --- /dev/null
> +++ b/tests/f_bigalloc_symlink_with_xattr/script
> @@ -0,0 +1,2 @@
> +ONE_PASS_ONLY="true"
> +. $cmd_dir/run_e2fsck
> --
> 2.13.0
> 


Cheers, Andreas






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

Powered by blists - more mailing lists