Skip to contentSkip to conversation

ext4 resise: extra brelse in setup_new_flex_group_blocks()

PATCH Applied Vasily Averin 3 participants 23 messages Oct 30, 2018 – Nov 6, 2018 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]

Vasily Averin

author
#1 lore raw

PATCH v2 1/11 ext4 resise: extra brelse in setup_new_flex_group_blocks()

currently bh is set to NULL only during first iteration of for cycle, then this pointer is not cleared after end of using.
Therefore rollback after errors can lead to extra brelse(bh) call, decrements bh counter and later trigger an unexpected warning in __brelse()

Patch moves brelse() calls in body of cycle to exclude requirement of brelse() call in rollback.

Fixes 33afdcc5402d ("ext4: add a function which sets up group blocks ...") # 3.3+

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
fs/ext4/resize.c +2 -6
index ebbc663d0798..c3fa30878ca8 100644
@@ -605,7 +605,6 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 		bh = bclean(handle, sb, block);
 		if (IS_ERR(bh)) {
 			err = PTR_ERR(bh);
-			bh = NULL;
 			goto out;
 		}
 		overhead = ext4_group_overhead_blocks(sb, group);

@@ -618,9 +617,9 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 		ext4_mark_bitmap_end(EXT4_B2C(sbi, group_data[i].blocks_count),
 				     sb->s_blocksize * 8, bh->b_data);
 		err = ext4_handle_dirty_metadata(handle, NULL, bh);
+		brelse(bh);
 		if (err)
 			goto out;
-		brelse(bh);
 
 handle_ib:
 		if (bg_flags[i] & EXT4_BG_INODE_UNINIT)

@@ -635,18 +634,16 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 		bh = bclean(handle, sb, block);
 		if (IS_ERR(bh)) {
 			err = PTR_ERR(bh);
-			bh = NULL;
 			goto out;
 		}
 
 		ext4_mark_bitmap_end(EXT4_INODES_PER_GROUP(sb),
 				     sb->s_blocksize * 8, bh->b_data);
 		err = ext4_handle_dirty_metadata(handle, NULL, bh);
+		brelse(bh);
 		if (err)
 			goto out;
-		brelse(bh);
 	}
-	bh = NULL;
 
 	/* Mark group tables in block bitmap */
 	for (j = 0; j < GROUP_TABLE_COUNT; j++) {

@@ -685,7 +682,6 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 	}
 
 out:
-	brelse(bh);
 	err2 = ext4_journal_stop(handle);
 	if (err2 && !err)
 		err = err2;
Signature · 1 line
2.17.1
maintainer Applied
#2 lore raw
Quoted 11 lines · Vasily Averin

currently bh is set to NULL only during first iteration of for cycle, then this pointer is not cleared after end of using.
Therefore rollback after errors can lead to extra brelse(bh) call, decrements bh counter and later trigger an unexpected warning in __brelse()

Patch moves brelse() calls in body of cycle to exclude requirement of brelse() call in rollback.

Fixes 33afdcc5402d ("ext4: add a function which sets up group blocks ...") # 3.3+

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

Thanks, applied. I addjusted the patch summary to read:

ext4: avoid potential extra brelse in setup_new_flex_group_blocks()

(Note that resise should have been spelled as "resize", by the way.)

- Ted

author
#3 lore raw

PATCH v2 2/11 ext4 resize: missing brelse() after errors in set_flexbg_block_bitmap()

In reply to a message not in the archive

Fixes 33afdcc5402d ("ext4: add a function which sets up group blocks ...") # 3.3

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
fs/ext4/resize.c +4 -2
index c3fa30878ca8..0a4dc6217e78 100644
@@ -459,16 +459,18 @@ static int set_flexbg_block_bitmap(struct super_block *sb, handle_t *handle,
 
 		BUFFER_TRACE(bh, "get_write_access");
 		err = ext4_journal_get_write_access(handle, bh);
-		if (err)
+		if (err) {
+			brelse(bh);
 			return err;
+		}
 		ext4_debug("mark block bitmap %#04llx (+%llu/%u)\n",
 			   first_cluster, first_cluster - start, count2);
 		ext4_set_bits(bh->b_data, first_cluster - start, count2);
 
 		err = ext4_handle_dirty_metadata(handle, NULL, bh);
+		brelse(bh);
 		if (unlikely(err))
 			return err;
-		brelse(bh);
 	}
 
 	return 0;
Signature · 1 line
2.17.1
maintainer Applied
#4 lore raw

Re: [PATCH v2 02/11] ext4 resize: missing brelse() after errors in set_flexbg_block_bitmap()

Quoted 3 lines · Vasily Averin

Fixes 33afdcc5402d ("ext4: add a function which sets up group blocks ...") # 3.3

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

Thanks, applied. I adjusted the patch summary:

ext4: add missing brelse() in set_flexbg_block_bitmap()'s error path

Also please note that properly Fixes should have a colon, and to add
Cc: [email protected] to trailer to make sure they get backported:

Fixes 33afdcc5402d ("ext4: add a function which sets up group blocks ...")
Cc [email protected] # 3.3

- Ted

author
#5 lore raw

PATCH v2 3/11 ext4 resize: brelse() cleanup in add_new_gdb_meta_bg()

In reply to a message not in the archive

gdb_bh must be released in case of errors before update of s_group_desc but it must not be released after update of group descriptors because in this case bh can be used later.

Fixes 01f795f9e0d6 ("ext4: add online resizing support for meta_bg ...") # 3.7

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/ext4/resize.c +1 -2
index 0a4dc6217e78..7131f35b62d9 100644
@@ -922,6 +922,7 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
 				     sizeof(struct buffer_head *),
 				     GFP_NOFS);
 	if (!n_group_desc) {
+		brelse(gdb_bh);
 		err = -ENOMEM;
 		ext4_warning(sb, "not enough memory for %lu groups",
 			     gdb_num + 1);

@@ -937,8 +938,6 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
 	kvfree(o_group_desc);
 	BUFFER_TRACE(gdb_bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, gdb_bh);
-	if (unlikely(err))
-		brelse(gdb_bh);
 	return err;
 }
Signature · 1 line
2.17.1
maintainer Applied
#6 lore raw

Re: [PATCH v2 03/11] ext4 resize: brelse() cleanup in add_new_gdb_meta_bg()

Quoted 7 lines · Vasily Averin

gdb_bh must be released in case of errors before update of s_group_desc but it must not be released after update of group descriptors because in this case bh can be used later.

Fixes 01f795f9e0d6 ("ext4: add online resizing support for meta_bg ...") # 3.7

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

Thanks, applied. The commit description is not accurate (I suspect it was obsoleted when your patch for RHEL was forward ported) do I dropped it.

I adjusted the patch summary to be:

ext4: add missing brelse() add_new_gdb_meta_bg()'s error path

The commments for the Fixes and Cc: [email protected] also apply.

- Ted

author
#7 lore raw

PATCH v2 4/11 ext4 resize: lost brelse() in update_backups()

In reply to a message not in the archive

bh was not released after error in ext4_journal_get_write_access()

Fixes ac27a0ec112a ("ext4: initial copy of files from ext3") # 2.6.19

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ext4/resize.c +3 -1
index 7131f35b62d9..3df326ee6d50 100644
@@ -1121,8 +1121,10 @@ static void update_backups(struct super_block *sb, sector_t blk_off, char *data,
 			   backup_block, backup_block -
 			   ext4_group_first_block_no(sb, group));
 		BUFFER_TRACE(bh, "get_write_access");
-		if ((err = ext4_journal_get_write_access(handle, bh)))
+		if ((err = ext4_journal_get_write_access(handle, bh))) {
+			brelse(bh);
 			break;
+		}
 		lock_buffer(bh);
 		memcpy(bh->b_data, data, size);
 		if (rest)
Signature · 1 line
2.17.1
maintainer Applied
#8 lore raw

Re: [PATCH v2 04/11] ext4 resize: lost brelse() in update_backups()

Quoted 5 lines · Vasily Averin

bh was not released after error in ext4_journal_get_write_access()

Fixes ac27a0ec112a ("ext4: initial copy of files from ext3") # 2.6.19

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

Thanks, applied. I fixed up the commit description and Fixes/Cc trailers.

- Ted

author
#9 lore raw

PATCH v2 5/11 ext4 resize: lost rollback in ext4_resize_fs()

In reply to a message not in the archive

Fixes 117fff10d7f1 ("ext4: grow the s_flex_groups array as needed ...") # 3.7

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/resize.c +1 -1
index 3df326ee6d50..5fee65afd58b 100644
@@ -2022,7 +2022,7 @@ int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count)
 
 	err = ext4_alloc_flex_bg_array(sb, n_group + 1);
 	if (err)
-		return err;
+		goto out;
 
 	err = ext4_mb_alloc_groupinfo(sb, n_group + 1);
 	if (err)
Signature · 1 line
2.17.1
maintainer Applied
#10 lore raw

Re: [PATCH v2 05/11] ext4 resize: lost rollback in ext4_resize_fs()

Quoted 3 lines · Vasily Averin

Fixes 117fff10d7f1 ("ext4: grow the s_flex_groups array as needed ...") # 3.7

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

Thanks, applied. I fixed up the commit description and Fixes/Cc trailers.

- Ted

author
#11 lore raw

PATCH v2 6/11 ext4 resize: lost resize_inode cleanup before retry in ext4_resize_fs()

In reply to a message not in the archive

Fixes 1c6bd7173d66 ("ext4: convert file system to meta_bg if needed ...") # 3.7

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/ext4/resize.c +4 -0
index 5fee65afd58b..85158e9de7c2 100644
@@ -2058,6 +2058,10 @@ int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count)
 		n_blocks_count_retry = 0;
 		free_flex_gd(flex_gd);
 		flex_gd = NULL;
+		if (resize_inode) {
+			iput(resize_inode);
+			resize_inode = NULL;
+		}
 		goto retry;
 	}
Signature · 1 line
2.17.1
maintainer Applied
#12 lore raw

Re: [PATCH v2 06/11] ext4 resize: lost resize_inode cleanup before retry in ext4_resize_fs()

Quoted 3 lines · Vasily Averin

Fixes 1c6bd7173d66 ("ext4: convert file system to meta_bg if needed ...") # 3.7

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

Thanks, applied. I fixed up the commit description and Fixes/Cc trailers.

- Ted

author
#13 lore raw

PATCH v2 7/11 ext4: lost put_bh in ext4_mark_iloc_dirty()

In reply to a message not in the archive

ext4_mark_iloc_dirty() callers expect that it releases iloc->bh even if it returns an error.

Fixes 0db1ff222d40 ("ext4: add shutdown bit and check for it") # 4.11

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/inode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/ext4/inode.c +3 -2
index c3d9a42c561e..55c8fca76daf 100644
@@ -5835,9 +5835,10 @@ int ext4_mark_iloc_dirty(handle_t *handle,
 {
 	int err = 0;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
+		put_bh(iloc->bh);
 		return -EIO;
-
+	}
 	if (IS_I_VERSION(inode))
 		inode_inc_iversion(inode);
Signature · 1 line
2.17.1
maintainer Applied
#14 lore raw

Re: [PATCH v2 07/11] ext4: lost put_bh in ext4_mark_iloc_dirty()

Quoted 6 lines · Vasily Averin

ext4_mark_iloc_dirty() callers expect that it releases iloc->bh even if it returns an error.

Fixes 0db1ff222d40 ("ext4: add shutdown bit and check for it") # 4.11

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

Thanks, applied. I fixed up the commit description and Fixes/Cc trailers. I used the one-line description:

ext4: avoid buffer leak on shutdown in ext4_mark_iloc_dirty()

- Ted

author
#15 lore raw

PATCH v2 8/11 ext4: lost brelse in ext4_orphan_add()

In reply to a message not in the archive

iloc.bh os taken in ext4_reserve_inode_write().
If (dirty == true) it is released in ext4_mark_iloc_dirty(), however it is not released in (dirty == false) case.

Fixes d745a8c20c1f ("ext4: reduce contention on s_orphan_lock") however iloc.bh count balance was broken earlier by
Fixes 6e3617e579e0 ("ext4: Handle non empty on-disk orphan link") #2.6.34

cc Dmitry Monakhov <[email protected]>
Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/namei.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ext4/namei.c +3 -1
index 67a38532032a..d388cce72db2 100644
@@ -2811,7 +2811,9 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode)
 			list_del_init(&EXT4_I(inode)->i_orphan);
 			mutex_unlock(&sbi->s_orphan_lock);
 		}
