Skip to contentSkip to conversation

ext4: fix possible crash on directory traversal, ignore deleted entries

PATCH New Stefan Brüns 2 participants 32 messages Sep 6, 2016 – Sep 23, 2016 1 message not in the archive ext4 file system

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stefan Brüns

author
#1 lore raw

PATCH v5 1/16 ext4: fix possible crash on directory traversal, ignore deleted entries

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 58 +++++++++++++++++++--------------------------------
 fs/ext4/ext4_write.c  |  2 +-
 include/ext4fs.h      |  2 +-
 3 files changed, 23 insertions(+), 39 deletions(-)
fs/ext4/ext4_common.c +21 -37
index f3235eb..40203cf 100644
@@ -533,16 +533,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 */

@@ -552,7 +550,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;

@@ -572,15 +570,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;
 				}
 			}

@@ -591,19 +583,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:

@@ -779,15 +767,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;

@@ -795,18 +781,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;

@@ -818,19 +801,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))

@@ -846,8 +831,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;
 	}

@@ -857,7 +841,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;

@@ -869,7 +853,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;
 	}
fs/ext4/ext4_write.c +1 -1
index 3194595..70a3b5b 100644
@@ -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;
include/ext4fs.h +1 -1
index 13d2c56..e3f6216 100644
@@ -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,
Signature · 1 line
2.9.3
#2 lore raw

PATCH v5 1/16 ext4: fix possible crash on directory traversal, ignore deleted entries

Quoted 17 lines · 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.

Quoted 6 lines

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/43b95355/attachment-0001.sig>
author
#3 lore raw

PATCH v5 2/16 ext4: propagate error if creation of directory entry fails

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 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(-)
fs/ext4/ext4_common.c +6 -6
index 40203cf..5799bee 100644
@@ -366,7 +366,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;

@@ -380,7 +380,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 */

@@ -393,13 +393,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:
 

@@ -518,8 +518,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))

@@ -528,6 +526,8 @@ restart:
 fail:
 	free(zero_buffer);
 	free(root_first_block_buffer);
+
+	return inodeno;
 }
 
 static int search_dir(struct ext2_inode *parent_inode, char *dirname)
fs/ext4/ext4_common.h +1 -1
index 370a717..cc9d0c5 100644
@@ -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,
fs/ext4/ext4_write.c +3 -1
index 70a3b5b..42abd8d 100644
@@ -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)
Signature · 1 line
2.9.3
#4 lore raw

PATCH v5 2/16 ext4: propagate error if creation of directory entry fails

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/a2f789cc/attachment-0001.sig>
author
#5 lore raw

PATCH v5 3/16 ext4: Do not crash when trying to grow a directory using extents

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/ext4/ext4_common.c +6 -1
index 5799bee..dff03fe 100644
@@ -453,8 +453,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;
Signature · 1 line
2.9.3
#6 lore raw

PATCH v5 3/16 ext4: Do not crash when trying to grow a directory using extents

Quoted 10 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/c54b95ae/attachment-0001.sig>
author
#7 lore raw

PATCH v5 4/16 ext4: Scan all directory blocks for space when inserting a new entry

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 74 +++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 44 deletions(-)
fs/ext4/ext4_common.c +30 -44
index dff03fe..062ea93 100644
@@ -370,14 +370,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;

@@ -389,6 +385,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) {

@@ -401,19 +398,18 @@ 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);
+	if (first_block_no_of_root <= 0)
+		goto fail;
 
 	status = ext4fs_devread((lbaint_t)first_block_no_of_root
 				* fs->sect_perblk,

@@ -425,42 +421,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;
 				}

@@ -470,7 +457,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);

@@ -487,8 +475,6 @@ restart:
 					goto fail;
 				goto restart;
 			}
-			dir->direntlen = cpu_to_le16(last_entry_dirlen);
-			break;
 		}
 
 		templength = le16_to_cpu(dir->direntlen);
