[PATCH] ext4: Improve feature checking

Sean Anderson seanga2 at gmail.com
Mon Jul 1 02:32:48 CEST 2024


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 at 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



More information about the U-Boot mailing list