[U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support
Michael Walle
michael at walle.cc
Tue Aug 16 11:30:03 CEST 2016
Am 2016-08-14 03:50, schrieb Stefan Bruens:
> 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
ok, sounds right ;)
>> @@ -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) {"
I'll put this and all remarks below, into a separate patch, because I
want this patch to only fix the endianess problem without any functional
changes.
-michael
>
>> /*
>> * 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) && ..
More information about the U-Boot
mailing list