[U-Boot] [PATCH v3 01/13] ext4: fix possible crash on directory traversal, ignore deleted entries

[U-Boot] [PATCH v3 01/13] ext4: fix possible crash on directory traversal, ignore deleted entries

[Stefan Brüns]

The following command triggers a segfault in search_dir:
./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
    ext4write host 0 0 /./foo 0x10'

The following command triggers a segfault in check_filename:
./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
    ext4write host 0 0 /. 0x10'

"." is the first entry in the directory, thus previous_dir is NULL. The
whole previous_dir block in search_dir seems to be a bad copy from
check_filename(...). As the changed data is not written to disk, the
statement is mostly harmless, save the possible NULL-ptr reference.

Typically a file is unlinked by extending the direntlen of the previous
entry. If the entry is the first entry in the directory block, it is
invalidated by setting inode=0.

The inode==0 case is hard to trigger without crafted filesystems. It only
hits if the first entry in a directory block is deleted and later a lookup
for the entry (by name) is done.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
---
 fs/ext4/ext4_common.c | 58 +++++++++++++++++++--------------------------------
 fs/ext4/ext4_write.c  |  2 +-
 include/ext4fs.h      |  2 +-
 3 files changed, 23 insertions(+), 39 deletions(-)

v2: Fix bad filename compare on delete, used substring only
v3: Added Reviewed-by

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 4a003cf..3736533 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -528,16 +528,14 @@ fail:
 static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 {
 	int status;
-	int inodeno;
+	int inodeno = 0;
 	int totalbytes;
 	int templength;
 	int direct_blk_idx;
 	long int blknr;
-	int found = 0;
 	char *ptr = NULL;
 	unsigned char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
-	struct ext2_dirent *previous_dir = NULL;
 	struct ext_filesystem *fs = get_fs();
 
 	/* read the block no allocated to a file */
@@ -547,7 +545,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 		if (blknr == 0)
 			goto fail;
 
-		/* read the blocks of parenet inode */
+		/* read the blocks of parent inode */
 		block_buffer = zalloc(fs->blksz);
 		if (!block_buffer)
 			goto fail;
@@ -567,15 +565,9 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 			 * space in the block that means
 			 * it is a last entry of directory entry
 			 */
-			if (strlen(dirname) == dir->namelen) {
+			if (dir->inode && (strlen(dirname) == dir->namelen)) {
 				if (strncmp(dirname, ptr + sizeof(struct ext2_dirent), dir->namelen) == 0) {
-					uint16_t new_len;
-					new_len = le16_to_cpu(previous_dir->direntlen);
-					new_len += le16_to_cpu(dir->direntlen);
-					previous_dir->direntlen = cpu_to_le16(new_len);
 					inodeno = le32_to_cpu(dir->inode);
-					dir->inode = 0;
-					found = 1;
 					break;
 				}
 			}
@@ -586,19 +578,15 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 			/* traversing the each directory entry */
 			templength = le16_to_cpu(dir->direntlen);
 			totalbytes = totalbytes + templength;
-			previous_dir = dir;
 			dir = (struct ext2_dirent *)((char *)dir + templength);
 			ptr = (char *)dir;
 		}
 
-		if (found == 1) {
-			free(block_buffer);
-			block_buffer = NULL;
-			return inodeno;
-		}
-
 		free(block_buffer);
 		block_buffer = NULL;
+
+		if (inodeno > 0)
+			return inodeno;
 	}
 
 fail:
@@ -774,15 +762,13 @@ fail:
 	return result_inode_no;
 }
 
-static int check_filename(char *filename, unsigned int blknr)
+static int unlink_filename(char *filename, unsigned int blknr)
 {
-	unsigned int first_block_no_of_root;
 	int totalbytes = 0;
 	int templength = 0;
 	int status, inodeno;
 	int found = 0;
 	char *root_first_block_buffer = NULL;
-	char *root_first_block_addr = NULL;
 	struct ext2_dirent *dir = NULL;
 	struct ext2_dirent *previous_dir = NULL;
 	char *ptr = NULL;
@@ -790,18 +776,15 @@ static int check_filename(char *filename, unsigned int blknr)
 	int ret = -1;
 
 	/* get the first block of root */
-	first_block_no_of_root = blknr;
 	root_first_block_buffer = zalloc(fs->blksz);
 	if (!root_first_block_buffer)
 		return -ENOMEM;
-	root_first_block_addr = root_first_block_buffer;
-	status = ext4fs_devread((lbaint_t)first_block_no_of_root *
-				fs->sect_perblk, 0,
+	status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0,
 				fs->blksz, root_first_block_buffer);
 	if (status == 0)
 		goto fail;
 
-	if (ext4fs_log_journal(root_first_block_buffer, first_block_no_of_root))
+	if (ext4fs_log_journal(root_first_block_buffer, blknr))
 		goto fail;
 	dir = (struct ext2_dirent *)root_first_block_buffer;
 	ptr = (char *)dir;
@@ -813,19 +796,21 @@ static int check_filename(char *filename, unsigned int blknr)
 		 * is free availble space in the block that
 		 * means it is a last entry of directory entry
 		 */
-		if (strlen(filename) == dir->namelen) {
-			if (strncmp(filename, ptr + sizeof(struct ext2_dirent),
-				dir->namelen) == 0) {
+		if (dir->inode && (strlen(filename) == dir->namelen) &&
+		    (strncmp(ptr + sizeof(struct ext2_dirent),
+			     filename, dir->namelen) == 0)) {
+			printf("file found, deleting\n");
+			inodeno = le32_to_cpu(dir->inode);
+			if (previous_dir) {
 				uint16_t new_len;
-				printf("file found deleting\n");
 				new_len = le16_to_cpu(previous_dir->direntlen);
 				new_len += le16_to_cpu(dir->direntlen);
 				previous_dir->direntlen = cpu_to_le16(new_len);
-				inodeno = le32_to_cpu(dir->inode);
+			} else {
 				dir->inode = 0;
-				found = 1;
-				break;
 			}
+			found = 1;
+			break;
 		}
 
 		if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
@@ -841,8 +826,7 @@ static int check_filename(char *filename, unsigned int blknr)
 
 
 	if (found == 1) {
-		if (ext4fs_put_metadata(root_first_block_addr,
-					first_block_no_of_root))
+		if (ext4fs_put_metadata(root_first_block_buffer, blknr))
 			goto fail;
 		ret = inodeno;
 	}
@@ -852,7 +836,7 @@ fail:
 	return ret;
 }
 
-int ext4fs_filename_check(char *filename)
+int ext4fs_filename_unlink(char *filename)
 {
 	short direct_blk_idx = 0;
 	long int blknr = -1;
@@ -864,7 +848,7 @@ int ext4fs_filename_check(char *filename)
 		blknr = read_allocated_block(g_parent_inode, direct_blk_idx);
 		if (blknr == 0)
 			break;
-		inodeno = check_filename(filename, blknr);
+		inodeno = unlink_filename(filename, blknr);
 		if (inodeno != -1)
 			return inodeno;
 	}
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 3194595..70a3b5b 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -882,7 +882,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	if (ext4fs_iget(parent_inodeno, g_parent_inode))
 		goto fail;
 	/* check if the filename is already present in root */
-	existing_file_inodeno = ext4fs_filename_check(filename);
+	existing_file_inodeno = ext4fs_filename_unlink(filename);
 	if (existing_file_inodeno != -1) {
 		ret = ext4fs_delete_file(existing_file_inodeno);
 		fs->first_pass_bbmap = 0;
diff --git a/include/ext4fs.h b/include/ext4fs.h
index 13d2c56..e3f6216 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -124,7 +124,7 @@ extern int gindex;
 
 int ext4fs_init(void);
 void ext4fs_deinit(void);
-int ext4fs_filename_check(char *filename);
+int ext4fs_filename_unlink(char *filename);
 int ext4fs_write(const char *fname, unsigned char *buffer,
 		 unsigned long sizebytes);
 int ext4_write_file(const char *filename, void *buf, loff_t offset, loff_t len,
-- 
2.9.3

[U-Boot] [PATCH v3 02/13] ext4: propagate error if creation of directory entry fails

[Stefan Brüns]

In case the dir entry creation failed, ext4fs_write would later overwrite
a random inode, as inodeno was never initialized.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
---
 fs/ext4/ext4_common.c | 12 ++++++------
 fs/ext4/ext4_common.h |  2 +-
 fs/ext4/ext4_write.c  |  4 +++-
 3 files changed, 10 insertions(+), 8 deletions(-)

v2: unchanged
v3: Added Reviewed-by

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 3736533..fd3522e 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -361,7 +361,7 @@ static int check_void_in_dentry(struct ext2_dirent *dir, char *filename)
 	return 0;
 }
 
-void ext4fs_update_parent_dentry(char *filename, int *p_ino, int file_type)
+int ext4fs_update_parent_dentry(char *filename, int file_type)
 {
 	unsigned int *zero_buffer = NULL;
 	char *root_first_block_buffer = NULL;
@@ -375,7 +375,7 @@ void ext4fs_update_parent_dentry(char *filename, int *p_ino, int file_type)
 	unsigned int last_entry_dirlen;
 	int sizeof_void_space = 0;
 	int templength = 0;
-	int inodeno;
+	int inodeno = -1;
 	int status;
 	struct ext_filesystem *fs = get_fs();
 	/* directory entry */
@@ -388,13 +388,13 @@ void ext4fs_update_parent_dentry(char *filename, int *p_ino, int file_type)
 	zero_buffer = zalloc(fs->blksz);
 	if (!zero_buffer) {
 		printf("No Memory\n");
-		return;
+		return -1;
 	}
 	root_first_block_buffer = zalloc(fs->blksz);
 	if (!root_first_block_buffer) {
 		free(zero_buffer);
 		printf("No Memory\n");
-		return;
+		return -1;
 	}
 restart:
 
@@ -513,8 +513,6 @@ restart:
 	temp_dir = temp_dir + sizeof(struct ext2_dirent);
 	memcpy(temp_dir, filename, strlen(filename));
 
-	*p_ino = inodeno;
-
 	/* update or write  the 1st block of root inode */
 	if (ext4fs_put_metadata(root_first_block_buffer,
 				first_block_no_of_root))
@@ -523,6 +521,8 @@ restart:
 fail:
 	free(zero_buffer);
 	free(root_first_block_buffer);
+
+	return inodeno;
 }
 
 static int search_dir(struct ext2_inode *parent_inode, char *dirname)
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
index 370a717..cc9d0c5 100644
--- a/fs/ext4/ext4_common.h
+++ b/fs/ext4/ext4_common.h
@@ -61,7 +61,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
 uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n);
 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);
+int ext4fs_update_parent_dentry(char *filename, int file_type);
 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,
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 70a3b5b..42abd8d 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -907,7 +907,9 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		goto fail;
 	}
 
