Re: [PATCH] ext4: Improve feature checking

[Sean Anderson <seanga2@gmail.com>]

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