From: "Brüns, Stefan" <Stefan.Bruens@rwth-aachen.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support
Date: Tue, 16 Aug 2016 17:05:40 +0000 [thread overview]
Message-ID: <2647756.yDekzaqZQ1@sbruens-linux> (raw)
In-Reply-To: <f356175bb1958ded2e8024dc59b951d0@walle.cc>
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@walle.cc>
> >
> > I have tested this on sandbox (x86_64), no regressions found. Some
> > remarks
> > below.
> >
> > Reviewed-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> > Tested-by: Stefan Br?ns <stefan.bruens@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
next prev parent reply other threads:[~2016-08-16 17:05 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-12 13:16 [U-Boot] [PATCH 1/4] ext4: change structure fields to __le/__be types Michael Walle
2016-08-12 13:16 ` [U-Boot] [PATCH 2/4] ext4: use kernel names for byte swaps Michael Walle
2016-08-12 13:16 ` [U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support Michael Walle
2016-08-14 1:50 ` Stefan Bruens
2016-08-16 9:30 ` Michael Walle
2016-08-16 11:41 ` Michael Walle
2016-08-16 17:05 ` Brüns, Stefan [this message]
2016-08-12 13:16 ` [U-Boot] [PATCH 4/4] ext4: fix wrong usage of le32_to_cpu() Michael Walle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2647756.yDekzaqZQ1@sbruens-linux \
--to=stefan.bruens@rwth-aachen.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox