ext4: lost brelse in __ext4_read_dirblock()

PATCH Applied Vasily Averin 3 participants 18 messages Oct 31, 2018 – Nov 8, 2018 1 message not in the archive ext4 file system

lore

Share & tools

lore

Vasily Averin

author

2018-10-31 19:12 UTC #1 lore raw

PATCH 1/7 ext4: lost brelse in __ext4_read_dirblock()

In reply to a message not in the archive

Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...") # 3.9

Signed-off-by Vasily Averin <[email protected]>

---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)

fs/ext4/namei.c +1 -0

index d388cce72db2..1a21e59ede72 100644
@@ -111,6 +111,7 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	if (!bh) {
 		ext4_error_inode(inode, func, line, block,
 				 "Directory hole found");
+		brelse(bh);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	dirent = (struct ext4_dir_entry *) bh->b_data;

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:12 UTC #6 lore raw

PATCH 2/7 ext4: possible sbi->s_group_desc leak in ext4_fill_super

In reply to a message not in the archive

Fixes bfe0a5f47ada ("ext4: add more mount time checks of the superblock") # 4.18

Signed-off-by Vasily Averin <[email protected]>

---
 fs/ext4/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/ext4/super.c +2 -1

index a221f1cdf704..ed4d36506ec2 100644
@@ -4100,7 +4100,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			 le32_to_cpu(es->s_inodes_count),
 			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
 		ret = -EINVAL;
-		goto failed_mount;
+		db_count = 0;
+		goto failed_mount2;
 	}
 
 	bgl_lock_init(sbi->s_blockgroup_lock);

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:12 UTC #8 lore raw

PATCH 3/7 ext4: lost release of s_journal_flag_rwsem on rollback in ext4_fill_super

In reply to a message not in the archive

Fixes c8585c6fcaf2 ("ext4: fix races between changing inode journal ...") # 4.7

cc Daeho Jeong <[email protected]>

Signed-off-by Vasily Averin <[email protected]>

---
 fs/ext4/super.c | 1 +
 1 file changed, 1 insertion(+)

fs/ext4/super.c +1 -0

index ed4d36506ec2..0c9e165b4e3f 100644
@@ -4511,6 +4511,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	percpu_counter_destroy(&sbi->s_freeinodes_counter);
 	percpu_counter_destroy(&sbi->s_dirs_counter);
 	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
+	percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
 failed_mount5:
 	ext4_ext_release(sb);
 	ext4_release_system_zone(sb);

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:12 UTC #10 lore raw

PATCH 4/7 ext4: lost brelse in ext4_xattr_get_block()

In reply to a message not in the archive

Fixes dec214d00e0d ("ext4: xattr inode deduplication") # 4.13

Signed-off-by Vasily Averin <[email protected]>

---
 fs/ext4/xattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/ext4/xattr.c +3 -1

index dc1aeab06dba..07c3a115f7ae 100644
@@ -2272,8 +2272,10 @@ static struct buffer_head *ext4_xattr_get_block(struct inode *inode)
 	if (!bh)
 		return ERR_PTR(-EIO);
 	error = ext4_xattr_check_block(inode, bh);
-	if (error)
+	if (error) {
+		brelse(bh);
 		return ERR_PTR(error);
+	}
 	return bh;
 }

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:12 UTC #12 lore raw

PATCH 5/7 ext4: bs.bh cleanup before re-using in ext4_xattr_block_find()

In reply to a message not in the archive

bs.bh was taken in previous ext4_xattr_block_find() call, it should be released before re-using

Fixes 7e01c8e5420b ("ext3/4: fix uninitialized bs in ...") # 2.6.26
cc: Tiger Yang <[email protected]>

Signed-off-by Vasily Averin <[email protected]>

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

fs/ext4/xattr.c +2 -0

