[U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support
Stefan Bruens
stefan.bruens at rwth-aachen.de
Sun Aug 14 03:50:41 CEST 2016
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 at walle.cc>
I have tested this on sandbox (x86_64), no regressions found. Some remarks
below.
Reviewed-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
Tested-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> ---
>
> Ok this patch is huge, please comment. I know, checkpatch fails because
> there are longer lines than 80 characters. I don't want do be rude, but the
> coding style is awful :/ Eg.
>
> http://git.denx.de/?p=u-boot.git;a=blob;f=fs/ext4/ext4_common.c;h=eb49fce04
> c5a290e839575945da722bc97d2f670;hb=HEAD#l440
> http://git.denx.de/?p=u-boot.git;a=blob;f=fs/ext4/ext4_write.c;h=e027916763
> f9b52937c7fe13f1698b67b94eb901;hb=HEAD#l137
>
> Many ugly places seems to come from the 80 characters limit. Most of the
> code looks like its right-aligned. I know there is a valid point for having
> this limit, eg refactor code into small functions. And frankly, the ext4
> write code badly needs such a refactoring. But I won't and can't do it. I
> can resend a new version which meet the 80 character restriction, but it
> won't make the code easier to read. And believe me, it is already hard to
> do it.
>
> Ok, back to business. Sparse really helped to find the places where the
> byteswaps were missing once you annotated it correctly with the
> __le16/__le32 types. If someone want to do check again, just run
> make C=1
> See Documentation/sparse.txt in the linux kernel for more information. I've
> done a short test, to verify I can (over)write a 700kB file. No cornertests
> etc.
>
> fs/ext4/ext4_common.c | 260
> ++++++++++++++++++++++++++----------------------- fs/ext4/ext4_common.h |
> 4 +-
> fs/ext4/ext4_journal.c | 15 +--
> fs/ext4/ext4_journal.h | 4 +-
> fs/ext4/ext4_write.c | 195 +++++++++++++++++++------------------
> include/ext_common.h | 2 +-
> 6 files changed, 251 insertions(+), 229 deletions(-)
>
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index e8ed30a..4eb4e18 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -33,19 +33,22 @@
>
> struct ext2_data *ext4fs_root;
> struct ext2fs_node *ext4fs_file;
> -uint32_t *ext4fs_indir1_block;
> +__le32 *ext4fs_indir1_block;
> int ext4fs_indir1_size;
> int ext4fs_indir1_blkno = -1;
> -uint32_t *ext4fs_indir2_block;
> +__le32 *ext4fs_indir2_block;
> int ext4fs_indir2_size;
> int ext4fs_indir2_blkno = -1;
>
> -uint32_t *ext4fs_indir3_block;
> +__le32 *ext4fs_indir3_block;
> int ext4fs_indir3_size;
> int ext4fs_indir3_blkno = -1;
> struct ext2_inode *g_parent_inode;
> static int symlinknest;
>
> +#define DEC_LE16(x) (x = cpu_to_le16(le16_to_cpu(x) - 1))
> +#define DEC_LE32(x) (x = cpu_to_le32(le32_to_cpu(x) - 1))
I would prefer a static function *per variable/field* - this will make it
easier to manipulate any high/low split counts for FEATURE_64BIT. Same for the
INC_xxx in ext4_write.c
> @@ -360,6 +364,9 @@ void ext4fs_update_parent_dentry(char *filename, int
> *p_ino, int file_type) /* directory entry */
> struct ext2_dirent *dir;
> char *temp_dir = NULL;
> + uint32_t new_blk_no;
> + uint32_t new_size;
> + uint32_t new_blockcnt;
>
> zero_buffer = zalloc(fs->blksz);
> if (!zero_buffer) {
> @@ -396,7 +403,7 @@ restart:
> goto fail;
> dir = (struct ext2_dirent *)root_first_block_buffer;
> totalbytes = 0;
> - while (dir->direntlen > 0) {
> + while (le16_to_cpu(dir->direntlen) > 0) {
direntlen is at least 8, so this is "while (true) {"
> /*
> * blocksize-totalbytes because last directory length
> * i.e. dir->direntlen is free availble space in the
> @@ -405,7 +412,7 @@ restart:
> */
>
> /* traversing the each directory entry */
> - if (fs->blksz - totalbytes == dir->direntlen) {
> + if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen)) {
> if (strlen(filename) % 4 != 0)
> padding_factor = 4 - (strlen(filename) % 4);
>
> @@ -430,32 +437,34 @@ restart:
> printf("Directory exceeds limit\n");
> goto fail;
> }
> - g_parent_inode->b.blocks.dir_blocks
> - [direct_blk_idx] = ext4fs_get_new_blk_no();
> - if (g_parent_inode->b.blocks.dir_blocks
> - [direct_blk_idx] == -1) {
> + new_blk_no = ext4fs_get_new_blk_no();
> + if (new_blk_no == -1) {
> printf("no block left to assign\n");
> goto fail;
> }
> - put_ext4(((uint64_t)
> - ((uint64_t)g_parent_inode->b.
> - blocks.dir_blocks[direct_blk_idx] *
> - (uint64_t)fs->blksz)), zero_buffer, fs->blksz);
> - g_parent_inode->size =
> - g_parent_inode->size + fs->blksz;
> - g_parent_inode->blockcnt =
> - g_parent_inode->blockcnt + fs->sect_perblk;
> + put_ext4((uint64_t)new_blk_no * fs->blksz, zero_buffer, fs-
>blksz);
> + g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] =
> + cpu_to_le32(new_blk_no);
This is (already was) broken if the directory uses extents, we have to fix
this (in a seperate patch).
> +
> + new_size = le32_to_cpu(g_parent_inode->size);
> + new_size += fs->blksz;
> + g_parent_inode->size = cpu_to_le32(new_size);
> +
> + new_blockcnt = le32_to_cpu(g_parent_inode->blockcnt);
> + new_blockcnt += fs->sect_perblk;
> + g_parent_inode->blockcnt = cpu_to_le32(new_blockcnt);
> +
> if (ext4fs_put_metadata
> (root_first_block_buffer,
> first_block_no_of_root))
> goto fail;
> goto restart;
> }
> - dir->direntlen = last_entry_dirlen;
> + dir->direntlen = cpu_to_le16(last_entry_dirlen);
> break;
> }
>
> - templength = dir->direntlen;
> + templength = le16_to_cpu(dir->direntlen);
> totalbytes = totalbytes + templength;
> sizeof_void_space = check_void_in_dentry(dir, filename);
> if (sizeof_void_space)
> @@ -534,7 +543,7 @@ static int search_dir(struct ext2_inode *parent_inode,
> char *dirname) dir = (struct ext2_dirent *)block_buffer;
> ptr = (char *)dir;
> totalbytes = 0;
> - while (dir->direntlen >= 0) {
> + while (le16_to_cpu(dir->direntlen) >= 0) {
dito, "while (true) { "
> @@ -780,7 +789,7 @@ static int check_filename(char *filename, unsigned int
> blknr) dir = (struct ext2_dirent *)root_first_block_buffer;
> ptr = (char *)dir;
> totalbytes = 0;
> - while (dir->direntlen >= 0) {
> + while (le16_to_cpu(dir->direntlen) >= 0) {
dito
> @@ -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 ...
> diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
> index 48fd2ac..370a717 100644
> --- a/fs/ext4/ext4_common.h
> +++ b/fs/ext4/ext4_common.h
> @@ -59,10 +59,10 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char
> *name,
>
> #if defined(CONFIG_EXT4_WRITE)
> uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n);
> -int ext4fs_checksum_update(unsigned int i);
> +uint16_t ext4fs_checksum_update(unsigned int i);
> int ext4fs_get_parent_inode_num(const char *dirname, char *dname, int
> flags); void ext4fs_update_parent_dentry(char *filename, int *p_ino, int
> file_type); -long int ext4fs_get_new_blk_no(void);
> +uint32_t ext4fs_get_new_blk_no(void);
> int ext4fs_get_new_inode_no(void);
> void ext4fs_reset_block_bmap(long int blockno, unsigned char *buffer,
> int index);
> diff --git a/fs/ext4/ext4_journal.c b/fs/ext4/ext4_journal.c
> index 3f61335..cf14049 100644
> --- a/fs/ext4/ext4_journal.c
> +++ b/fs/ext4/ext4_journal.c
> @@ -151,7 +151,7 @@ int ext4fs_log_gdt(char *gd_table)
> * journal_buffer -- Buffer containing meta data
> * blknr -- Block number on disk of the meta data buffer
> */
> -int ext4fs_log_journal(char *journal_buffer, long int blknr)
> +int ext4fs_log_journal(char *journal_buffer, uint32_t blknr)
> {
> struct ext_filesystem *fs = get_fs();
> short i;
> @@ -183,7 +183,7 @@ int ext4fs_log_journal(char *journal_buffer, long int
> blknr) * metadata_buffer -- Buffer containing meta data
> * blknr -- Block number on disk of the meta data buffer
> */
> -int ext4fs_put_metadata(char *metadata_buffer, long int blknr)
> +int ext4fs_put_metadata(char *metadata_buffer, uint32_t blknr)
> {
> struct ext_filesystem *fs = get_fs();
> if (!metadata_buffer) {
> @@ -215,7 +215,7 @@ void print_revoke_blks(char *revk_blk)
> printf("total bytes %d\n", max);
>
> while (offset < max) {
> - blocknr = be32_to_cpu(*((long int *)(revk_blk + offset)));
> + blocknr = be32_to_cpu(*((__be32 *)(revk_blk + offset)));
> printf("revoke blknr is %ld\n", blocknr);
> offset += 4;
> }
> @@ -302,7 +302,7 @@ int check_blknr_for_revoke(long int blknr, int
> sequence_no) max = be32_to_cpu(header->r_count);
>
> while (offset < max) {
> - blocknr = be32_to_cpu(*((long int *)
> + blocknr = be32_to_cpu(*((__be32 *)
> (revk_blk + offset)));
> if (blocknr == blknr)
> goto found;
> @@ -420,7 +420,7 @@ int ext4fs_check_journal_state(int recovery_flag)
> temp_buff);
> jsb = (struct journal_superblock_t *) temp_buff;
>
> - if (fs->sb->feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER) {
> + if (le32_to_cpu(fs->sb->feature_incompat) & EXT3_FEATURE_INCOMPAT_RECOVER)
if (fs->sb->revision_level != 0) && ..
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
work: +49 2405 49936-424
More information about the U-Boot
mailing list