Signature · 1 line
2.9.3
#8 lore raw

PATCH v5 4/16 ext4: Scan all directory blocks for space when inserting a new entry

Quoted 8 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/0bf25c7b/attachment-0001.sig>
author
#9 lore raw

PATCH v5 5/16 ext4: Avoid corruption of directories with hash tree indexes

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_write.c | 5 +++++
 include/ext4fs.h     | 1 +
 2 files changed, 6 insertions(+)
fs/ext4/ext4_write.c +5 -0
index 42abd8d..50c8415 100644
@@ -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) {
include/ext4fs.h +1 -0
index e3f6216..6e31c73 100644
@@ -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
Signature · 1 line
2.9.3
#10 lore raw

PATCH v5 5/16 ext4: Avoid corruption of directories with hash tree indexes

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/20565260/attachment-0001.sig>
author
#11 lore raw

PATCH v5 6/16 ext4: Scan all directory blocks when looking up an entry

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 84 ++++++++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 44 deletions(-)
fs/ext4/ext4_common.c +40 -44
index 062ea93..e8e6c00 100644
@@ -525,64 +525,57 @@ 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;
+	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);
 
-		/* read the blocks of parent inode */
-		block_buffer = zalloc(fs->blksz);
-		if (!block_buffer)
+	block_buffer = zalloc(fs->blksz);
+	if (!block_buffer)
+		goto fail;
+
+	/* 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;
 
-		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;
-				}
-			}
+		offset = 0;
+		do {
+			dir = (struct ext2_dirent *)(block_buffer + offset);
+			direntname = (char*)(dir) + sizeof(struct ext2_dirent);
 
-			if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
+			int direntlen = le16_to_cpu(dir->direntlen);
+			if (direntlen < sizeof(struct ext2_dirent))
 				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;
-		}
+			if (dir->inode && (strlen(dirname) == dir->namelen) &&
+			    (strncmp(dirname, direntname, dir->namelen) == 0)) {
+				inodeno = le32_to_cpu(dir->inode);
+				break;
+			}
+
+			offset += direntlen;
 
-		free(block_buffer);
-		block_buffer = NULL;
+		} while (offset < fs->blksz);
 
-		if (inodeno > 0)
+		if (inodeno > 0) {
+			free(block_buffer);
 			return inodeno;
+		}
 	}
 
 fail:

@@ -834,14 +827,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);
Signature · 1 line
2.9.3
#12 lore raw

PATCH v5 6/16 ext4: Scan all directory blocks when looking up an entry

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/e5289d4f/attachment-0001.sig>
author
#13 lore raw

PATCH v5 7/16 ext4: Only update number of of unused inodes if GDT_CSUM feature is set

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
fs/ext4/ext4_common.c +7 -7
index e8e6c00..e58ba36 100644
@@ -988,12 +988,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) {

@@ -1014,7 +1015,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) *

@@ -1069,12 +1071,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;
 	}
Signature · 1 line
2.9.3
#14 lore raw

PATCH v5 7/16 ext4: Only update number of of unused inodes if GDT_CSUM feature is set

Quoted 7 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/807a8707/attachment-0001.sig>
author
#15 lore raw

PATCH v5 8/16 ext4: Do not clear zalloc'ed buffers a second time

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 3 ---
 1 file changed, 3 deletions(-)
fs/ext4/ext4_common.c +0 -3
index e58ba36..539d622 100644
@@ -929,7 +929,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);

@@ -946,7 +945,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,

@@ -1040,7 +1038,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;
Signature · 1 line
2.9.3
#16 lore raw

PATCH v5 8/16 ext4: Do not clear zalloc'ed buffers a second time

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/e6572b95/attachment.sig>
author
#17 lore raw

PATCH v5 9/16 ext4: After completely filled group, scan next group from the beginning

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
fs/ext4/ext4_common.c +5 -3
index 539d622..8fc7559 100644
@@ -903,8 +903,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) {

@@ -922,8 +922,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;
 		}
 

@@ -940,6 +941,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;
 		}
Signature · 1 line
2.9.3
#18 lore raw

PATCH v5 9/16 ext4: After completely filled group, scan next group from the beginning

Quoted 10 lines · 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 ; '
Quoted 7 lines

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/825e8086/attachment.sig>
author
#19 lore raw

PATCH v5 10/16 ext4: Avoid out-of-bounds access of block bitmap

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_common.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)
fs/ext4/ext4_common.c +12 -22
index 8fc7559..72fc4af 100644
@@ -163,18 +163,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;

@@ -182,21 +176,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;
Signature · 1 line
2.9.3
#20 lore raw

PATCH v5 10/16 ext4: Avoid out-of-bounds access of block bitmap

Quoted 9 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/5deefb80/attachment.sig>
author
#21 lore raw

PATCH v5 11/16 ext4: Fix memory leak in case of failure

In reply to a message not in the archive

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

Signed-off-by Stefan Br?ns <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ext4/ext4_write.c +2 -2
index 50c8415..5e208ef 100644
@@ -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;
Signature · 1 line
2.9.3
#22 lore raw

PATCH v5 11/16 ext4: Fix memory leak in case of failure

Quoted 5 lines · Stefan Br?ns

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

Signed-off-by Stefan Br?ns <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/202fb4f4/attachment.sig>
author
#23 lore raw

PATCH v5 12/16 ext4: Use correct value for inode size even on revision 0 filesystems

In reply to a message not in the archive

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

Signed-off-by Stefan Br?ns <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_write.c | 1 -
 include/ext_common.h | 2 --
 2 files changed, 3 deletions(-)
fs/ext4/ext4_write.c +0 -1
index 5e208ef..81a750b 100644
@@ -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 */
include/ext_common.h +0 -2
index 4cd2aa7..25216ca 100644
@@ -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
Signature · 1 line
2.9.3
#24 lore raw

