[U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support

[Brüns, Stefan]

On Dienstag, 16. August 2016 13:41:21 CEST you wrote:
> Hi Stefan,
> 
> Am 2016-08-14 03:50, schrieb Stefan Bruens:
> > On Freitag, 12. August 2016 15:16:20 CEST Michael Walle wrote:
> >> All fields were accessed directly instead of using the proper byte
> >> swap
> >> functions. Thus, ext4 write support was only usable on little-endian
> >> architectures. Fix this.
> >> 
> >> Signed-off-by: Michael Walle <michael at walle.cc>
> > 
> > I have tested this on sandbox (x86_64), no regressions found. Some
> > remarks
> > below.
> > 
> > Reviewed-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> > Tested-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> 
> [snip]
> 
> >> @@ -2234,7 +2246,7 @@ int ext4fs_mount(unsigned part_length)
> >> 
> >>  	 * and we do not support metadata_csum (and cannot reliably find
> >>  	 * files when it is set.  Refuse to mount.
> >>  	 */
> >> 
> >> -	if (data->sblock.feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> >> +	if (le32_to_cpu(data->sblock.feature_incompat) &
> >> EXT4_FEATURE_INCOMPAT_64BIT) { printf("Unsupported feature found
> >> (64bit,
> >> possibly metadata_csum), not mounting\n"); goto fail;
> >> 
> >>  	}
> > 
> > This should have a if ((data->sblock.revision_level !=0) && ... in
> > front,
> > features are not defined for revision 0. Applies to other places as
> > well ...
> 
> are you sure about that? I can't find any code in the kernel where
> features are only valid if revision > 0. Also, I couldn't find anything
> in the ext4 documentation wiki:
> 
> https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout

This document states that inode_size and all later fields are only defined for 
revision 1.

The statement seems to originate from here:
http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/
ext2_fs.h#n651

Now, if the remainder of the superblock after def_resgid is guaranteed to be 
zeroed the feature checks are ok for any revision, whereas a zero inode size 
would be bad and defaulting to 128 is needed.

As e2fsprogs is skipping any revision-level tests for feature checks, it seems 
to be ok to assume remaining fields to be zeroed.

Kind regards,

Stefan