e2fsck: fix endianness problem when reading htree nodes

Wrong directory block number can be saved in ->previous on big endian
system in parse_int_node(). Fix it by moving the mask out of the endian
conversion.

Fixes: ae9efd05a986 ("e2fsck: 3 level hash tree directory optimization")
Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/pass2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Mar 9, 2018, at 4:28 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> Wrong directory block number can be saved in ->previous on big endian
> system in parse_int_node(). Fix it by moving the mask out of the endian
> conversion.
> 
> Fixes: ae9efd05a986 ("e2fsck: 3 level hash tree directory optimization")
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

I was just going to make a comment about there being a cosmetic issue that
it should use 0x00ffffff to make it clear the whole top byte was masked
off, and/or add a #define to indicate what this value actually is, but
looking at this more closely it seems there is also another bug here...

> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index 1b0504c..345b29e 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -664,7 +664,7 @@ static void parse_int_node(ext2_filsys fs,
> 		}
> 
> 		dx_db->previous =
> -			i ? ext2fs_le32_to_cpu(ent[i-1].block & 0x0ffffff) : 0;
> +			i ? ext2fs_le32_to_cpu(ent[i-1].block) & 0x0ffffff : 0;

The dx_get_block() function in the kernel uses "0x0fffffff" (28 bits) to mask
the logical block number, so the directory size can be up to 2^28*blocksize
(=1TB for 4KB blocks) vs. previous limit of 2^24*blocksize (=64GB).  The old
limit was too small for a 3-level htree that allows up to 2^27 leaf blocks
on 4KB block filesystems (blocksize/8 ^ 3).

We haven't yet implemented the "use top bits to store block fullness to help
leaf block compaction" feature, so expanding the mask doesn't hurt us.

Rather than using 0xffffff directly, it makes sense to add something like:

    #define EXT4_DX_BLOCK_MASK 0x0fffffff

and use this in both the kernel and e2fsprogs?  I checked the rest of e2fsprogs
and it looks like this is the only place where this mask is used.

Cheers, Andreas

On Fri, Mar 09, 2018 at 12:12:04PM -0700, Andreas Dilger wrote:
> 
> Rather than using 0xffffff directly, it makes sense to add something like:
> 
>     #define EXT4_DX_BLOCK_MASK 0x0fffffff
> 
> and use this in both the kernel and e2fsprogs?  I checked the rest of e2fsprogs
> and it looks like this is the only place where this mask is used.

Makes sense to me.  I'll apply this patch with this change.

Also, this mask is actually used in one more place, in
parse_int_node():

		blk = ext2fs_le32_to_cpu(ent[i].block) & 0x0ffffff;

I'll fix that up too.

						- Ted

On Fri, Mar 09, 2018 at 12:12:04PM -0700, Andreas Dilger wrote:
> On Mar 9, 2018, at 4:28 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> > 
> > Wrong directory block number can be saved in ->previous on big endian
> > system in parse_int_node(). Fix it by moving the mask out of the endian
> > conversion.
> > 
> > Fixes: ae9efd05a986 ("e2fsck: 3 level hash tree directory optimization")
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> I was just going to make a comment about there being a cosmetic issue that
> it should use 0x00ffffff to make it clear the whole top byte was masked
> off, and/or add a #define to indicate what this value actually is, but
> looking at this more closely it seems there is also another bug here...
> 
> > diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> > index 1b0504c..345b29e 100644
> > --- a/e2fsck/pass2.c
> > +++ b/e2fsck/pass2.c
> > @@ -664,7 +664,7 @@ static void parse_int_node(ext2_filsys fs,
> > 		}
> > 
> > 		dx_db->previous =
> > -			i ? ext2fs_le32_to_cpu(ent[i-1].block & 0x0ffffff) : 0;
> > +			i ? ext2fs_le32_to_cpu(ent[i-1].block) & 0x0ffffff : 0;
> 
> The dx_get_block() function in the kernel uses "0x0fffffff" (28 bits) to mask
> the logical block number, so the directory size can be up to 2^28*blocksize
> (=1TB for 4KB blocks) vs. previous limit of 2^24*blocksize (=64GB).  The old
> limit was too small for a 3-level htree that allows up to 2^27 leaf blocks
> on 4KB block filesystems (blocksize/8 ^ 3).
> 
> We haven't yet implemented the "use top bits to store block fullness to help
> leaf block compaction" feature, so expanding the mask doesn't hurt us.
> 
> Rather than using 0xffffff directly, it makes sense to add something like:
> 
>     #define EXT4_DX_BLOCK_MASK 0x0fffffff
> 
> and use this in both the kernel and e2fsprogs?  I checked the rest of e2fsprogs
> and it looks like this is the only place where this mask is used.

Thanks Andreas, I think it makes sense.

-Lukas

> 
> Cheers, Andreas
> 
> 
> 
> 
>