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

PATCH Superseded Stefan Brüns 2 participants 22 messages Aug 28, 2016 – Sep 5, 2016 2 messages 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

2016-08-28 20:42 UTC #1 lore raw

PATCH v3 1/13 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(-)

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

fs/ext4/ext4_common.c +21 ~~-37~~

index 4a003cf..3736533 100644
@@ -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;
 	}

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

Stefan Brüns

author

2016-08-28 20:42 UTC #2 lore raw

PATCH v3 2/13 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(-)

v2: unchanged
v3: Added Reviewed-by

fs/ext4/ext4_common.c +6 -6

index 3736533..fd3522e 100644
@@ -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)

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

Stefan Brüns

author

2016-08-28 20:42 UTC #3 lore raw

PATCH v3 3/13 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(-)

v2: unchanged
v3: Added Reviewed-by

fs/ext4/ext4_common.c +6 -1

index fd3522e..49d6465 100644
@@ -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;

Signature · 1 line

2.9.3

Stefan Brüns

author

2016-08-28 20:42 UTC #4 lore raw

PATCH v3 4/13 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]>

---
 fs/ext4/ext4_common.c | 72 ++++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +28 ~~-44~~

index 49d6465..96dc371 100644
@@ -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);

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 13:56 UTC #5 lore raw

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

Hi Stefan,

Quoted 157 lines

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

---
 fs/ext4/ext4_common.c | 72

++++++++++++++++++++------------------------------- 1 file changed, 28 insertions(+), 44 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +13 ~~-24~~

index 49d6465..96dc371 100644
@@ -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); -

fs/ext4/ext4_common.c +0 -2

-			new_entry_byte_reqd = strlen(filename) +
-			    sizeof(struct ext2_dirent) +

padding_factor;

fs/ext4/ext4_common.c +0 -4

