From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Walle Date: Tue, 16 Aug 2016 11:30:03 +0200 Subject: [U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support In-Reply-To: <2198104.q3j2TPqOcn@pebbles.site> References: <1471007781-23656-1-git-send-email-michael@walle.cc> <1471007781-23656-3-git-send-email-michael@walle.cc> <2198104.q3j2TPqOcn@pebbles.site> Message-ID: <2a4ae1787be659033246cd368af890a7@walle.cc> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > I have tested this on sandbox (x86_64), no regressions found. Some > remarks > below. > > Reviewed-by: Stefan Br?ns > Tested-by: Stefan Br?ns > >> --- >> >> 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) && ..