-	}
+	} else
+		brelse(iloc.bh);
+
 	jbd_debug(4, "superblock will point to %lu\n", inode->i_ino);
 	jbd_debug(4, "orphan inode %lu will point to %d\n",
 			inode->i_ino, NEXT_ORPHAN(inode));
Signature · 1 line
2.17.1
maintainer Applied
#16 lore raw

Re: [PATCH v2 08/11] ext4: lost brelse in ext4_orphan_add()

Quoted 10 lines · Vasily Averin

iloc.bh os taken in ext4_reserve_inode_write().
If (dirty == true) it is released in ext4_mark_iloc_dirty(), however it is not released in (dirty == false) case.

Fixes d745a8c20c1f ("ext4: reduce contention on s_orphan_lock") however iloc.bh count balance was broken earlier by
Fixes 6e3617e579e0 ("ext4: Handle non empty on-disk orphan link") #2.6.34

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

Thanks, applied. I fixed up the commit description and Fixes/Cc trailers. I used the one-line description:

ext4: avoid buffer leak in ext4_orphan_add() after prior errors

- Ted

author
#17 lore raw

PATCH v2 9/11 ext4: iloc.bh cleanup in add_new_gdb()

In reply to a message not in the archive

iloc.bh was taken in ext4_reserve_inode_write() and released in ext4_mark_iloc_dirty(). It should not be released 2nd time in rollback after failed ext4_handle_dirty_metadata(gdb_bh)

Fixes b40971426a83 ("ext4: add error checking to calls to ...") # 2.6.38

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/resize.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/ext4/resize.c +3 -2
index 85158e9de7c2..aedfd6a6fcd1 100644
@@ -871,7 +871,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	err = ext4_handle_dirty_metadata(handle, NULL, gdb_bh);
 	if (unlikely(err)) {
 		ext4_std_error(sb, err);
-		goto exit_inode;
+		goto exit_kfree;
 	}
 	brelse(dind);
 

@@ -891,8 +891,9 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	return err;
 
 exit_inode:
-	kvfree(n_group_desc);
 	brelse(iloc.bh);
+exit_kfree:
+	kvfree(n_group_desc);
 exit_dind:
 	brelse(dind);
 exit_bh:
Signature · 1 line
2.17.1
maintainer
#18 lore raw

Re: [PATCH v2 09/11] ext4: iloc.bh cleanup in add_new_gdb()

Quoted 7 lines · Vasily Averin

iloc.bh was taken in ext4_reserve_inode_write() and released in ext4_mark_iloc_dirty(). It should not be released 2nd time in rollback after failed ext4_handle_dirty_metadata(gdb_bh)

Fixes b40971426a83 ("ext4: add error checking to calls to ...") # 2.6.38

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

I fixed this in a simpler way:

From 6a91a2eb1c5af1381caebfb4c1c91dc175351f6f Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Tue, 6 Nov 2018 17:18:17 -0500
Subject: [PATCH] ext4: avoid possible double brelse() in add_new_gdb() on
error path

Fixes b40971426a83 ("ext4: add error checking to calls to ...")
Reported-by Vasily Averin <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
Cc [email protected] # 2.6.38
---
 fs/ext4/resize.c | 1 +
 1 file changed, 1 insertion(+)
fs/ext4/resize.c +1 -0
index 85158e9de7c2..a5efee34415f 100644
@@ -871,6 +871,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	err = ext4_handle_dirty_metadata(handle, NULL, gdb_bh);
 	if (unlikely(err)) {
 		ext4_std_error(sb, err);
+		iloc.bh = NULL;
 		goto exit_inode;
 	}
 	brelse(dind);
Signature · 1 line
2.18.0.rc0
author
#19 lore raw

PATCH v2 10/11 ext4: remove useless brelse call in ext4_xattr_inode_update_ref()

In reply to a message not in the archive

brelse(iloc.bh) is useless here, it is always called with iloc.bh = NULL