PATCH v5 12/16 ext4: Use correct value for inode size even on revision 0 filesystems

Quoted 5 lines · Stefan Br?ns

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

Signed-off-by Stefan Br?ns <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/09b76b43/attachment.sig>
author
#25 lore raw

PATCH v5 13/16 ext4: initialize full inode for inodes bigger than 128 bytes

In reply to a message not in the archive

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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>
---
 fs/ext4/ext4_write.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
fs/ext4/ext4_write.c +4 -6
index 81a750b..5b518ed 100644
@@ -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;
Signature · 1 line
2.9.3
#26 lore raw

PATCH v5 13/16 ext4: initialize full inode for inodes bigger than 128 bytes

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Lukasz Majewski <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/4bb028da/attachment.sig>
author
#27 lore raw

PATCH v5 14/16 ext4: remove duplicated block release code for extents

In reply to a message not in the archive

The data blocks are identical for files using traditional direct/indirect block allocation scheme and extent trees, thus this code part can be common. Only the code to deallocate the indirect blocks to record the used blocks has to be seperate, respectively the code to release extent tree index blocks.

Actually the code to release the extent tree index blocks is still missing, but at least add a FIXME at the appropriate place.

Signed-off-by Stefan Br?ns <[email protected]>
---
 fs/ext4/ext4_write.c | 110 ++++++++++++++++-----------------------------------
 1 file changed, 33 insertions(+), 77 deletions(-)
