[PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems

[PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems

[Eric Biggers]

From: Eric Biggers <ebiggers@google.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.

Fix this by updating ext2fs_inode_data_blocks2() to subtract the cluster
size rather than the block size.

Signed-off-by: Eric Biggers <ebiggers@google.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@RswM|d61u%&K3pz<I`jg5`9<4$npQ4fK%lpp`?f7mKB
zD~Iqbn<wa1?0j|$!y{*hw>KRQpDqo#XVdZZrqT3=(Xr31@3AsJ44-P=E@a^wbLINi
z`(}08^*)m)9&Bu6S7H3j&dZ=IW+=8GL1aN~b;H5eeTx~Y<kOUA<lerQq%$w~c3IAu
zZ(`XRO;L_#7i@cctIU4x=bU@>KfIF<y0Y7T37Pt2gNnMUX3+98T$AnOzGTkTZc7nn
zEBXER4ukI7yGI?CbAdesCoY(X$#0Q(5x@u+0W)TuYkBOqqxR~AyH7qDp6rpHbVYi1
z-PFgM&)n0T^u~I<$6V7cmFMZlV@eaGUTt2Pxx!d{*}`S-rnl|-eD=xS?~~`mUzh$@
z@7j01Z>Q<(@~rbFR|?ISEj(80?QCnT@v_c&(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@9JMJ6@ORz?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

Re: [PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5184 bytes --]

On May 21, 2017, at 12:23 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> From: Eric Biggers <ebiggers@google.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@google.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@RswM|d61u%&K3pz<I`jg5`9<4$npQ4fK%lpp`?f7mKB
> zD~Iqbn<wa1?0j|$!y{*hw>KRQpDqo#XVdZZrqT3=(Xr31@3AsJ44-P=E@a^wbLINi
> z`(}08^*)m)9&Bu6S7H3j&dZ=IW+=8GL1aN~b;H5eeTx~Y<kOUA<lerQq%$w~c3IAu
> zZ(`XRO;L_#7i@cctIU4x=bU@>KfIF<y0Y7T37Pt2gNnMUX3+98T$AnOzGTkTZc7nn
> zEBXER4ukI7yGI?CbAdesCoY(X$#0Q(5x@u+0W)TuYkBOqqxR~AyH7qDp6rpHbVYi1
> z-PFgM&)n0T^u~I<$6V7cmFMZlV@eaGUTt2Pxx!d{*}`S-rnl|-eD=xS?~~`mUzh$@
> z@7j01Z>Q<(@~rbFR|?ISEj(80?QCnT@v_c&(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@9JMJ6@ORz?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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems

[Eric Biggers]

Hi Andreas,

On Tue, May 23, 2017 at 10:19:35AM -0600, Andreas Dilger wrote:
> On May 21, 2017, at 12:23 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > From: Eric Biggers <ebiggers@google.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
> 

Yes, I still think we probably should do that.  This bug needed to be fixed
anyway though, since ext2fs_inode_data_blocks2() is used for a bit more than
just distinguishing between fast and slow symlinks.

Eric

Re: [PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems

[Theodore Ts'o]

On Sat, May 20, 2017 at 11:23:03PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.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.
> 
> Fix this by updating ext2fs_inode_data_blocks2() to subtract the cluster
> size rather than the block size.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

						- Ted