-	ext4fs_update_parent_dentry(filename, &inodeno, FILETYPE_REG);
+	inodeno = ext4fs_update_parent_dentry(filename, FILETYPE_REG);
+	if (inodeno == -1)
+		goto fail;
 	/* prepare file inode */
 	inode_buffer = zalloc(fs->inodesz);
 	if (!inode_buffer)
-- 
2.9.3

[U-Boot] [PATCH v3 03/13] ext4: Do not crash when trying to grow a directory using extents

[Stefan Brüns]

The following command crashes u-boot:
./sandbox/u-boot -c 'i=0; host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
  while test $i -lt 200 ; do echo $i; setexpr i $i + 1;
  ext4write host 0 0 /foobar${i} 0; done'

Previously, the code updated the direct_block even for extents, and
fortunately crashed before pushing garbage to the disk.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
---
 fs/ext4/ext4_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

v2: unchanged
v3: Added Reviewed-by

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index fd3522e..49d6465 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -448,8 +448,13 @@ restart:
 			    sizeof(struct ext2_dirent) + padding_factor;
 			if ((fs->blksz - totalbytes - last_entry_dirlen) <
 				new_entry_byte_reqd) {
-				printf("1st Block Full:Allocate new block\n");
+				printf("Last Block Full:Allocate new block\n");
 
+				if (le32_to_cpu(g_parent_inode->flags) &
+						EXT4_EXTENTS_FL) {
+					printf("Directory uses extents\n");
+					goto fail;
+				}
 				if (direct_blk_idx == INDIRECT_BLOCKS - 1) {
 					printf("Directory exceeds limit\n");
 					goto fail;
-- 
2.9.3

[U-Boot] [PATCH v3 04/13] ext4: Scan all directory blocks for space when inserting a new entry

[Stefan Brüns]

Previously, only the last directory block was scanned for available space.
Instead, scan all blocks back to front, and if no sufficient space is
found, eventually append a new block.
Blocks are only appended if the directory does not use extents or the new
block would require insertion of indirect blocks, as the old code does.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 72 ++++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 49d6465..96dc371 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -365,14 +365,10 @@ int ext4fs_update_parent_dentry(char *filename, int file_type)
 {
 	unsigned int *zero_buffer = NULL;
 	char *root_first_block_buffer = NULL;
-	int direct_blk_idx;
-	long int root_blknr;
+	int blk_idx;
 	long int first_block_no_of_root = 0;
-	long int previous_blknr = -1;
 	int totalbytes = 0;
-	short int padding_factor = 0;
 	unsigned int new_entry_byte_reqd;
-	unsigned int last_entry_dirlen;
 	int sizeof_void_space = 0;
 	int templength = 0;
 	int inodeno = -1;
@@ -384,6 +380,7 @@ int ext4fs_update_parent_dentry(char *filename, int file_type)
 	uint32_t new_blk_no;
 	uint32_t new_size;
 	uint32_t new_blockcnt;
+	uint32_t directory_blocks;
 
 	zero_buffer = zalloc(fs->blksz);
 	if (!zero_buffer) {
@@ -396,19 +393,16 @@ int ext4fs_update_parent_dentry(char *filename, int file_type)
 		printf("No Memory\n");
 		return -1;
 	}
+	new_entry_byte_reqd = ROUND(strlen(filename) +
+				    sizeof(struct ext2_dirent), 4);
 restart:
+	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
+		LOG2_BLOCK_SIZE(ext4fs_root);
+	blk_idx = directory_blocks - 1;
 
+restart_read:
 	/* read the block no allocated to a file */
-	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
-	     direct_blk_idx++) {
-		root_blknr = read_allocated_block(g_parent_inode,
-						  direct_blk_idx);
-		if (root_blknr == 0) {
-			first_block_no_of_root = previous_blknr;
-			break;
-		}
-		previous_blknr = root_blknr;
-	}
+	first_block_no_of_root = read_allocated_block(g_parent_inode, blk_idx);
 
 	status = ext4fs_devread((lbaint_t)first_block_no_of_root
 				* fs->sect_perblk,
@@ -420,42 +414,33 @@ restart:
 		goto fail;
 	dir = (struct ext2_dirent *)root_first_block_buffer;
 	totalbytes = 0;
+
 	while (le16_to_cpu(dir->direntlen) > 0) {
-		/*
-		 * blocksize-totalbytes because last directory length
-		 * i.e. dir->direntlen is free availble space in the
-		 * block that means  it is a last entry of directory
-		 * entry
-		 */
+		unsigned short used_len = ROUND(dir->namelen +
+		    sizeof(struct ext2_dirent), 4);
 
-		/* traversing the each directory entry */
+		/* last entry of block */
 		if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen)) {
-			if (strlen(filename) % 4 != 0)
-				padding_factor = 4 - (strlen(filename) % 4);
-
-			new_entry_byte_reqd = strlen(filename) +
-			    sizeof(struct ext2_dirent) + padding_factor;
-			padding_factor = 0;
-			/*
-			 * update last directory entry length to its
-			 * length because we are creating new directory
-			 * entry
-			 */
-			if (dir->namelen % 4 != 0)
-				padding_factor = 4 - (dir->namelen % 4);
 
-			last_entry_dirlen = dir->namelen +
-			    sizeof(struct ext2_dirent) + padding_factor;
-			if ((fs->blksz - totalbytes - last_entry_dirlen) <
-				new_entry_byte_reqd) {
-				printf("Last Block Full:Allocate new block\n");
+			/* check if new entry fits */
+			if ((used_len + new_entry_byte_reqd) <=
+			    le16_to_cpu(dir->direntlen)) {
+				dir->direntlen = cpu_to_le16(used_len);
+				break;
+			} else {
+				if (blk_idx > 0) {
+					printf("Block full, trying previous\n");
+					blk_idx--;
+					goto restart_read;
+				}
+				printf("All blocks full: Allocate new\n");
 
 				if (le32_to_cpu(g_parent_inode->flags) &
 						EXT4_EXTENTS_FL) {
 					printf("Directory uses extents\n");
 					goto fail;
 				}
-				if (direct_blk_idx == INDIRECT_BLOCKS - 1) {
+				if (directory_blocks >= INDIRECT_BLOCKS) {
 					printf("Directory exceeds limit\n");
 					goto fail;
 				}
@@ -465,7 +450,8 @@ restart:
 					goto fail;
 				}
 				put_ext4((uint64_t)new_blk_no * fs->blksz, zero_buffer, fs->blksz);
-				g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] =
+				g_parent_inode->b.blocks.
+					dir_blocks[directory_blocks] =
 					cpu_to_le32(new_blk_no);
 
 				new_size = le32_to_cpu(g_parent_inode->size);
@@ -482,8 +468,6 @@ restart:
 					goto fail;
 				goto restart;
 			}
-			dir->direntlen = cpu_to_le16(last_entry_dirlen);
-			break;
 		}
 
 		templength = le16_to_cpu(dir->direntlen);
-- 
2.9.3

[U-Boot] [PATCH v3 04/13] ext4: Scan all directory blocks for space when inserting a new entry

[Lukasz Majewski]

Hi Stefan,

> Previously, only the last directory block was scanned for available
> space. Instead, scan all blocks back to front, and if no sufficient
> space is found, eventually append a new block.
> Blocks are only appended if the directory does not use extents or the
> new block would require insertion of indirect blocks, as the old code
> does.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 72
> ++++++++++++++++++++------------------------------- 1 file changed,
> 28 insertions(+), 44 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 49d6465..96dc371 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -365,14 +365,10 @@ int ext4fs_update_parent_dentry(char *filename,
> int file_type) {
>  	unsigned int *zero_buffer = NULL;
>  	char *root_first_block_buffer = NULL;
> -	int direct_blk_idx;
> -	long int root_blknr;
> +	int blk_idx;
>  	long int first_block_no_of_root = 0;
> -	long int previous_blknr = -1;
>  	int totalbytes = 0;
> -	short int padding_factor = 0;
>  	unsigned int new_entry_byte_reqd;
> -	unsigned int last_entry_dirlen;
>  	int sizeof_void_space = 0;
>  	int templength = 0;
>  	int inodeno = -1;
> @@ -384,6 +380,7 @@ int ext4fs_update_parent_dentry(char *filename,
> int file_type) uint32_t new_blk_no;
>  	uint32_t new_size;
>  	uint32_t new_blockcnt;
> +	uint32_t directory_blocks;
>  
>  	zero_buffer = zalloc(fs->blksz);
>  	if (!zero_buffer) {
> @@ -396,19 +393,16 @@ int ext4fs_update_parent_dentry(char *filename,
> int file_type) printf("No Memory\n");
>  		return -1;
>  	}
> +	new_entry_byte_reqd = ROUND(strlen(filename) +
> +				    sizeof(struct ext2_dirent), 4);
>  restart:
> +	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
> +		LOG2_BLOCK_SIZE(ext4fs_root);
> +	blk_idx = directory_blocks - 1;
>  
> +restart_read:
>  	/* read the block no allocated to a file */
> -	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
> -	     direct_blk_idx++) {
> -		root_blknr = read_allocated_block(g_parent_inode,
> -						  direct_blk_idx);
> -		if (root_blknr == 0) {
> -			first_block_no_of_root = previous_blknr;
> -			break;
> -		}
> -		previous_blknr = root_blknr;
> -	}
> +	first_block_no_of_root =
> read_allocated_block(g_parent_inode, blk_idx); 
>  	status = ext4fs_devread((lbaint_t)first_block_no_of_root
>  				* fs->sect_perblk,
> @@ -420,42 +414,33 @@ restart:
>  		goto fail;
>  	dir = (struct ext2_dirent *)root_first_block_buffer;
>  	totalbytes = 0;
> +
>  	while (le16_to_cpu(dir->direntlen) > 0) {
> -		/*
> -		 * blocksize-totalbytes because last directory length
> -		 * i.e. dir->direntlen is free availble space in the
> -		 * block that means  it is a last entry of directory
> -		 * entry
> -		 */
> +		unsigned short used_len = ROUND(dir->namelen +
> +		    sizeof(struct ext2_dirent), 4);
>  
> -		/* traversing the each directory entry */
> +		/* last entry of block */
>  		if (fs->blksz - totalbytes ==
> le16_to_cpu(dir->direntlen)) {
> -			if (strlen(filename) % 4 != 0)
> -				padding_factor = 4 -
> (strlen(filename) % 4); -
> -			new_entry_byte_reqd = strlen(filename) +
> -			    sizeof(struct ext2_dirent) +
> padding_factor;
> -			padding_factor = 0;
> -			/*
> -			 * update last directory entry length to its
> -			 * length because we are creating new
> directory
> -			 * entry
> -			 */
> -			if (dir->namelen % 4 != 0)
> -				padding_factor = 4 - (dir->namelen %
> 4); 
> -			last_entry_dirlen = dir->namelen +
> -			    sizeof(struct ext2_dirent) +
> padding_factor;
> -			if ((fs->blksz - totalbytes -
> last_entry_dirlen) <
> -				new_entry_byte_reqd) {
> -				printf("Last Block Full:Allocate new
> block\n");
> +			/* check if new entry fits */
> +			if ((used_len + new_entry_byte_reqd) <=
> +			    le16_to_cpu(dir->direntlen)) {
> +				dir->direntlen =
> cpu_to_le16(used_len);
> +				break;
> +			} else {
> +				if (blk_idx > 0) {
> +					printf("Block full, trying
> previous\n");
> +					blk_idx--;
> +					goto restart_read;
> +				}
> +				printf("All blocks full: Allocate
> new\n"); 
>  				if
> (le32_to_cpu(g_parent_inode->flags) & EXT4_EXTENTS_FL) {
>  					printf("Directory uses
> extents\n"); goto fail;
>  				}
> -				if (direct_blk_idx ==
> INDIRECT_BLOCKS - 1) {
> +				if (directory_blocks >=
> INDIRECT_BLOCKS) { printf("Directory exceeds limit\n");
>  					goto fail;
>  				}
> @@ -465,7 +450,8 @@ restart:
>  					goto fail;
>  				}
>  				put_ext4((uint64_t)new_blk_no *
> fs->blksz, zero_buffer, fs->blksz);
> -
> g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] =
> +				g_parent_inode->b.blocks.
> +					dir_blocks[directory_blocks]
> = cpu_to_le32(new_blk_no);
>  
>  				new_size =
> le32_to_cpu(g_parent_inode->size); @@ -482,8 +468,6 @@ restart:
>  					goto fail;
>  				goto restart;
>  			}
> -			dir->direntlen =
> cpu_to_le16(last_entry_dirlen);
> -			break;
>  		}
>  
>  		templength = le16_to_cpu(dir->direntlen);

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 05/13] ext4: Avoid corruption of directories with hash tree indexes

[Stefan Brüns]

While directories can be read using the old linear scan method, adding a
new file would require updating the index tree (alternatively, the whole
tree could be removed).

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
---
 fs/ext4/ext4_write.c | 5 +++++
 include/ext4fs.h     | 1 +
 2 files changed, 6 insertions(+)

v2: unchanged
v3: Added Reviewed-by

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 42abd8d..50c8415 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -881,6 +881,11 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		goto fail;
 	if (ext4fs_iget(parent_inodeno, g_parent_inode))
 		goto fail;
+	/* do not mess up a directory using hash trees */
+	if (le32_to_cpu(g_parent_inode->flags) & EXT4_INDEX_FL) {
+		printf("hash tree directory\n");
+		goto fail;
+	}
 	/* check if the filename is already present in root */
 	existing_file_inodeno = ext4fs_filename_unlink(filename);
 	if (existing_file_inodeno != -1) {
diff --git a/include/ext4fs.h b/include/ext4fs.h
index e3f6216..6e31c73 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -28,6 +28,7 @@
 #define __EXT4__
 #include <ext_common.h>
 
+#define EXT4_INDEX_FL		0x00001000 /* Inode uses hash tree index */
 #define EXT4_EXTENTS_FL		0x00080000 /* Inode uses extents */
 #define EXT4_EXT_MAGIC			0xf30a
 #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM	0x0010
-- 
2.9.3

[U-Boot] [PATCH v3 06/13] ext4: scan all directory blocks when looking up an entry

[Stefan Brüns]

Scanning only the direct blocks of the directory file may falsely report
an existing file as nonexisting, and worse can also lead to creation
of a duplicate entry on file creation.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
---
 fs/ext4/ext4_common.c | 77 ++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

v2: unchanged
v3: Added Reviewed-by
    use parent_inode instead of g_parent_inode when determining directory size

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 96dc371..eae23b7 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -518,64 +518,56 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 {
 	int status;
 	int inodeno = 0;
-	int totalbytes;
-	int templength;
-	int direct_blk_idx;
+	int offset;
+	int blk_idx;
 	long int blknr;
-	char *ptr = NULL;
 	unsigned char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
 	struct ext_filesystem *fs = get_fs();
+	uint32_t directory_blocks;
+	char *direntname;
 
-	/* read the block no allocated to a file */
-	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
-		direct_blk_idx++) {
-		blknr = read_allocated_block(parent_inode, direct_blk_idx);
-		if (blknr == 0)
-			goto fail;
+	directory_blocks = le32_to_cpu(parent_inode->size) >>
+		LOG2_BLOCK_SIZE(ext4fs_root);
+
+	block_buffer = zalloc(fs->blksz);
+	if (!block_buffer)
+		goto fail;
 
-		/* read the blocks of parent inode */
-		block_buffer = zalloc(fs->blksz);
-		if (!block_buffer)
+	/* get the block no allocated to a file */
+	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
+		blknr = read_allocated_block(parent_inode, blk_idx);
+		if (blknr == 0)
 			goto fail;
 
+		/* read the directory block */
 		status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk,
 					0, fs->blksz, (char *)block_buffer);
 		if (status == 0)
 			goto fail;
 
+		offset = 0;
 		dir = (struct ext2_dirent *)block_buffer;
-		ptr = (char *)dir;
-		totalbytes = 0;
-		while (le16_to_cpu(dir->direntlen) >= 0) {
-			/*
-			 * blocksize-totalbytes because last directory
-			 * length i.e.,*dir->direntlen is free availble
-			 * space in the block that means
-			 * it is a last entry of directory entry
-			 */
-			if (dir->inode && (strlen(dirname) == dir->namelen)) {
-				if (strncmp(dirname, ptr + sizeof(struct ext2_dirent), dir->namelen) == 0) {
-					inodeno = le32_to_cpu(dir->inode);
-					break;
-				}
+		direntname = (char *)block_buffer + sizeof(struct ext2_dirent);
+		while (le16_to_cpu(dir->direntlen) >= 8) {
+			if (dir->inode && (strlen(dirname) == dir->namelen) &&
+			    (strncmp(dirname, direntname, dir->namelen) == 0)) {
+				inodeno = le32_to_cpu(dir->inode);
+				break;
 			}
 
-			if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
+			offset += le16_to_cpu(dir->direntlen);
+			if (offset >= fs->blksz)
 				break;
 
-			/* traversing the each directory entry */
-			templength = le16_to_cpu(dir->direntlen);
-			totalbytes = totalbytes + templength;
-			dir = (struct ext2_dirent *)((char *)dir + templength);
-			ptr = (char *)dir;
+			dir = (struct ext2_dirent *)(block_buffer + offset);
+			direntname += offset;
 		}
 
-		free(block_buffer);
-		block_buffer = NULL;
-
-		if (inodeno > 0)
+		if (inodeno > 0) {
+			free(block_buffer);
 			return inodeno;
+		}
 	}
 
 fail:
@@ -827,14 +819,17 @@ fail:
 
 int ext4fs_filename_unlink(char *filename)
 {
-	short direct_blk_idx = 0;
+	int blk_idx;
 	long int blknr = -1;
 	int inodeno = -1;
+	uint32_t directory_blocks;
+
+	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
+		LOG2_BLOCK_SIZE(ext4fs_root);
 
 	/* read the block no allocated to a file */
-	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
-		direct_blk_idx++) {
-		blknr = read_allocated_block(g_parent_inode, direct_blk_idx);
+	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
+		blknr = read_allocated_block(g_parent_inode, blk_idx);
 		if (blknr == 0)
 			break;
 		inodeno = unlink_filename(filename, blknr);
-- 
2.9.3

[U-Boot] [PATCH v3 07/13] ext4: only update number of of unused inodes if GDT_CSUM feature is set

[Stefan Brüns]

e2fsck warns about "Group descriptor 0 marked uninitialized without
feature set."
The bg_itable_unused field is only defined if FEATURE_RO_COMPAT_GDT_CSUM
is set, and should be set (kept) zero otherwise.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index eae23b7..0018937 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -980,12 +980,13 @@ int ext4fs_get_new_inode_no(void)
 	if (!journal_buffer || !zero_buffer)
 		goto fail;
 	struct ext2_block_group *bgd = (struct ext2_block_group *)fs->gdtable;
+	int has_gdt_chksum = le32_to_cpu(fs->sb->feature_ro_compat) &
+		EXT4_FEATURE_RO_COMPAT_GDT_CSUM ? 1 : 0;
 
 	if (fs->first_pass_ibmap == 0) {
 		for (i = 0; i < fs->no_blkgrp; i++) {
 			if (bgd[i].free_inodes) {
-				if (bgd[i].bg_itable_unused !=
-						bgd[i].free_inodes)
+				if (has_gdt_chksum)
 					bgd[i].bg_itable_unused =
 						bgd[i].free_inodes;
 				if (le16_to_cpu(bgd[i].bg_flags) & EXT4_BG_INODE_UNINIT) {
@@ -1006,7 +1007,8 @@ int ext4fs_get_new_inode_no(void)
 							(i * inodes_per_grp);
 				fs->first_pass_ibmap++;
 				ext4fs_bg_free_inodes_dec(&bgd[i]);
-				ext4fs_bg_itable_unused_dec(&bgd[i]);
+				if (has_gdt_chksum)
+					ext4fs_bg_itable_unused_dec(&bgd[i]);
 				ext4fs_sb_free_inodes_dec(fs->sb);
 				status = ext4fs_devread(
 							(lbaint_t)le32_to_cpu(bgd[i].inode_id) *
@@ -1061,12 +1063,10 @@ restart:
 				goto fail;
 			prev_inode_bitmap_index = ibmap_idx;
 		}
-		if (bgd[ibmap_idx].bg_itable_unused !=
-				bgd[ibmap_idx].free_inodes)
+		ext4fs_bg_free_inodes_dec(&bgd[ibmap_idx]);
+		if (has_gdt_chksum)
 			bgd[ibmap_idx].bg_itable_unused =
 					bgd[ibmap_idx].free_inodes;
-		ext4fs_bg_free_inodes_dec(&bgd[ibmap_idx]);
-		ext4fs_bg_itable_unused_dec(&bgd[ibmap_idx]);
 		ext4fs_sb_free_inodes_dec(fs->sb);
 		goto success;
 	}
-- 
2.9.3

[U-Boot] [PATCH v3 07/13] ext4: only update number of of unused inodes if GDT_CSUM feature is set

[Lukasz Majewski]

Hi Stefan,

> e2fsck warns about "Group descriptor 0 marked uninitialized without
> feature set."
> The bg_itable_unused field is only defined if
> FEATURE_RO_COMPAT_GDT_CSUM is set, and should be set (kept) zero
> otherwise.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index eae23b7..0018937 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -980,12 +980,13 @@ int ext4fs_get_new_inode_no(void)
>  	if (!journal_buffer || !zero_buffer)
>  		goto fail;
>  	struct ext2_block_group *bgd = (struct ext2_block_group
> *)fs->gdtable;
> +	int has_gdt_chksum = le32_to_cpu(fs->sb->feature_ro_compat) &
> +		EXT4_FEATURE_RO_COMPAT_GDT_CSUM ? 1 : 0;
>  
>  	if (fs->first_pass_ibmap == 0) {
>  		for (i = 0; i < fs->no_blkgrp; i++) {
>  			if (bgd[i].free_inodes) {
> -				if (bgd[i].bg_itable_unused !=
> -						bgd[i].free_inodes)
> +				if (has_gdt_chksum)
>  					bgd[i].bg_itable_unused =
>  						bgd[i].free_inodes;
>  				if (le16_to_cpu(bgd[i].bg_flags) &
> EXT4_BG_INODE_UNINIT) { @@ -1006,7 +1007,8 @@ int
> ext4fs_get_new_inode_no(void) (i * inodes_per_grp);
>  				fs->first_pass_ibmap++;
>  				ext4fs_bg_free_inodes_dec(&bgd[i]);
> -				ext4fs_bg_itable_unused_dec(&bgd[i]);
> +				if (has_gdt_chksum)
> +
> ext4fs_bg_itable_unused_dec(&bgd[i]);
> ext4fs_sb_free_inodes_dec(fs->sb); status = ext4fs_devread(
>  							(lbaint_t)le32_to_cpu(bgd[i].inode_id)
> * @@ -1061,12 +1063,10 @@ restart:
>  				goto fail;
>  			prev_inode_bitmap_index = ibmap_idx;
>  		}
> -		if (bgd[ibmap_idx].bg_itable_unused !=
> -				bgd[ibmap_idx].free_inodes)
> +		ext4fs_bg_free_inodes_dec(&bgd[ibmap_idx]);
> +		if (has_gdt_chksum)
>  			bgd[ibmap_idx].bg_itable_unused =
>  					bgd[ibmap_idx].free_inodes;
> -		ext4fs_bg_free_inodes_dec(&bgd[ibmap_idx]);
> -		ext4fs_bg_itable_unused_dec(&bgd[ibmap_idx]);
>  		ext4fs_sb_free_inodes_dec(fs->sb);
>  		goto success;
>  	}

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 08/13] ext4: do not clear zalloc'ed buffers a second time

[Stefan Brüns]

zero_buffer is never written, thus clearing it is pointless.
journal_buffer is completely initialized by ext4fs_devread (or in case
of failure, not used).

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 3 ---
 1 file changed, 3 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 0018937..1ebdbe6 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -921,7 +921,6 @@ restart:
 
 		if (le16_to_cpu(bgd[bg_idx].bg_flags) & EXT4_BG_BLOCK_UNINIT) {
 			uint16_t new_flags;
-			memset(zero_buffer, '\0', fs->blksz);
 			put_ext4((uint64_t)le32_to_cpu(bgd[bg_idx].block_id) * fs->blksz,
 				 zero_buffer, fs->blksz);
 			memcpy(fs->blk_bmaps[bg_idx], zero_buffer, fs->blksz);
@@ -938,7 +937,6 @@ restart:
 
 		/* journal backup */
 		if (prev_bg_bitmap_index != bg_idx) {
-			memset(journal_buffer, '\0', fs->blksz);
 			status = ext4fs_devread(
 						(lbaint_t)le32_to_cpu(bgd[bg_idx].block_id)
 						* fs->sect_perblk,
@@ -1032,7 +1030,6 @@ restart:
 		ibmap_idx = fs->curr_inode_no / inodes_per_grp;
 		if (le16_to_cpu(bgd[ibmap_idx].bg_flags) & EXT4_BG_INODE_UNINIT) {
 			int new_flags;
-			memset(zero_buffer, '\0', fs->blksz);
 			put_ext4((uint64_t)le32_to_cpu(bgd[ibmap_idx].inode_id) * fs->blksz,
 				 zero_buffer, fs->blksz);
 			new_flags = le16_to_cpu(bgd[ibmap_idx].bg_flags) & ~EXT4_BG_INODE_UNINIT;
-- 
2.9.3

[U-Boot] [PATCH v3 08/13] ext4: do not clear zalloc'ed buffers a second time

[Lukasz Majewski]

Hi Stefan,

> zero_buffer is never written, thus clearing it is pointless.
> journal_buffer is completely initialized by ext4fs_devread (or in case
> of failure, not used).
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 0018937..1ebdbe6 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -921,7 +921,6 @@ restart:
>  
>  		if (le16_to_cpu(bgd[bg_idx].bg_flags) &
> EXT4_BG_BLOCK_UNINIT) { uint16_t new_flags;
> -			memset(zero_buffer, '\0', fs->blksz);
>  			put_ext4((uint64_t)le32_to_cpu(bgd[bg_idx].block_id)
> * fs->blksz, zero_buffer, fs->blksz);
>  			memcpy(fs->blk_bmaps[bg_idx], zero_buffer,
> fs->blksz); @@ -938,7 +937,6 @@ restart:
>  
>  		/* journal backup */
>  		if (prev_bg_bitmap_index != bg_idx) {
> -			memset(journal_buffer, '\0', fs->blksz);
>  			status = ext4fs_devread(
>  						(lbaint_t)le32_to_cpu(bgd[bg_idx].block_id)
>  						* fs->sect_perblk,
> @@ -1032,7 +1030,6 @@ restart:
>  		ibmap_idx = fs->curr_inode_no / inodes_per_grp;
>  		if (le16_to_cpu(bgd[ibmap_idx].bg_flags) &
> EXT4_BG_INODE_UNINIT) { int new_flags;
> -			memset(zero_buffer, '\0', fs->blksz);
>  			put_ext4((uint64_t)le32_to_cpu(bgd[ibmap_idx].inode_id)
> * fs->blksz, zero_buffer, fs->blksz);
>  			new_flags =
> le16_to_cpu(bgd[ibmap_idx].bg_flags) & ~EXT4_BG_INODE_UNINIT;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 09/13] ext4: After completely filled group, scan next group from the beginning

[Stefan Brüns]

The last free block of a block group may be in its middle. After it has
been allocated, the next block group should be scanned from its beginning.

The following command triggers the bad behaviour (on a blocksize 1024 fs):

./sandbox/u-boot -c 'i=0; host bind 0 ./disk.raw ;
	while test $i -lt 260 ; do echo $i; setexpr i $i + 1;
		ext4write host 0:2 0 /X${i} 0x1450; done ;
	ext4write host 0:2 0 /X240 0x2000 ; '

When 'X240' is extended from 5200 byte to 8192 byte, the new blocks should
start from the first free block (8811), but it uses the blocks 8098-8103
and 16296-16297 -- 8103 + 1 + 8192 = 16296. This can be shown with
debugfs, commands 'ffb' and 'stat X240'.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 1ebdbe6..362668b 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -895,8 +895,8 @@ uint32_t ext4fs_get_new_blk_no(void)
 
 		goto fail;
 	} else {
-restart:
 		fs->curr_blkno++;
+restart:
 		/* get the blockbitmap index respective to blockno */
 		bg_idx = fs->curr_blkno / blk_per_grp;
 		if (fs->blksz == 1024) {
@@ -914,8 +914,9 @@ restart:
 
 		if (bgd[bg_idx].free_blocks == 0) {
 			debug("block group %u is full. Skipping\n", bg_idx);
-			fs->curr_blkno = fs->curr_blkno + blk_per_grp;
-			fs->curr_blkno--;
+			fs->curr_blkno = (bg_idx + 1) * blk_per_grp;
+			if (fs->blksz == 1024)
+				fs->curr_blkno += 1;
 			goto restart;
 		}
 
@@ -932,6 +933,7 @@ restart:
 				   bg_idx) != 0) {
 			debug("going for restart for the block no %ld %u\n",
 			      fs->curr_blkno, bg_idx);
+			fs->curr_blkno++;
 			goto restart;
 		}
 
-- 
2.9.3

[U-Boot] [PATCH v3 09/13] ext4: After completely filled group, scan next group from the beginning

[Lukasz Majewski]

Hi Stefan,

> The last free block of a block group may be in its middle. After it
> has been allocated, the next block group should be scanned from its
> beginning.
> 
> The following command triggers the bad behaviour (on a blocksize 1024
> fs):
> 
> ./sandbox/u-boot -c 'i=0; host bind 0 ./disk.raw ;
> 	while test $i -lt 260 ; do echo $i; setexpr i $i + 1;
> 		ext4write host 0:2 0 /X${i} 0x1450; done ;
> 	ext4write host 0:2 0 /X240 0x2000 ; '
> 
> When 'X240' is extended from 5200 byte to 8192 byte, the new blocks
> should start from the first free block (8811), but it uses the blocks
> 8098-8103 and 16296-16297 -- 8103 + 1 + 8192 = 16296. This can be
> shown with debugfs, commands 'ffb' and 'stat X240'.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 1ebdbe6..362668b 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -895,8 +895,8 @@ uint32_t ext4fs_get_new_blk_no(void)
>  
>  		goto fail;
>  	} else {
> -restart:
>  		fs->curr_blkno++;
> +restart:
>  		/* get the blockbitmap index respective to blockno */
>  		bg_idx = fs->curr_blkno / blk_per_grp;
>  		if (fs->blksz == 1024) {
> @@ -914,8 +914,9 @@ restart:
>  
>  		if (bgd[bg_idx].free_blocks == 0) {
>  			debug("block group %u is full. Skipping\n",
> bg_idx);
> -			fs->curr_blkno = fs->curr_blkno +
> blk_per_grp;
> -			fs->curr_blkno--;
> +			fs->curr_blkno = (bg_idx + 1) * blk_per_grp;
> +			if (fs->blksz == 1024)
> +				fs->curr_blkno += 1;
>  			goto restart;
>  		}
>  
> @@ -932,6 +933,7 @@ restart:
>  				   bg_idx) != 0) {
>  			debug("going for restart for the block no
> %ld %u\n", fs->curr_blkno, bg_idx);
> +			fs->curr_blkno++;
>  			goto restart;
>  		}
>  

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 10/13] ext4: Avoid out-of-bounds access of block bitmap

[Stefan Brüns]

If the blocksize is 1024, count is initialized with 1. Incrementing count
by 8 will never match (count == fs->blksz * 8), and ptr may be
incremented beyond the buffer end if the bitmap is filled. Add the
startblock offset after the loop.

Remove the second loop, as only the first iteration will be done.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 362668b..11da6fa 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -158,18 +158,12 @@ static int _get_new_inode_no(unsigned char *buffer)
 
 static int _get_new_blk_no(unsigned char *buffer)
 {
-	unsigned char input;
-	int operand, status;
+	int operand;
 	int count = 0;
-	int j = 0;
+	int i;
 	unsigned char *ptr = buffer;
 	struct ext_filesystem *fs = get_fs();
 
-	if (fs->blksz != 1024)
-		count = 0;
-	else
-		count = 1;
-
 	while (*ptr == 255) {
 		ptr++;
 		count += 8;
@@ -177,21 +171,17 @@ static int _get_new_blk_no(unsigned char *buffer)
 			return -1;
 	}
 
-	for (j = 0; j < fs->blksz; j++) {
-		input = *ptr;
-		int i = 0;
-		while (i <= 7) {
-			operand = 1 << i;
-			status = input & operand;
-			if (status) {
-				i++;
-				count++;
-			} else {
-				*ptr |= operand;
-				return count;
-			}
+	if (fs->blksz == 1024)
+		count += 1;
+
+	for (i = 0; i <= 7; i++) {
+		operand = 1 << i;
+		if (*ptr & operand) {
+			count++;
+		} else {
+			*ptr |= operand;
+			return count;
 		}
-		ptr = ptr + 1;
 	}
 
 	return -1;
-- 
2.9.3

[U-Boot] [PATCH v3 10/13] ext4: Avoid out-of-bounds access of block bitmap

[Lukasz Majewski]

Hi Stefan,

> If the blocksize is 1024, count is initialized with 1. Incrementing
> count by 8 will never match (count == fs->blksz * 8), and ptr may be
> incremented beyond the buffer end if the bitmap is filled. Add the
> startblock offset after the loop.
> 
> Remove the second loop, as only the first iteration will be done.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 362668b..11da6fa 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -158,18 +158,12 @@ static int _get_new_inode_no(unsigned char
> *buffer) 
>  static int _get_new_blk_no(unsigned char *buffer)
>  {
> -	unsigned char input;
> -	int operand, status;
> +	int operand;
>  	int count = 0;
> -	int j = 0;
> +	int i;
>  	unsigned char *ptr = buffer;
>  	struct ext_filesystem *fs = get_fs();
>  
> -	if (fs->blksz != 1024)
> -		count = 0;
> -	else
> -		count = 1;
> -
>  	while (*ptr == 255) {
>  		ptr++;
>  		count += 8;
> @@ -177,21 +171,17 @@ static int _get_new_blk_no(unsigned char
> *buffer) return -1;
>  	}
>  
> -	for (j = 0; j < fs->blksz; j++) {
> -		input = *ptr;
> -		int i = 0;
> -		while (i <= 7) {
> -			operand = 1 << i;
> -			status = input & operand;
> -			if (status) {
> -				i++;
> -				count++;
> -			} else {
> -				*ptr |= operand;
> -				return count;
> -			}
> +	if (fs->blksz == 1024)
> +		count += 1;
> +
> +	for (i = 0; i <= 7; i++) {
> +		operand = 1 << i;
> +		if (*ptr & operand) {
> +			count++;
> +		} else {
> +			*ptr |= operand;
> +			return count;
>  		}
> -		ptr = ptr + 1;
>  	}
>  
>  	return -1;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 11/13] ext4: Fix memory leak in case of failure

[Stefan Brüns]

temp_ptr should always be freed, even if the function is left via
goto fail.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 50c8415..5e208ef 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -974,7 +974,6 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 			sizeof(struct ext2_inode));
 		if (ext4fs_put_metadata(temp_ptr, parent_itable_blkno))
 			goto fail;
-		free(temp_ptr);
 	} else {
 		/*
 		 * If parent and child fall in same inode table block
@@ -985,7 +984,6 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		gd_index--;
 		if (ext4fs_put_metadata(temp_ptr, itable_blkno))
 			goto fail;
-		free(temp_ptr);
 	}
 	ext4fs_update();
 	ext4fs_deinit();
@@ -996,6 +994,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	fs->curr_inode_no = 0;
 	free(inode_buffer);
 	free(g_parent_inode);
+	free(temp_ptr);
 	g_parent_inode = NULL;
 
 	return 0;
@@ -1003,6 +1002,7 @@ fail:
 	ext4fs_deinit();
 	free(inode_buffer);
 	free(g_parent_inode);
+	free(temp_ptr);
 	g_parent_inode = NULL;
 
 	return -1;
-- 
2.9.3

[U-Boot] [PATCH v3 11/13] ext4: Fix memory leak in case of failure

[Lukasz Majewski]

Hi Stefan,

> temp_ptr should always be freed, even if the function is left via
> goto fail.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_write.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 50c8415..5e208ef 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -974,7 +974,6 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer, sizeof(struct ext2_inode));
>  		if (ext4fs_put_metadata(temp_ptr,
> parent_itable_blkno)) goto fail;
> -		free(temp_ptr);
>  	} else {
>  		/*
>  		 * If parent and child fall in same inode table block
> @@ -985,7 +984,6 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer, gd_index--;
>  		if (ext4fs_put_metadata(temp_ptr, itable_blkno))
>  			goto fail;
> -		free(temp_ptr);
>  	}
>  	ext4fs_update();
>  	ext4fs_deinit();
> @@ -996,6 +994,7 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer, fs->curr_inode_no = 0;
>  	free(inode_buffer);
>  	free(g_parent_inode);
> +	free(temp_ptr);
>  	g_parent_inode = NULL;
>  
>  	return 0;
> @@ -1003,6 +1002,7 @@ fail:
>  	ext4fs_deinit();
>  	free(inode_buffer);
>  	free(g_parent_inode);
> +	free(temp_ptr);
>  	g_parent_inode = NULL;
>  
>  	return -1;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 12/13] ext4: Use correct value for inode size even on revision 0 filesystems

[Stefan Brüns]

fs->inodesz is already correctly (i.e. dependent on fs revision)
initialized in ext4fs_mount.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_write.c | 1 -
 include/ext_common.h | 2 --
 2 files changed, 3 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 5e208ef..81a750b 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -609,7 +609,6 @@ int ext4fs_init(void)
 
 	/* populate fs */
 	fs->blksz = EXT2_BLOCK_SIZE(ext4fs_root);
-	fs->inodesz = INODE_SIZE_FILESYSTEM(ext4fs_root);
 	fs->sect_perblk = fs->blksz >> fs->dev_desc->log2blksz;
 
 	/* get the superblock */
diff --git a/include/ext_common.h b/include/ext_common.h
index 4cd2aa7..25216ca 100644
--- a/include/ext_common.h
+++ b/include/ext_common.h
@@ -52,8 +52,6 @@
 #define LOG2_BLOCK_SIZE(data)	   (le32_to_cpu		   \
 				    (data->sblock.log2_block_size) \
 				    + EXT2_MIN_BLOCK_LOG_SIZE)
-#define INODE_SIZE_FILESYSTEM(data)	(le16_to_cpu \
-			(data->sblock.inode_size))
 
 #define EXT2_FT_DIR	2
 #define SUCCESS	1
-- 
2.9.3

[U-Boot] [PATCH v3 12/13] ext4: Use correct value for inode size even on revision 0 filesystems

[Lukasz Majewski]

Hi Stefan,

> fs->inodesz is already correctly (i.e. dependent on fs revision)
> initialized in ext4fs_mount.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_write.c | 1 -
>  include/ext_common.h | 2 --
>  2 files changed, 3 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 5e208ef..81a750b 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -609,7 +609,6 @@ int ext4fs_init(void)
>  
>  	/* populate fs */
>  	fs->blksz = EXT2_BLOCK_SIZE(ext4fs_root);
> -	fs->inodesz = INODE_SIZE_FILESYSTEM(ext4fs_root);
>  	fs->sect_perblk = fs->blksz >> fs->dev_desc->log2blksz;
>  
>  	/* get the superblock */
> diff --git a/include/ext_common.h b/include/ext_common.h
> index 4cd2aa7..25216ca 100644
> --- a/include/ext_common.h
> +++ b/include/ext_common.h
> @@ -52,8 +52,6 @@
>  #define LOG2_BLOCK_SIZE(data)
> (le32_to_cpu		   \ (data->sblock.log2_block_size) \
>  				    + EXT2_MIN_BLOCK_LOG_SIZE)
> -#define INODE_SIZE_FILESYSTEM(data)	(le16_to_cpu \
> -			(data->sblock.inode_size))
>  
>  #define EXT2_FT_DIR	2
>  #define SUCCESS	1

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 13/13] ext4: initialize full inode for inodes bigger than 128 bytes

[Stefan Brüns]

Make sure the the extra_isize field (offset 128) is initialized to 0 to
mark any extra data as invalid.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_write.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

v3: Patch added to series

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 81a750b..38fbf68 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -454,7 +454,7 @@ static int ext4fs_delete_file(int inodeno)
 		node_inode->data = ext4fs_root;
 		node_inode->ino = inodeno;
 		node_inode->inode_read = 0;
-		memcpy(&(node_inode->inode), &inode, sizeof(struct ext2_inode));
+		memcpy(&(node_inode->inode), &inode, fs->inodesz);
 
 		for (i = 0; i < no_blocks; i++) {
 			blknr = read_allocated_block(&(node_inode->inode), i);
@@ -560,7 +560,7 @@ static int ext4fs_delete_file(int inodeno)
 
 	read_buffer = read_buffer + blkoff;
 	inode_buffer = (struct ext2_inode *)read_buffer;
-	memset(inode_buffer, '\0', sizeof(struct ext2_inode));
+	memset(inode_buffer, '\0', fs->inodesz);
 
 	/* write the inode to original position in inode table */
 	if (ext4fs_put_metadata(start_block_address, blkno))
@@ -866,7 +866,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256);
 	memset(filename, 0x00, 256);
 
-	g_parent_inode = zalloc(sizeof(struct ext2_inode));
+	g_parent_inode = zalloc(fs->inodesz);
 	if (!g_parent_inode)
 		goto fail;
 
@@ -969,8 +969,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		if (ext4fs_log_journal(temp_ptr, parent_itable_blkno))
 			goto fail;
 
-		memcpy(temp_ptr + blkoff, g_parent_inode,
-			sizeof(struct ext2_inode));
+		memcpy(temp_ptr + blkoff, g_parent_inode, fs->inodesz);
 		if (ext4fs_put_metadata(temp_ptr, parent_itable_blkno))
 			goto fail;
 	} else {
@@ -978,8 +977,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		 * If parent and child fall in same inode table block
 		 * both should be kept in 1 buffer
 		 */
-		memcpy(temp_ptr + blkoff, g_parent_inode,
-		       sizeof(struct ext2_inode));
+		memcpy(temp_ptr + blkoff, g_parent_inode, fs->inodesz);
 		gd_index--;
 		if (ext4fs_put_metadata(temp_ptr, itable_blkno))
 			goto fail;
-- 
2.9.3

[U-Boot] [PATCH v3 13/13] ext4: initialize full inode for inodes bigger than 128 bytes

[Lukasz Majewski]

Hi Stefan,

> Make sure the the extra_isize field (offset 128) is initialized to 0
	    ^^^ that ?

> to mark any extra data as invalid.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_write.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 81a750b..38fbf68 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -454,7 +454,7 @@ static int ext4fs_delete_file(int inodeno)
>  		node_inode->data = ext4fs_root;
>  		node_inode->ino = inodeno;
>  		node_inode->inode_read = 0;
> -		memcpy(&(node_inode->inode), &inode, sizeof(struct
> ext2_inode));
> +		memcpy(&(node_inode->inode), &inode, fs->inodesz);
>  
>  		for (i = 0; i < no_blocks; i++) {
>  			blknr =
> read_allocated_block(&(node_inode->inode), i); @@ -560,7 +560,7 @@
> static int ext4fs_delete_file(int inodeno) 
>  	read_buffer = read_buffer + blkoff;
>  	inode_buffer = (struct ext2_inode *)read_buffer;
> -	memset(inode_buffer, '\0', sizeof(struct ext2_inode));
> +	memset(inode_buffer, '\0', fs->inodesz);
>  
>  	/* write the inode to original position in inode table */
>  	if (ext4fs_put_metadata(start_block_address, blkno))
> @@ -866,7 +866,7 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer, ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256);
>  	memset(filename, 0x00, 256);
>  
> -	g_parent_inode = zalloc(sizeof(struct ext2_inode));
> +	g_parent_inode = zalloc(fs->inodesz);
>  	if (!g_parent_inode)
>  		goto fail;
>  
> @@ -969,8 +969,7 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer, if (ext4fs_log_journal(temp_ptr, parent_itable_blkno))
>  			goto fail;
>  
> -		memcpy(temp_ptr + blkoff, g_parent_inode,
> -			sizeof(struct ext2_inode));
> +		memcpy(temp_ptr + blkoff, g_parent_inode,
> fs->inodesz); if (ext4fs_put_metadata(temp_ptr, parent_itable_blkno))
>  			goto fail;
>  	} else {
> @@ -978,8 +977,7 @@ int ext4fs_write(const char *fname, unsigned char
> *buffer,
>  		 * If parent and child fall in same inode table block
>  		 * both should be kept in 1 buffer
>  		 */
> -		memcpy(temp_ptr + blkoff, g_parent_inode,
> -		       sizeof(struct ext2_inode));
> +		memcpy(temp_ptr + blkoff, g_parent_inode,
> fs->inodesz); gd_index--;
>  		if (ext4fs_put_metadata(temp_ptr, itable_blkno))
>  			goto fail;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v3 13/13] ext4: initialize full inode for inodes bigger than 128 bytes

[Stefan Bruens]

On Sonntag, 28. August 2016 22:42:38 CEST you wrote:
> Make sure the the extra_isize field (offset 128) is initialized to 0 to
> mark any extra data as invalid.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_write.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> v3: Patch added to series
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 81a750b..38fbf68 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -454,7 +454,7 @@ static int ext4fs_delete_file(int inodeno)
>  		node_inode->data = ext4fs_root;
>  		node_inode->ino = inodeno;
>  		node_inode->inode_read = 0;
> -		memcpy(&(node_inode->inode), &inode, sizeof(struct ext2_inode));
> +		memcpy(&(node_inode->inode), &inode, fs->inodesz);
This line is actually broken, node_inode->inode is sizeof(struct ext2_inode), 
i.e 128 bytes. It is responsible for the crash Thomas reported.

Kind regards,

Stefan 
-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424