fs/ext4/ext4_write.c +33 -77
index 5b518ed..20005f5 100644
@@ -447,92 +447,48 @@ static int ext4fs_delete_file(int inodeno)
 		no_blocks++;
 
 	if (le32_to_cpu(inode.flags) & EXT4_EXTENTS_FL) {
-		struct ext2fs_node *node_inode =
-		    zalloc(sizeof(struct ext2fs_node));
-		if (!node_inode)
-			goto fail;
-		node_inode->data = ext4fs_root;
-		node_inode->ino = inodeno;
-		node_inode->inode_read = 0;
-		memcpy(&(node_inode->inode), &inode, sizeof(struct ext2_inode));
-
-		for (i = 0; i < no_blocks; i++) {
-			blknr = read_allocated_block(&(node_inode->inode), i);
-			bg_idx = blknr / blk_per_grp;
-			if (fs->blksz == 1024) {
-				remainder = blknr % blk_per_grp;
-				if (!remainder)
-					bg_idx--;
-			}
-			ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx],
-						bg_idx);
-			debug("EXT4_EXTENTS Block releasing %ld: %d\n",
-			      blknr, bg_idx);
-
-			ext4fs_bg_free_blocks_inc(&bgd[bg_idx]);
-			ext4fs_sb_free_blocks_inc(fs->sb);
-
-			/* journal backup */
-			if (prev_bg_bmap_idx != bg_idx) {
-				status =
-				    ext4fs_devread(
-						   (lbaint_t)le32_to_cpu(bgd[bg_idx].block_id) *
-						   fs->sect_perblk, 0,
-						   fs->blksz, journal_buffer);
-				if (status == 0)
-					goto fail;
-				if (ext4fs_log_journal(journal_buffer,
-							le32_to_cpu(bgd[bg_idx].block_id)))
-					goto fail;
-				prev_bg_bmap_idx = bg_idx;
-			}
-		}
-		if (node_inode) {
-			free(node_inode);
-			node_inode = NULL;
-		}
+		/* FIXME delete extent index blocks, i.e. eh_depth >= 1 */
+		struct ext4_extent_header *eh =
+			(struct ext4_extent_header *)
+				inode.b.blocks.dir_blocks;
+		debug("del: dep=%d entries=%d\n", eh->eh_depth, eh->eh_entries);
 	} else {
-
 		delete_single_indirect_block(&inode);
 		delete_double_indirect_block(&inode);
 		delete_triple_indirect_block(&inode);
+	}
 
-		/* read the block no allocated to a file */
-		no_blocks = le32_to_cpu(inode.size) / fs->blksz;
-		if (le32_to_cpu(inode.size) % fs->blksz)
-			no_blocks++;
-		for (i = 0; i < no_blocks; i++) {
-			blknr = read_allocated_block(&inode, i);
-			bg_idx = blknr / blk_per_grp;
-			if (fs->blksz == 1024) {
-				remainder = blknr % blk_per_grp;
-				if (!remainder)
-					bg_idx--;
-			}
-			ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx],
-						bg_idx);
-			debug("ActualB releasing %ld: %d\n", blknr, bg_idx);
+	/* release data blocks */
+	for (i = 0; i < no_blocks; i++) {
+		blknr = read_allocated_block(&inode, i);
+		bg_idx = blknr / blk_per_grp;
+		if (fs->blksz == 1024) {
+			remainder = blknr % blk_per_grp;
+			if (!remainder)
+				bg_idx--;
+		}
+		ext4fs_reset_block_bmap(blknr, fs->blk_bmaps[bg_idx],
+					bg_idx);
+		debug("EXT4 Block releasing %ld: %d\n", blknr, bg_idx);
 
-			ext4fs_bg_free_blocks_inc(&bgd[bg_idx]);
-			ext4fs_sb_free_blocks_inc(fs->sb);
-			/* journal backup */
-			if (prev_bg_bmap_idx != bg_idx) {
-				memset(journal_buffer, '\0', fs->blksz);
-				status = ext4fs_devread(
-							(lbaint_t)le32_to_cpu(bgd[bg_idx].block_id)
-							* fs->sect_perblk,
-							0, fs->blksz,
-							journal_buffer);
-				if (status == 0)
-					goto fail;
-				if (ext4fs_log_journal(journal_buffer,
-						le32_to_cpu(bgd[bg_idx].block_id)))
-					goto fail;
-				prev_bg_bmap_idx = bg_idx;
-			}
+		ext4fs_bg_free_blocks_inc(&bgd[bg_idx]);
+		ext4fs_sb_free_blocks_inc(fs->sb);
+		/* journal backup */
+		if (prev_bg_bmap_idx != bg_idx) {
+			uint32_t bgd_blknr = le32_to_cpu(bgd[bg_idx].block_id);
+			status = ext4fs_devread((lbaint_t)bgd_blknr *
+						fs->sect_perblk,
+						0, fs->blksz,
+						journal_buffer);
+			if (status == 0)
+				goto fail;
+			if (ext4fs_log_journal(journal_buffer, bgd_blknr))
+				goto fail;
+			prev_bg_bmap_idx = bg_idx;
 		}
 	}
 
