From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Bruens Date: Sun, 14 Aug 2016 03:50:41 +0200 Subject: [U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support In-Reply-To: <1471007781-23656-3-git-send-email-michael@walle.cc> References: <1471007781-23656-1-git-send-email-michael@walle.cc> <1471007781-23656-3-git-send-email-michael@walle.cc> Message-ID: <2198104.q3j2TPqOcn@pebbles.site> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > @@ -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