-			padding_factor = 0;
-			/*
-			 * update last directory entry length to its
-			 * length because we are creating new

Stefan Brüns

author

2016-08-28 20:42 UTC #6 lore raw

PATCH v3 5/13 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(+)

v2: unchanged
v3: Added Reviewed-by

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

Stefan Brüns

author

2016-08-28 20:42 UTC #7 lore raw

PATCH v3 6/13 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 | 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

fs/ext4/ext4_common.c +36 ~~-41~~

index 96dc371..eae23b7 100644
@@ -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);

Signature · 1 line

2.9.3

Stefan Brüns

author

2016-08-28 20:42 UTC #8 lore raw

PATCH v3 7/13 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]>

---
 fs/ext4/ext4_common.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +7 -7

index eae23b7..0018937 100644
@@ -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;
 	}

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 14:03 UTC #9 lore raw

PATCH v3 7/13 ext4: only update number of of unused inodes if GDT_CSUM feature is set

Hi Stefan,

Quoted 49 lines

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]>
---
 fs/ext4/ext4_common.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
v3: Patch added to series
fs/ext4/ext4_common.c +3 -2
index eae23b7..0018937 100644
@@ -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/ext4/ext4_common.c +2 -1
 				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:
fs/ext4/ext4_common.c +0 -1
 				goto fail;
 			prev_inode_bitmap_index = ibmap_idx;
 		}
-		if (bgd[ibmap_idx].bg_itable_unused !=

fs/ext4/ext4_common.c restart: · line 1072

Quoted 10 lines

fs/ext4/ext4_common.c +2 -3

-				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 <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #10 lore raw

PATCH v3 8/13 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]>

---
 fs/ext4/ext4_common.c | 3 ---
 1 file changed, 3 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +0 -3

index 0018937..1ebdbe6 100644
@@ -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;

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 14:04 UTC #11 lore raw

PATCH v3 8/13 ext4: do not clear zalloc'ed buffers a second time

Hi Stefan,

Quoted 39 lines

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]>

---
 fs/ext4/ext4_common.c | 3 ---
 1 file changed, 3 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +0 -1

index 0018937..1ebdbe6 100644
@@ -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:

fs/ext4/ext4_common.c +0 -2

 		/* 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 =

Quoted 1 line

le16_to_cpu(bgd[ibmap_idx].bg_flags) & ~EXT4_BG_INODE_UNINIT;

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #12 lore raw

PATCH v3 9/13 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]>

---
 fs/ext4/ext4_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +5 -3

index 1ebdbe6..362668b 100644
@@ -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;
 		}

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 14:06 UTC #13 lore raw

PATCH v3 9/13 ext4: After completely filled group, scan next group from the beginning

Hi Stefan,

Quoted 53 lines

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]>
---
 fs/ext4/ext4_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
v3: Patch added to series
fs/ext4/ext4_common.c +4 -3
index 1ebdbe6..362668b 100644
@@ -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;
fs/ext4/ext4_common.c +0 -0
 		}

fs/ext4/ext4_common.c restart: · line 939

Quoted 7 lines

fs/ext4/ext4_common.c +1 -0

@@ -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 <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #14 lore raw

PATCH v3 10/13 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]>

---
 fs/ext4/ext4_common.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +12 ~~-22~~

index 362668b..11da6fa 100644
@@ -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;

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 14:08 UTC #15 lore raw

PATCH v3 10/13 ext4: Avoid out-of-bounds access of block bitmap

Hi Stefan,

Quoted 61 lines

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

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

---
 fs/ext4/ext4_common.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

v3: Patch added to series

fs/ext4/ext4_common.c +7 ~~-21~~

index 362668b..11da6fa 100644
@@ -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;

fs/ext4/ext4_common.c static int _get_new_blk_no(unsigned char · line 187

Quoted 10 lines

fs/ext4/ext4_common.c +5 -1

+		if (*ptr & operand) {
+			count++;
+		} else {
+			*ptr |= operand;
+			return count;
 		}
-		ptr = ptr + 1;
 	}
 
 	return -1;

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #16 lore raw

PATCH v3 11/13 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]>

---
 fs/ext4/ext4_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

v3: Patch added to series

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

Lukasz Majewski

2016-08-29 14:09 UTC #17 lore raw

PATCH v3 11/13 ext4: Fix memory leak in case of failure

Hi Stefan,

Quoted 39 lines

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

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

---
 fs/ext4/ext4_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

v3: Patch added to series

fs/ext4/ext4_write.c +1 -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;

fs/ext4/ext4_write.c fail: · line 1008

Quoted 7 lines

fs/ext4/ext4_write.c +1 -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 <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #18 lore raw

PATCH v3 12/13 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]>

---
 fs/ext4/ext4_write.c | 1 -
 include/ext_common.h | 2 --
 2 files changed, 3 deletions(-)

v3: Patch added to series

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

Lukasz Majewski

2016-08-29 14:09 UTC #19 lore raw

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

Hi Stefan,

Quoted 28 lines

fs->inodesz is already correctly (i.e. dependent on fs revision) initialized in ext4fs_mount.
Signed-off-by Stefan Br?ns <[email protected]>
---
 fs/ext4/ext4_write.c | 1 -
 include/ext_common.h | 2 --
 2 files changed, 3 deletions(-)
v3: Patch added to series
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 -0
index 4cd2aa7..25216ca 100644

include/ext_common.h line 57

Quoted 8 lines

include/ext_common.h +0 -2

@@ -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 <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Stefan Brüns

author

2016-08-28 20:42 UTC #20 lore raw

PATCH v3 13/13 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]>

---
 fs/ext4/ext4_write.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

v3: Patch added to series

fs/ext4/ext4_write.c +5 -7

index 81a750b..38fbf68 100644
@@ -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;

Signature · 1 line

2.9.3

Lukasz Majewski

2016-08-29 14:11 UTC #21 lore raw

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

Hi Stefan,

Quoted 1 line

Make sure the the extra_isize field (offset 128) is initialized to 0

	    ^^^ that ?

Quoted 61 lines

to mark any extra data as invalid.

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

---
 fs/ext4/ext4_write.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

v3: Patch added to series

fs/ext4/ext4_write.c +1 -1

index 81a750b..38fbf68 100644
@@ -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)

fs/ext4/ext4_write.c +4 -6

 	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))

Quoted 1 line

goto fail;

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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

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

Stefan Bruens

author

2016-09-05 23:56 UTC #23 lore raw

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

Quoted 15 lines · 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 <[email protected]>
---
 fs/ext4/ext4_write.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
v3: Patch added to series
fs/ext4/ext4_write.c +0 -0
index 81a750b..38fbf68 100644

fs/ext4/ext4_write.c static int ext4fs_delete_file(int inodeno) · line 457

Quoted 5 lines

fs/ext4/ext4_write.c +1 -1

@@ -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

Signature · 3 lines

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