+	/* release inode */
 	/* from the inode no to blockno */
 	inodes_per_block = fs->blksz / fs->inodesz;
 	ibmap_idx = inodeno / inode_per_grp;
Signature · 1 line
2.9.3
#28 lore raw

PATCH v5 14/16 ext4: remove duplicated block release code for extents

Quoted 10 lines · Stefan Br?ns

The data blocks are identical for files using traditional direct/indirect block allocation scheme and extent trees, thus this code part can be common. Only the code to deallocate the indirect blocks to record the used blocks has to be seperate, respectively the code to release extent tree index blocks.

Actually the code to release the extent tree index blocks is still missing, but at least add a FIXME at the appropriate place.

Signed-off-by Stefan Br?ns <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/b2dee734/attachment.sig>
author
#29 lore raw

PATCH v5 15/16 ext4: Correct block number handling, empty block vs. error code

In reply to a message not in the archive

read_allocated block may return block number 0, which is just an indicator a chunk of the file is not backed by a block, i.e. it is sparse.

During file deletions, just continue with the next logical block, for other operations treat blocknumber <= 0 as an error.

For writes, blocknumber 0 should never happen, as U-Boot always allocates blocks for the whole file. Reading already handles this correctly, i.e. the read buffer is 0-fillled.

Not treating block 0 as sparse block leads to FS corruption, e.g.

	./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
		ext4write host 0 0 /2.5GB.file 1 '

The 2.5GB.file from the fs test is actually a sparse file.

Signed-off-by Stefan Br?ns <[email protected]>
---
 fs/ext4/ext4_common.c |  6 +++---
 fs/ext4/ext4_write.c  | 11 ++++++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)
fs/ext4/ext4_common.c +3 -3
index 72fc4af..a78b0b8 100644
@@ -534,7 +534,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 	/* 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)
+		if (blknr <= 0)
 			goto fail;
 
 		/* read the directory block */

@@ -828,7 +828,7 @@ int ext4fs_filename_unlink(char *filename)
 	/* read the block no allocated to a file */
 	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
 		blknr = read_allocated_block(g_parent_inode, blk_idx);
-		if (blknr == 0)
+		if (blknr <= 0)
 			break;
 		inodeno = unlink_filename(filename, blknr);
 		if (inodeno != -1)

