Re: [U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active
On 01/02/2019 16:23, Tom Rini wrote:
On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote:
u-boot does not supports updating the metadata chacksums
Signed-off-by: Jean-Jacques Hiblot
---
Changes in v2:
- Prevent write access if metadata checksum is enabled
fs/ext4/ext4_write.c | 12 ++--
include/ext4fs.h | 1 +
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index a7f543f7df..1de29236f0 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
g_parent_inode = zalloc(fs->inodesz);
if (!g_parent_inode)
- goto fail;
+ goto fail_ext4fs_init;
if (ext4fs_init() != 0) {
printf("error in File System init\n");
- return -1;
+ goto fail_ext4fs_init;
+ }
+
+ if (le32_to_cpu(fs->sb->feature_ro_compat) &
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
+ printf("u-boot doesn't support updating the metadata checksums
yet\n");
+ goto fail;
}
+
inodes_per_block = fs->blksz / fs->inodesz;
parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
if (parent_inodeno == -1)
I'm following up to myself on this one as, Ugh. To repeat myself from a
while back:
commit 6f94ab6656ceffb3f2a972c8de4c554502b6f2b7
Author: Tom Rini
Date: Fri Jul 22 17:59:11 2016 -0400
ext4: Refuse to mount filesystems with 64bit feature set
With e2fsprogs after 1.43 the 64bit and metadata_csum features are
enabled by default. The metadata_csum feature changes how
ext4_group_desc->bg_checksum is calculated, which would break write
support. The 64bit feature however introduces changes such that it
cannot be read by implementations that do not support it. Since we do
not support this, we must not mount it.
Cc: Stephen Warren
Cc: Simon Glass
Cc: Lukasz Majewski
Cc: Stefan Roese
Reported-by: Andrew Bradford
Signed-off-by: Tom Rini
Which means that starting way back then I should have also done
something to say we cannot write to these new images either. It's good
and important to finally catch this failure now. I suspect it's however
going to start to be an unexpected problem. Have you any idea how much
work would go in to supporting the metadata_csum feature? Thanks again!
I had a look and this is going to be a non-trivial job. The ext4 code is
showing its age. And then there would be another couple of flags to
handle too like journal checksuming, and then also hashtree dirs.
There are a couple of ext4 libs out there (I'm thinking of lwext4 or
e2fsprogs) that do a much better job at handling the
compatible/incompatible/read-only flags. I wonder if we wouldn't be
better off adapting one of them than fixing the current implementation.
JJ
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active
On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote:
> u-boot does not supports updating the metadata chacksums
>
> Signed-off-by: Jean-Jacques Hiblot
>
> ---
>
> Changes in v2:
> - Prevent write access if metadata checksum is enabled
>
> fs/ext4/ext4_write.c | 12 ++--
> include/ext4fs.h | 1 +
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index a7f543f7df..1de29236f0 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer,
>
> g_parent_inode = zalloc(fs->inodesz);
> if (!g_parent_inode)
> - goto fail;
> + goto fail_ext4fs_init;
>
> if (ext4fs_init() != 0) {
> printf("error in File System init\n");
> - return -1;
> + goto fail_ext4fs_init;
> + }
> +
> + if (le32_to_cpu(fs->sb->feature_ro_compat) &
> + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
> + printf("u-boot doesn't support updating the metadata checksums
> yet\n");
> + goto fail;
> }
> +
> inodes_per_block = fs->blksz / fs->inodesz;
> parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
> if (parent_inodeno == -1)
I'm following up to myself on this one as, Ugh. To repeat myself from a
while back:
commit 6f94ab6656ceffb3f2a972c8de4c554502b6f2b7
Author: Tom Rini
Date: Fri Jul 22 17:59:11 2016 -0400
ext4: Refuse to mount filesystems with 64bit feature set
With e2fsprogs after 1.43 the 64bit and metadata_csum features are
enabled by default. The metadata_csum feature changes how
ext4_group_desc->bg_checksum is calculated, which would break write
support. The 64bit feature however introduces changes such that it
cannot be read by implementations that do not support it. Since we do
not support this, we must not mount it.
Cc: Stephen Warren
Cc: Simon Glass
Cc: Lukasz Majewski
Cc: Stefan Roese
Reported-by: Andrew Bradford
Signed-off-by: Tom Rini
Which means that starting way back then I should have also done
something to say we cannot write to these new images either. It's good
and important to finally catch this failure now. I suspect it's however
going to start to be an unexpected problem. Have you any idea how much
work would go in to supporting the metadata_csum feature? Thanks again!
--
Tom
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active
On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote: > u-boot does not supports updating the metadata chacksums > > Signed-off-by: Jean-Jacques Hiblot > Reviewed-by: Tom Rini -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active
u-boot does not supports updating the metadata chacksums
Signed-off-by: Jean-Jacques Hiblot
---
Changes in v2:
- Prevent write access if metadata checksum is enabled
fs/ext4/ext4_write.c | 12 ++--
include/ext4fs.h | 1 +
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index a7f543f7df..1de29236f0 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
g_parent_inode = zalloc(fs->inodesz);
if (!g_parent_inode)
- goto fail;
+ goto fail_ext4fs_init;
if (ext4fs_init() != 0) {
printf("error in File System init\n");
- return -1;
+ goto fail_ext4fs_init;
+ }
+
+ if (le32_to_cpu(fs->sb->feature_ro_compat) &
+ EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
+ printf("u-boot doesn't support updating the metadata checksums
yet\n");
+ goto fail;
}
+
inodes_per_block = fs->blksz / fs->inodesz;
parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
if (parent_inodeno == -1)
@@ -990,6 +997,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
return 0;
fail:
ext4fs_deinit();
+fail_ext4fs_init:
free(inode_buffer);
free(g_parent_inode);
free(temp_ptr);
diff --git a/include/ext4fs.h b/include/ext4fs.h
index bb55639107..bcf440364e 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -32,6 +32,7 @@
#define EXT4_EXTENTS_FL0x0008 /* Inode uses extents */
#define EXT4_EXT_MAGIC 0xf30a
#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM0x0010
+#define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400
#define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040
#define EXT4_FEATURE_INCOMPAT_64BIT0x0080
#define EXT4_INDIRECT_BLOCKS 12
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
