[U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support

[Michael Walle <michael@walle.cc>]

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@walle.cc>
> 
> I have tested this on sandbox (x86_64), no regressions found. Some 
> remarks
> below.
> 
> Reviewed-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> Tested-by: Stefan Br?ns <stefan.bruens@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) && ..