@@ -1590,7 +1590,7 @@ long int read_allocated_block(struct ext2_inode *inode, int fileblock)
 			if (status == 0) {
 				printf("** SI ext2fs read block (indir 1)"
 					"failed. **\n");
-				return 0;
+				return -1;
 			}
 			ext4fs_indir1_blkno =
 				le32_to_cpu(inode->b.blocks.
fs/ext4/ext4_write.c +10 -1
index 20005f5..b3ea07b 100644
@@ -461,6 +461,10 @@ static int ext4fs_delete_file(int inodeno)
 	/* release data blocks */
 	for (i = 0; i < no_blocks; i++) {
 		blknr = read_allocated_block(&inode, i);
+		if (blknr == 0)
+			continue;
+		if (blknr < 0)
+			goto fail;
 		bg_idx = blknr / blk_per_grp;
 		if (fs->blksz == 1024) {
 			remainder = blknr % blk_per_grp;

@@ -718,6 +722,10 @@ void ext4fs_deinit(void)
 	fs->curr_blkno = 0;
 }
 
+/*
+ * Write data to filesystem blocks. Uses same optimization for
+ * contigous sectors as ext4fs_read_file
+ */
 static int ext4fs_write_file(struct ext2_inode *file_inode,
 			     int pos, unsigned int len, char *buf)
 {

@@ -744,7 +752,7 @@ static int ext4fs_write_file(struct ext2_inode *file_inode,
 		int blockend = fs->blksz;
 		int skipfirst = 0;
 		blknr = read_allocated_block(file_inode, i);
-		if (blknr < 0)
+		if (blknr <= 0)
 			return -1;
 
 		blknr = blknr << log2_fs_blocksize;

@@ -910,6 +918,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	/* copy the file content into data blocks */
 	if (ext4fs_write_file(file_inode, 0, sizebytes, (char *)buffer) == -1) {
 		printf("Error in copying content\n");
+		/* FIXME: Deallocate data blocks */
 		goto fail;
 	}
 	ibmap_idx = parent_inodeno / le32_to_cpu(ext4fs_root->sblock.inodes_per_group);
Signature · 1 line
2.9.3
#30 lore raw

PATCH v5 15/16 ext4: Correct block number handling, empty block vs. error code

Quoted 10 lines · Stefan Br?ns

read_allocated block may return block number 0, which is just an indicator a chunk of the file is not backed by a block, i.e. it is sparse.

During file deletions, just continue with the next logical block, for other operations treat blocknumber <= 0 as an error.

For writes, blocknumber 0 should never happen, as U-Boot always allocates blocks for the whole file. Reading already handles this correctly, i.e. the read buffer is 0-fillled.

Quoted 6 lines

Not treating block 0 as sparse block leads to FS corruption, e.g.

	./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
		ext4write host 0 0 /2.5GB.file 1 '

The 2.5GB.file from the fs test is actually a sparse file.

Signed-off-by Stefan Br?ns <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/6ee2ea98/attachment.sig>
author
#31 lore raw

PATCH v5 16/16 ext4: Fix memory leak of journal buffer if block is updated multiple times

In reply to a message not in the archive

If the same block is updated multiple times in a row during a single file system operation, gd_index is decremented to use the same journal entry again. Avoid loosing the already allocated buffer.

Signed-off-by Stefan Br?ns <[email protected]>
---
 fs/ext4/ext4_journal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
fs/ext4/ext4_journal.c +5 -1
index cf14049..5a25be4 100644
@@ -190,7 +190,11 @@ int ext4fs_put_metadata(char *metadata_buffer, uint32_t blknr)
 		printf("Invalid input arguments %s\n", __func__);
 		return -EINVAL;
 	}
-	dirty_block_ptr[gd_index]->buf = zalloc(fs->blksz);
+	if (dirty_block_ptr[gd_index]->buf)
+		assert(dirty_block_ptr[gd_index]->blknr == blknr);
+	else
+		dirty_block_ptr[gd_index]->buf = zalloc(fs->blksz);
+
 	if (!dirty_block_ptr[gd_index]->buf)
 		return -ENOMEM;
 	memcpy(dirty_block_ptr[gd_index]->buf, metadata_buffer, fs->blksz);
Signature · 1 line
2.9.3
#32 lore raw

PATCH v5 16/16 ext4: Fix memory leak of journal buffer if block is updated multiple times

Quoted 5 lines · Stefan Br?ns

If the same block is updated multiple times in a row during a single file system operation, gd_index is decremented to use the same journal entry again. Avoid loosing the already allocated buffer.

Signed-off-by Stefan Br?ns <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/3bfaa69f/attachment.sig>