Message ID | 20240630212556.5627-1-richard@nod.at |
---|---|
State | Changes Requested |
Delegated to: | Sean Anderson |
Headers | show |
Series | ext4: Improve feature checking | expand |
On 6/30/24 17:25, Richard Weinberger wrote: > Evaluate the filesystem incompat and ro_compat bit fields to judge > whether the filesystem can be read or written. > For the read side only a scary warning is shown so far. > I'd love to about mounting too, but I fear this will break some setups I think you are missing a verb in the first half of your sentence > where the driver works by chance. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > After facing ext4 write corruptions and read errors I checked > the ext4 implementation briefly. > Hopefully this patch helps other users to detect earlier that > they're using ext4 features which are not supported by u-boot. > > To make feature checking possible I had to figure what this > implementation actually supports. > I hope I got them right, feedback is welcome! > > Thanks, > //richard > --- > fs/ext4/ext4_common.c | 8 +++++++ > fs/ext4/ext4_write.c | 12 ++++++++-- > include/ext4fs.h | 55 ++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c > index 2ff0dca249..7148d35ee0 100644 > --- a/fs/ext4/ext4_common.c > +++ b/fs/ext4/ext4_common.c > @@ -2386,6 +2386,14 @@ int ext4fs_mount(void) > fs->inodesz = 128; > fs->gdsize = 32; > } else { > + int missing = __le32_to_cpu(data->sblock.feature_incompat) & \ > + ~(EXT4_FEATURE_INCOMPAT_SUPP | \ > + EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO); > + > + if (missing) > + log_err("fs uses incompatible features: %08x, failures *are* expected!\n", > + missing); > + I think it is a bit unclear to the user what their action should be. Maybe something like printf("EXT4 incompatible features: %08x, ignoring...\n") and then maybe add a comment like what you have in the commit message. > debug("EXT4 features COMPAT: %08x INCOMPAT: %08x RO_COMPAT: %08x\n", > __le32_to_cpu(data->sblock.feature_compatibility), > __le32_to_cpu(data->sblock.feature_incompat), > diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c > index d057f6b5a7..4aae3c5f7f 100644 > --- a/fs/ext4/ext4_write.c > +++ b/fs/ext4/ext4_write.c > @@ -869,6 +869,7 @@ int ext4fs_write(const char *fname, const char *buffer, > ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256); > bool store_link_in_inode = false; > memset(filename, 0x00, 256); > + int missing_feat; > > if (type != FILETYPE_REG && type != FILETYPE_SYMLINK) > return -1; > @@ -882,8 +883,15 @@ int ext4fs_write(const char *fname, const char *buffer, > return -1; > } > > - if (le32_to_cpu(fs->sb->feature_ro_compat) & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) { > - printf("Unsupported feature metadata_csum found, not writing.\n"); > + missing_feat = le32_to_cpu(fs->sb->feature_incompat) & ~EXT4_FEATURE_INCOMPAT_SUPP; > + if (missing_feat) { > + log_err("Unsupported features found %08x, not writing.\n", missing_feat); > + return -1; > + } > + > + missing_feat = le32_to_cpu(fs->sb->feature_ro_compat) & ~EXT4_FEATURE_RO_COMPAT_SUPP; > + if (missing_feat) { > + log_err("Unsupported RO compat features found %08x, not writing.\n", missing_feat); > return -1; > } > > diff --git a/include/ext4fs.h b/include/ext4fs.h > index d96edfd057..01b66f469f 100644 > --- a/include/ext4fs.h > +++ b/include/ext4fs.h > @@ -34,12 +34,65 @@ struct disk_partition; > #define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/ > #define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */ > #define EXT4_EXT_MAGIC 0xf30a > -#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 > + > +#define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001 > +#define EXT4_FEATURE_RO_COMPAT_LARGE_FILE 0x0002 > +#define EXT4_FEATURE_RO_COMPAT_BTREE_DIR 0x0004 > +#define EXT4_FEATURE_RO_COMPAT_HUGE_FILE 0x0008 > +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 > +#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020 > +#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040 > +#define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100 > +#define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200 > #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400 > + > +#define EXT4_FEATURE_INCOMPAT_FILETYPE 0x0002 > +#define EXTE_FEATURE_INCOMPAT_RECOVER 0x0004 > #define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 > #define EXT4_FEATURE_INCOMPAT_64BIT 0x0080 > +#define EXT4_FEATURE_INCOMPAT_MMP 0x0100 > +#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200 > +#define EXT4_FEATURE_INCOMPAT_CSUM_SEED 0x2000 > +#define EXT4_FEATURE_INCOMPAT_ENCRYPT 0x10000 > + > #define EXT4_INDIRECT_BLOCKS 12 > > +/* > + * Incompat features supported by this implementation. > + */ > +#define EXT4_FEATURE_INCOMPAT_SUPP ( EXT4_FEATURE_INCOMPAT_FILETYPE \ > + | EXTE_FEATURE_INCOMPAT_RECOVER \ EXT4 > + | EXT4_FEATURE_INCOMPAT_EXTENTS \ > + | EXT4_FEATURE_INCOMPAT_64BIT \ > + | EXT4_FEATURE_INCOMPAT_FLEX_BG \ > + ) Operators should go at the end of the line. So like #define EXT4_FEATURE_INCOMPAT_SUPP (EXT4_FEATURE_INCOMPAT_FILETYPE | \ EXT4_FEATURE_INCOMPAT_RECOVER | \ EXT4_FEATURE_INCOMPAT_EXTENTS | \ EXT4_FEATURE_INCOMPAT_64BIT | \ EXT4_FEATURE_INCOMPAT_FLEX_BG) > +/* > + * Incompat features supported by this implementation only in a lazy > + * way, good enough for reading files. > + * > + * - Multi mount protection (mmp) is not supported, but for read-only > + * we get away with it. > + * - Same fore metadata_csum_seed and metadata_csum. nit: for > + * - The implementation has also no clue about fscrypt, but it can read > + * unencrypted files. Reading encrypted files will read garbage. > + */ > +#define EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO ( EXT4_FEATURE_INCOMPAT_MMP \ > + | EXT4_FEATURE_INCOMPAT_CSUM_SEED \ > + | EXT4_FEATURE_INCOMPAT_ENCRYPT \ > + ) ditto > +/* > + * Read-only compat features we support. > + * If unknown ro compat features are detected, writing to the fs is denied. > + */ > +#define EXT4_FEATURE_RO_COMPAT_SUPP ( EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER \ > + | EXT4_FEATURE_RO_COMPAT_LARGE_FILE \ > + | EXT4_FEATURE_RO_COMPAT_HUGE_FILE \ > + | EXT4_FEATURE_RO_COMPAT_DIR_NLINK \ > + | EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE \ > + ) ditto > #define EXT4_BG_INODE_UNINIT 0x0001 > #define EXT4_BG_BLOCK_UNINIT 0x0002 > #define EXT4_BG_INODE_ZEROED 0x0004 Anyway, the substance of this patch is good. Just needs a little cosmetic cleanup. --Sean
On 6/30/24 20:23, Sean Anderson wrote: > On 6/30/24 17:25, Richard Weinberger wrote: >> Evaluate the filesystem incompat and ro_compat bit fields to judge >> whether the filesystem can be read or written. >> For the read side only a scary warning is shown so far. >> I'd love to about mounting too, but I fear this will break some setups > > I think you are missing a verb in the first half of your sentence > >> where the driver works by chance. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> After facing ext4 write corruptions and read errors I checked >> the ext4 implementation briefly. >> Hopefully this patch helps other users to detect earlier that >> they're using ext4 features which are not supported by u-boot. >> >> To make feature checking possible I had to figure what this >> implementation actually supports. >> I hope I got them right, feedback is welcome! >> >> Thanks, >> //richard >> --- >> fs/ext4/ext4_common.c | 8 +++++++ >> fs/ext4/ext4_write.c | 12 ++++++++-- >> include/ext4fs.h | 55 ++++++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 72 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c >> index 2ff0dca249..7148d35ee0 100644 >> --- a/fs/ext4/ext4_common.c >> +++ b/fs/ext4/ext4_common.c >> @@ -2386,6 +2386,14 @@ int ext4fs_mount(void) >> fs->inodesz = 128; >> fs->gdsize = 32; >> } else { >> + int missing = __le32_to_cpu(data->sblock.feature_incompat) & \ >> + ~(EXT4_FEATURE_INCOMPAT_SUPP | \ >> + EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO); >> + >> + if (missing) >> + log_err("fs uses incompatible features: %08x, failures *are* expected!\n", >> + missing); >> + > > I think it is a bit unclear to the user what their action should be. Maybe something like > > printf("EXT4 incompatible features: %08x, ignoring...\n") > > and then maybe add a comment like what you have in the commit message. or maybe even printf("EXT4 incompatible features: %08x, mounting read-only...\n") >> debug("EXT4 features COMPAT: %08x INCOMPAT: %08x RO_COMPAT: %08x\n", >> __le32_to_cpu(data->sblock.feature_compatibility), >> __le32_to_cpu(data->sblock.feature_incompat), >> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c >> index d057f6b5a7..4aae3c5f7f 100644 >> --- a/fs/ext4/ext4_write.c >> +++ b/fs/ext4/ext4_write.c >> @@ -869,6 +869,7 @@ int ext4fs_write(const char *fname, const char *buffer, >> ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256); >> bool store_link_in_inode = false; >> memset(filename, 0x00, 256); >> + int missing_feat; >> if (type != FILETYPE_REG && type != FILETYPE_SYMLINK) >> return -1; >> @@ -882,8 +883,15 @@ int ext4fs_write(const char *fname, const char *buffer, >> return -1; >> } >> - if (le32_to_cpu(fs->sb->feature_ro_compat) & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) { >> - printf("Unsupported feature metadata_csum found, not writing.\n"); >> + missing_feat = le32_to_cpu(fs->sb->feature_incompat) & ~EXT4_FEATURE_INCOMPAT_SUPP; >> + if (missing_feat) { >> + log_err("Unsupported features found %08x, not writing.\n", missing_feat); >> + return -1; >> + } >> + >> + missing_feat = le32_to_cpu(fs->sb->feature_ro_compat) & ~EXT4_FEATURE_RO_COMPAT_SUPP; >> + if (missing_feat) { >> + log_err("Unsupported RO compat features found %08x, not writing.\n", missing_feat); >> return -1; >> } >> diff --git a/include/ext4fs.h b/include/ext4fs.h >> index d96edfd057..01b66f469f 100644 >> --- a/include/ext4fs.h >> +++ b/include/ext4fs.h >> @@ -34,12 +34,65 @@ struct disk_partition; >> #define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/ >> #define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */ >> #define EXT4_EXT_MAGIC 0xf30a >> -#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 >> + >> +#define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001 >> +#define EXT4_FEATURE_RO_COMPAT_LARGE_FILE 0x0002 >> +#define EXT4_FEATURE_RO_COMPAT_BTREE_DIR 0x0004 >> +#define EXT4_FEATURE_RO_COMPAT_HUGE_FILE 0x0008 >> +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 >> +#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020 >> +#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040 >> +#define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100 >> +#define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200 >> #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400 >> + >> +#define EXT4_FEATURE_INCOMPAT_FILETYPE 0x0002 >> +#define EXTE_FEATURE_INCOMPAT_RECOVER 0x0004 >> #define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 >> #define EXT4_FEATURE_INCOMPAT_64BIT 0x0080 >> +#define EXT4_FEATURE_INCOMPAT_MMP 0x0100 >> +#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200 >> +#define EXT4_FEATURE_INCOMPAT_CSUM_SEED 0x2000 >> +#define EXT4_FEATURE_INCOMPAT_ENCRYPT 0x10000 >> + >> #define EXT4_INDIRECT_BLOCKS 12 >> +/* >> + * Incompat features supported by this implementation. >> + */ >> +#define EXT4_FEATURE_INCOMPAT_SUPP ( EXT4_FEATURE_INCOMPAT_FILETYPE \ >> + | EXTE_FEATURE_INCOMPAT_RECOVER \ > > EXT4 > >> + | EXT4_FEATURE_INCOMPAT_EXTENTS \ >> + | EXT4_FEATURE_INCOMPAT_64BIT \ >> + | EXT4_FEATURE_INCOMPAT_FLEX_BG \ >> + ) > > Operators should go at the end of the line. So like > > #define EXT4_FEATURE_INCOMPAT_SUPP (EXT4_FEATURE_INCOMPAT_FILETYPE | \ > EXT4_FEATURE_INCOMPAT_RECOVER | \ > EXT4_FEATURE_INCOMPAT_EXTENTS | \ > EXT4_FEATURE_INCOMPAT_64BIT | \ > EXT4_FEATURE_INCOMPAT_FLEX_BG) > >> +/* >> + * Incompat features supported by this implementation only in a lazy >> + * way, good enough for reading files. >> + * >> + * - Multi mount protection (mmp) is not supported, but for read-only >> + * we get away with it. >> + * - Same fore metadata_csum_seed and metadata_csum. > > nit: for > >> + * - The implementation has also no clue about fscrypt, but it can read >> + * unencrypted files. Reading encrypted files will read garbage. >> + */ >> +#define EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO ( EXT4_FEATURE_INCOMPAT_MMP \ >> + | EXT4_FEATURE_INCOMPAT_CSUM_SEED \ >> + | EXT4_FEATURE_INCOMPAT_ENCRYPT \ >> + ) > > ditto > >> +/* >> + * Read-only compat features we support. >> + * If unknown ro compat features are detected, writing to the fs is denied. >> + */ >> +#define EXT4_FEATURE_RO_COMPAT_SUPP ( EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER \ >> + | EXT4_FEATURE_RO_COMPAT_LARGE_FILE \ >> + | EXT4_FEATURE_RO_COMPAT_HUGE_FILE \ >> + | EXT4_FEATURE_RO_COMPAT_DIR_NLINK \ >> + | EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE \ >> + ) > > ditto > >> #define EXT4_BG_INODE_UNINIT 0x0001 >> #define EXT4_BG_BLOCK_UNINIT 0x0002 >> #define EXT4_BG_INODE_ZEROED 0x0004 > > Anyway, the substance of this patch is good. Just needs a little cosmetic cleanup. > > --Sean
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 2ff0dca249..7148d35ee0 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2386,6 +2386,14 @@ int ext4fs_mount(void) fs->inodesz = 128; fs->gdsize = 32; } else { + int missing = __le32_to_cpu(data->sblock.feature_incompat) & \ + ~(EXT4_FEATURE_INCOMPAT_SUPP | \ + EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO); + + if (missing) + log_err("fs uses incompatible features: %08x, failures *are* expected!\n", + missing); + debug("EXT4 features COMPAT: %08x INCOMPAT: %08x RO_COMPAT: %08x\n", __le32_to_cpu(data->sblock.feature_compatibility), __le32_to_cpu(data->sblock.feature_incompat), diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index d057f6b5a7..4aae3c5f7f 100644 --- a/fs/ext4/ext4_write.c +++ b/fs/ext4/ext4_write.c @@ -869,6 +869,7 @@ int ext4fs_write(const char *fname, const char *buffer, ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256); bool store_link_in_inode = false; memset(filename, 0x00, 256); + int missing_feat; if (type != FILETYPE_REG && type != FILETYPE_SYMLINK) return -1; @@ -882,8 +883,15 @@ int ext4fs_write(const char *fname, const char *buffer, return -1; } - if (le32_to_cpu(fs->sb->feature_ro_compat) & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) { - printf("Unsupported feature metadata_csum found, not writing.\n"); + missing_feat = le32_to_cpu(fs->sb->feature_incompat) & ~EXT4_FEATURE_INCOMPAT_SUPP; + if (missing_feat) { + log_err("Unsupported features found %08x, not writing.\n", missing_feat); + return -1; + } + + missing_feat = le32_to_cpu(fs->sb->feature_ro_compat) & ~EXT4_FEATURE_RO_COMPAT_SUPP; + if (missing_feat) { + log_err("Unsupported RO compat features found %08x, not writing.\n", missing_feat); return -1; } diff --git a/include/ext4fs.h b/include/ext4fs.h index d96edfd057..01b66f469f 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -34,12 +34,65 @@ struct disk_partition; #define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/ #define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */ #define EXT4_EXT_MAGIC 0xf30a -#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 + +#define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001 +#define EXT4_FEATURE_RO_COMPAT_LARGE_FILE 0x0002 +#define EXT4_FEATURE_RO_COMPAT_BTREE_DIR 0x0004 +#define EXT4_FEATURE_RO_COMPAT_HUGE_FILE 0x0008 +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 +#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020 +#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040 +#define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100 +#define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200 #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400 + +#define EXT4_FEATURE_INCOMPAT_FILETYPE 0x0002 +#define EXTE_FEATURE_INCOMPAT_RECOVER 0x0004 #define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 #define EXT4_FEATURE_INCOMPAT_64BIT 0x0080 +#define EXT4_FEATURE_INCOMPAT_MMP 0x0100 +#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200 +#define EXT4_FEATURE_INCOMPAT_CSUM_SEED 0x2000 +#define EXT4_FEATURE_INCOMPAT_ENCRYPT 0x10000 + #define EXT4_INDIRECT_BLOCKS 12 +/* + * Incompat features supported by this implementation. + */ +#define EXT4_FEATURE_INCOMPAT_SUPP ( EXT4_FEATURE_INCOMPAT_FILETYPE \ + | EXTE_FEATURE_INCOMPAT_RECOVER \ + | EXT4_FEATURE_INCOMPAT_EXTENTS \ + | EXT4_FEATURE_INCOMPAT_64BIT \ + | EXT4_FEATURE_INCOMPAT_FLEX_BG \ + ) + +/* + * Incompat features supported by this implementation only in a lazy + * way, good enough for reading files. + * + * - Multi mount protection (mmp) is not supported, but for read-only + * we get away with it. + * - Same fore metadata_csum_seed and metadata_csum. + * - The implementation has also no clue about fscrypt, but it can read + * unencrypted files. Reading encrypted files will read garbage. + */ +#define EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO ( EXT4_FEATURE_INCOMPAT_MMP \ + | EXT4_FEATURE_INCOMPAT_CSUM_SEED \ + | EXT4_FEATURE_INCOMPAT_ENCRYPT \ + ) + +/* + * Read-only compat features we support. + * If unknown ro compat features are detected, writing to the fs is denied. + */ +#define EXT4_FEATURE_RO_COMPAT_SUPP ( EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER \ + | EXT4_FEATURE_RO_COMPAT_LARGE_FILE \ + | EXT4_FEATURE_RO_COMPAT_HUGE_FILE \ + | EXT4_FEATURE_RO_COMPAT_DIR_NLINK \ + | EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE \ + ) + #define EXT4_BG_INODE_UNINIT 0x0001 #define EXT4_BG_BLOCK_UNINIT 0x0002 #define EXT4_BG_INODE_ZEROED 0x0004
Evaluate the filesystem incompat and ro_compat bit fields to judge whether the filesystem can be read or written. For the read side only a scary warning is shown so far. I'd love to about mounting too, but I fear this will break some setups where the driver works by chance. Signed-off-by: Richard Weinberger <richard@nod.at> --- After facing ext4 write corruptions and read errors I checked the ext4 implementation briefly. Hopefully this patch helps other users to detect earlier that they're using ext4 features which are not supported by u-boot. To make feature checking possible I had to figure what this implementation actually supports. I hope I got them right, feedback is welcome! Thanks, //richard --- fs/ext4/ext4_common.c | 8 +++++++ fs/ext4/ext4_write.c | 12 ++++++++-- include/ext4fs.h | 55 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 3 deletions(-)