index 07c3a115f7ae..07b9a335c8eb 100644
@@ -2395,6 +2395,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 			error = ext4_xattr_block_set(handle, inode, &i, &bs);
 		} else if (error == -ENOSPC) {
 			if (EXT4_I(inode)->i_file_acl && !bs.s.base) {
+				brelse(bs.bh);
+				bs.bh = NULL;
 				error = ext4_xattr_block_find(inode, &i, &bs);
 				if (error)
 					goto cleanup;

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:13 UTC #14 lore raw

PATCH 6/7 ext4: lost brelse in ext4_xattr_move_to_block()

In reply to a message not in the archive

Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
cc: Jan Kara <[email protected]>
however issue was present in original ext4_expand_extra_isize_ea() Fixes 6dd4ee7cab7e ("ext4: Expand extra_inodes space per ...") # 2.6.23
cc: Kalpak Shah <[email protected]>

Signed-off-by Vasily Averin <[email protected]>

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

fs/ext4/xattr.c +2 -0

index 07b9a335c8eb..5c9bc0d85cc0 100644
@@ -2617,6 +2617,8 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 	kfree(buffer);
 	if (is)
 		brelse(is->iloc.bh);
+	if (bs)
+		brelse(bs->bh);
 	kfree(is);
 	kfree(bs);

Signature · 1 line

2.17.1

Vasily Averin

author

2018-10-31 19:13 UTC #17 lore raw

PATCH 7/7 ext4: lost brelse in ext4_expand_extra_isize_ea()

In reply to a message not in the archive

Fixes de05ca852679 ("ext4: move call to ext4_error() into ...") # 4.17

Signed-off-by Vasily Averin <[email protected]>

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

fs/ext4/xattr.c +5 -2

index 5c9bc0d85cc0..0b9688683526 100644
@@ -2698,7 +2698,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 			       struct ext4_inode *raw_inode, handle_t *handle)
 {
 	struct ext4_xattr_ibody_header *header;
-	struct buffer_head *bh;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	static unsigned int mnt_count;
 	size_t min_offs;

@@ -2739,13 +2738,17 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	 * EA block can hold new_extra_isize bytes.
 	 */
 	if (EXT4_I(inode)->i_file_acl) {
+		struct buffer_head *bh;
+
 		bh = sb_bread(inode->i_sb, EXT4_I(inode)->i_file_acl);
 		error = -EIO;
 		if (!bh)
 			goto cleanup;
 		error = ext4_xattr_check_block(inode, bh);
-		if (error)
+		if (error) {
+			brelse(bh);
 			goto cleanup;
+		}
 		base = BHDR(bh);
 		end = bh->b_data + bh->b_size;
 		min_offs = end - base;

Signature · 1 line

2.17.1

Jan Kara

reviewer Reviewed-by on v1 6/7

2018-11-07 13:50 UTC #15 lore raw

Re: [PATCH 6/7] ext4: lost brelse in ext4_xattr_move_to_block()

In reply to Vasily Averin

Quoted 7 lines · Vasily Averin

Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
cc: Jan Kara <[email protected]>
however issue was present in original ext4_expand_extra_isize_ea() Fixes 6dd4ee7cab7e ("ext4: Expand extra_inodes space per ...") # 2.6.23
cc: Kalpak Shah <[email protected]>
Signed-off-by Vasily Averin <[email protected]>

Good catch. Thanks for the fix. The patch looks good. You can add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 20 lines

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

fs/ext4/xattr.c +2 -0

index 07b9a335c8eb..5c9bc0d85cc0 100644
@@ -2617,6 +2617,8 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 	kfree(buffer);
 	if (is)
 		brelse(is->iloc.bh);
+	if (bs)
+		brelse(bs->bh);
 	kfree(is);
 	kfree(bs);

Signature · 1 line

2.17.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Theodore Y. Ts'o

maintainer

2018-11-07 15:25 UTC #2 lore raw

In reply to Vasily Averin

Quoted 12 lines · Vasily Averin

Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...") # 3.9
Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)
fs/ext4/namei.c +0 -0
index d388cce72db2..1a21e59ede72 100644

fs/ext4/namei.c static struct buffer_head *__ext4_read_dirblock(struct inode *inode, · line 117

Quoted 7 lines

fs/ext4/namei.c +1 -0

@@ -111,6 +111,7 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	if (!bh) {
 		ext4_error_inode(inode, func, line, block,
 				 "Directory hole found");
+		brelse(bh);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	dirent = (struct ext4_dir_entry *) bh->b_data;

Inside the conditional, bh == NULL, so the brelse isn't needed.

Cheers,

- Ted

Theodore Y. Ts'o

maintainer

2018-11-07 15:33 UTC #7 lore raw

Re: [PATCH 2/7] ext4: possible sbi->s_group_desc leak in ext4_fill_super

In reply to Vasily Averin

Quoted 12 lines · Vasily Averin

Fixes bfe0a5f47ada ("ext4: add more mount time checks of the superblock") # 4.18
Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/ext4/super.c +0 -0
index a221f1cdf704..ed4d36506ec2 100644

fs/ext4/super.c static int ext4_fill_super(struct super_block *sb, void *data, int silent) · line 4107

Quoted 9 lines

fs/ext4/super.c +2 -1

@@ -4100,7 +4100,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			 le32_to_cpu(es->s_inodes_count),
 			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
 		ret = -EINVAL;
-		goto failed_mount;
+		db_count = 0;
+		goto failed_mount2;
 	}
 
 	bgl_lock_init(sbi->s_blockgroup_lock);

Thanks for spotting this!

A better fix would be to move this check earlier, before the allocation of sbi->s_group_desc.

- Ted

commit 9fa8bc9b9d3c6864f8dddd83893a02fcc1b48b62

Author: Theodore Ts'o <[email protected]>
Date:   Wed Nov 7 10:32:53 2018 -0500

ext4: fix possible leak of sbi->s_group_desc_leak in error path

Fixes bfe0a5f47ada ("ext4: add more mount time checks of the superblock")

Reported-by Vasily Averin <[email protected]>

Signed-off-by Theodore Ts'o <[email protected]>

Cc [email protected] # 4.18

fs/ext4/super.c +8 -8

index a221f1cdf704..92092b55db1e 100644
@@ -4075,6 +4075,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	sbi->s_groups_count = blocks_count;
 	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
 			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
+	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
+	    le32_to_cpu(es->s_inodes_count)) {
+		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
+			 le32_to_cpu(es->s_inodes_count),
+			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
+		ret = -EINVAL;
+		goto failed_mount;
+	}
 	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
 		   EXT4_DESC_PER_BLOCK(sb);
 	if (ext4_has_feature_meta_bg(sb)) {

@@ -4094,14 +4102,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		ret = -ENOMEM;
 		goto failed_mount;
 	}
-	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
-	    le32_to_cpu(es->s_inodes_count)) {
-		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
-			 le32_to_cpu(es->s_inodes_count),
-			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
-		ret = -EINVAL;
-		goto failed_mount;
-	}
 
 	bgl_lock_init(sbi->s_blockgroup_lock);

Theodore Y. Ts'o

maintainer Applied

2018-11-07 15:57 UTC #9 lore raw

Re: [PATCH 3/7] ext4: lost release of s_journal_flag_rwsem on rollback in ext4_fill_super

In reply to Vasily Averin

Quoted 4 lines · Vasily Averin

Fixes c8585c6fcaf2 ("ext4: fix races between changing inode journal ...") # 4.7
cc Daeho Jeong <[email protected]>
Signed-off-by Vasily Averin <[email protected]>

Thanks, applied.

- Ted

Theodore Y. Ts'o

maintainer Applied

2018-11-07 16:02 UTC #11 lore raw

Re: [PATCH 4/7] ext4: lost brelse in ext4_xattr_get_block()

In reply to Vasily Averin

Quoted 3 lines · Vasily Averin

Fixes dec214d00e0d ("ext4: xattr inode deduplication") # 4.13
Signed-off-by Vasily Averin <[email protected]>

Thanks, applied. I used the description line:

ext4: fix buffer leak in ext4_xattr_get_block() on error path

- Ted

Theodore Y. Ts'o

maintainer Applied

2018-11-07 16:08 UTC #13 lore raw

Re: [PATCH 5/7] ext4: bs.bh cleanup before re-using in ext4_xattr_block_find()

In reply to Vasily Averin

Quoted 7 lines · Vasily Averin

bs.bh was taken in previous ext4_xattr_block_find() call, it should be released before re-using
Fixes 7e01c8e5420b ("ext3/4: fix uninitialized bs in ...") # 2.6.26
cc: Tiger Yang <[email protected]>
Signed-off-by Vasily Averin <[email protected]>

Thanks, applied. I used the commit description:

ext4: release bs.bh before re-using in ext4_xattr_block_find()

- Ted

Theodore Y. Ts'o

maintainer Applied

2018-11-07 16:11 UTC #16 lore raw

Re: [PATCH 6/7] ext4: lost brelse in ext4_xattr_move_to_block()

In reply to Jan Kara

Quoted 7 lines · Jan Kara

Quoted 6 lines · Vasily Averin
Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
cc: Jan Kara <[email protected]>
however issue was present in original ext4_expand_extra_isize_ea() Fixes 6dd4ee7cab7e ("ext4: Expand extra_inodes space per ...") # 2.6.23
cc: Kalpak Shah <[email protected]>

Quoted 5 lines

Quoted 1 line
Signed-off-by Vasily Averin <[email protected]>
Good catch. Thanks for the fix. The patch looks good. You can add:
Reviewed-by Jan Kara <[email protected]>

Thanks, applied. I used the commit description:

ext4: fix buffer leak in ext4_xattr_move_to_block() on error path

- Ted

Vasily Averin

author

2018-11-07 17:18 UTC #3 lore raw

In reply to Theodore Y. Ts'o

I was wrong here, but brelse is lost in next error in this function. I'll resend new patch.

Quoted 27 lines · Theodore Y. Ts'o

Quoted 12 lines · Vasily Averin
Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...") # 3.9
Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)
fs/ext4/namei.c +0 -0
index d388cce72db2..1a21e59ede72 100644
fs/ext4/namei.c static struct buffer_head *__ext4_read_dirblock(struct inode *inode, · line 117
Quoted 7 lines
fs/ext4/namei.c +1 -0
@@ -111,6 +111,7 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	if (!bh) {
 		ext4_error_inode(inode, func, line, block,
 				 "Directory hole found");
+		brelse(bh);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	dirent = (struct ext4_dir_entry *) bh->b_data;
Inside the conditional, bh == NULL, so the brelse isn't needed.
Cheers,
- Ted

Vasily Averin

author

2018-11-07 17:30 UTC #4 lore raw

PATCH v2 1/1 ext4: lost brelse in __ext4_read_dirblock()

Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...")
Cc: [email protected] # 3.9

Signed-off-by Vasily Averin <[email protected]>

---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)

fs/ext4/namei.c +1 -0

index d388cce72db2..6a6b90363ef1 100644
@@ -126,6 +126,7 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	if (!is_dx_block && type == INDEX) {
 		ext4_error_inode(inode, func, line, block,
 		       "directory leaf block found instead of index block");
+		brelse(bh);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	if (!ext4_has_metadata_csum(inode->i_sb) ||