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

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

[Vasily Averin]

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

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d388cce72db2..1a21e59ede72 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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;
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:12:18PM +0300, Vasily Averin wrote:
> Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...") # 3.9
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/ext4/namei.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index d388cce72db2..1a21e59ede72 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -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

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

[Vasily Averin]

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

On 11/7/18 6:25 PM, Theodore Y. Ts'o wrote:
> On Wed, Oct 31, 2018 at 10:12:18PM +0300, Vasily Averin wrote:
>> Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...") # 3.9
>>
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>>  fs/ext4/namei.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index d388cce72db2..1a21e59ede72 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -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
> 

[PATCH v2] ext4: lost brelse in __ext4_read_dirblock()

[Vasily Averin]

Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...")
Cc: stable@kernel.org # 3.9

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/namei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d388cce72db2..6a6b90363ef1 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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) ||
-- 
2.17.1

Re: [PATCH v2] ext4: lost brelse in __ext4_read_dirblock()

[Theodore Y. Ts'o]

On Wed, Nov 07, 2018 at 08:30:17PM +0300, Vasily Averin wrote:
> Fixes dc6982ff4db1 ("ext4: refactor code to read directory blocks ...")
> Cc: stable@kernel.org # 3.9
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.  I used the commit description:

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

	      	     			    - Ted

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

[Vasily Averin]

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

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a221f1cdf704..ed4d36506ec2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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);
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:12:27PM +0300, Vasily Averin wrote:
> Fixes bfe0a5f47ada ("ext4: add more mount time checks of the superblock") # 4.18
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/ext4/super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a221f1cdf704..ed4d36506ec2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -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 <tytso@mit.edu>
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 <vvs@virtuozzo.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>
    Cc: stable@kernel.org # 4.18

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a221f1cdf704..92092b55db1e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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);
 

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

[Vasily Averin]

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

cc: Daeho Jeong <daeho.jeong@samsung.com>
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/super.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ed4d36506ec2..0c9e165b4e3f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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);
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:12:35PM +0300, Vasily Averin wrote:
> Fixes c8585c6fcaf2 ("ext4: fix races between changing inode journal ...") # 4.7
> 
> cc: Daeho Jeong <daeho.jeong@samsung.com>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.

					- Ted

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

[Vasily Averin]

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

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index dc1aeab06dba..07c3a115f7ae 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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;
 }
 
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:12:43PM +0300, Vasily Averin wrote:
> Fixes dec214d00e0d ("ext4: xattr inode deduplication") # 4.13
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.  I used the description line:

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

    	      	     	     			    - Ted
						    

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

[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 <tiger.yang@oracle.com>

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 07c3a115f7ae..07b9a335c8eb 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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;
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:12:52PM +0300, Vasily Averin wrote:
> 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 <tiger.yang@oracle.com>
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.  I used the commit description:

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

    	  	  	       		   - Ted

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

[Vasily Averin]

Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
cc: Jan Kara <jack@suse.cz>
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 <kalpak@clusterfs.com>

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 07b9a335c8eb..5c9bc0d85cc0 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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);
 
-- 
2.17.1

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

[Jan Kara]

On Wed 31-10-18 22:13:00, Vasily Averin wrote:
> Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
> cc: Jan Kara <jack@suse.cz>
> 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 <kalpak@clusterfs.com>
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

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

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/xattr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 07b9a335c8eb..5c9bc0d85cc0 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -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);
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Theodore Y. Ts'o]

On Wed, Nov 07, 2018 at 02:50:16PM +0100, Jan Kara wrote:
> On Wed 31-10-18 22:13:00, Vasily Averin wrote:
> > Fixes 3f2571c1f91f ("ext4: factor out xattr moving")
> > cc: Jan Kara <jack@suse.cz>
> > 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 <kalpak@clusterfs.com>
> > 
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Good catch. Thanks for the fix. The patch looks good. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, applied.  I used the commit description:

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

    	      	     	     - Ted

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

[Vasily Averin]

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

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ext4/xattr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 5c9bc0d85cc0..0b9688683526 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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;
-- 
2.17.1

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

[Theodore Y. Ts'o]

On Wed, Oct 31, 2018 at 10:13:09PM +0300, Vasily Averin wrote:
> Fixes de05ca852679 ("ext4: move call to ext4_error() into ...") # 4.17
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thanks, applied.  I used the commit description:

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

    	      	     	     			- Ted