Fixes dec214d00e0d ("ext4: xattr inode deduplication") # 4.13
cc: Tahsin Erdogan <[email protected]>

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/xattr.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
fs/ext4/xattr.c +1 -5
index f36fc5d5b257..dc1aeab06dba 100644
@@ -1031,10 +1031,8 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
 	inode_lock(ea_inode);
 
 	ret = ext4_reserve_inode_write(handle, ea_inode, &iloc);
-	if (ret) {
-		iloc.bh = NULL;
+	if (ret)
 		goto out;
-	}
 
 	ref_count = ext4_xattr_inode_get_ref(ea_inode);
 	ref_count += ref_change;

@@ -1080,12 +1078,10 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
 	}
 
 	ret = ext4_mark_iloc_dirty(handle, ea_inode, &iloc);
-	iloc.bh = NULL;
 	if (ret)
 		ext4_warning_inode(ea_inode,
 				   "ext4_mark_iloc_dirty() failed ret=%d", ret);
 out:
-	brelse(iloc.bh);
 	inode_unlock(ea_inode);
 	return ret;
 }
Signature · 1 line
2.17.1
maintainer Applied
#20 lore raw

Re: [PATCH v2 10/11] ext4: remove useless brelse call in ext4_xattr_inode_update_ref()

Quoted 6 lines · Vasily Averin

brelse(iloc.bh) is useless here, it is always called with iloc.bh = NULL

Fixes dec214d00e0d ("ext4: xattr inode deduplication") # 4.13
cc: Tahsin Erdogan <[email protected]>

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

Applied. I droppped the "Fixes" since it's really not needed for stable kernel; this is really more of a cleanup. I used the commit description:

ext4: remove unneeded brelse call in ext4_xattr_inode_update_ref()

- Ted

author
#21 lore raw

PATCH v2 11/11 ext4: access to uninitialized bh fields in ext4_xattr_set_handle()

In reply to a message not in the archive

On-stack initialization does not guarantee zeroying of unintialized fields. So is.iloc.bh and bs.bh can be contain garbage of old stack conent.

Errors in the beginning of ext4_xattr_set_handle() function lead to jump to "cleanup:" label where brelse(is.iloc.bh) and brelse(bs.bh) can access uninitialized bh fields of on-stack located "is" and "bs" structures.

Issue was inherited from ext3 and was present in first ext4 commit.

Fixes ac27a0ec112a ("ext4: initial copy of files from ext3") # 2.6.19

Signed-off-by Vasily Averin <[email protected]>
---
 fs/ext4/xattr.c | 2 ++
 1 file changed, 2 insertions(+)
fs/ext4/xattr.c +2 -0
index dc1aeab06dba..aae12425597e 100644
@@ -2303,9 +2303,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
 	};
 	struct ext4_xattr_ibody_find is = {
 		.s = { .not_found = -ENODATA, },
+		.iloc = { .bh = NULL, },
 	};
 	struct ext4_xattr_block_find bs = {
 		.s = { .not_found = -ENODATA, },
+		.bh = NULL,
 	};
 	int no_expand;
 	int error;
Signature · 1 line
2.17.1

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

author
#23 lore raw

Re: [PATCH v2 11/11] ext4: access to uninitialized bh fields in ext4_xattr_set_handle()

Quoted 7 lines · Andreas Dilger

Could you please explain your statement below that on-stack initialization does not zero unspecified fields?  According to documents I found, for example:

https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

they *are* initialized to zero.

I did not know it,
I re-checked it in generated assembler code and found that you are right and I was wrong.

Please drop this patch,
should I resend of rest of this patch set once again?

Thank you,
Vasily Averin

#24 lore raw

Re: [PATCH v2 11/11] ext4: access to uninitialized bh fields in ext4_xattr_set_handle()

Quoted 8 lines · Vasily Averin
Quoted 6 lines · Andreas Dilger

Could you please explain your statement below that on-stack initialization does not zero unspecified fields? According to documents I found, for example:

https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Quoted 8 lines
Quoted 1 line

they *are* initialized to zero.

I did not know it,
I re-checked it in generated assembler code and found that you are right and I was wrong.

Please drop this patch,
should I resend of rest of this patch set once again?

I don't think it is necessary to resend the whole patch series. Ted should notice these emails on this patch and not apply it.

Cheers, Andreas