[PATCH v2 0/4] ext2: fix race between setxattr and write back

[PATCH v2 0/4] ext2: fix race between setxattr and write back

[Ye Bin]

Diff v2 vs v1:
1. Do not use reservation window when allocate block for xattr.
2. Remove BUG() in ext2_try_to_allocate_with_rsv().

Ye Bin (4):
  ext2: remove ext2_new_block()
  ext2: introduce flag argument for ext2_new_blocks()
  ext2: fix race between setxattr and write back
  ext2: dump current reservation window info

 fs/ext2/balloc.c | 32 +++++++++++++++++---------------
 fs/ext2/ext2.h   |  8 ++++++--
 fs/ext2/inode.c  |  2 +-
 fs/ext2/xattr.c  |  4 +++-
 4 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] ext2: remove ext2_new_block()

[Ye Bin]

Now, only xattr allocate block use ext2_new_block(), so just opencode it in
the xattr code.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext2/balloc.c | 7 -------
 fs/ext2/ext2.h   | 1 -
 fs/ext2/xattr.c  | 4 +++-
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index c8049c90323d..c8515fc501ec 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1429,13 +1429,6 @@ ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
 	return 0;
 }
 
-ext2_fsblk_t ext2_new_block(struct inode *inode, unsigned long goal, int *errp)
-{
-	unsigned long count = 1;
-
-	return ext2_new_blocks(inode, goal, &count, errp);
-}
-
 #ifdef EXT2FS_DEBUG
 
 unsigned long ext2_count_free(struct buffer_head *map, unsigned int numchars)
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 35a041c47c38..954fb82ab22c 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -695,7 +695,6 @@ static inline struct ext2_inode_info *EXT2_I(struct inode *inode)
 /* balloc.c */
 extern int ext2_bg_has_super(struct super_block *sb, int group);
 extern unsigned long ext2_bg_num_gdb(struct super_block *sb, int group);
-extern ext2_fsblk_t ext2_new_block(struct inode *, unsigned long, int *);
 extern ext2_fsblk_t ext2_new_blocks(struct inode *, unsigned long,
 				unsigned long *, int *);
 extern int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk,
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 1c9187188d68..e9546cc65db2 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -742,7 +742,9 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 			/* We need to allocate a new block */
 			ext2_fsblk_t goal = ext2_group_first_block_no(sb,
 						EXT2_I(inode)->i_block_group);
-			int block = ext2_new_block(inode, goal, &error);
+			unsigned long count = 1;
+			int block = ext2_new_blocks(inode, goal, &count,
+						    &error);
 			if (error)
 				goto cleanup;
 			ea_idebug(inode, "creating block %d", block);
-- 
2.31.1

[PATCH v2 2/4] ext2: introduce flag argument for ext2_new_blocks()

[Ye Bin]

This patch introduce introduce flag argument for ext2_new_blocks(), and also
introduce EXT2_ALLOC_NORESERVE flags.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext2/balloc.c | 3 ++-
 fs/ext2/ext2.h   | 7 ++++++-
 fs/ext2/inode.c  | 2 +-
 fs/ext2/xattr.c  | 2 +-
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index c8515fc501ec..99a19f2ecd6f 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1193,6 +1193,7 @@ int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk,
  * @goal:		given target block(filesystem wide)
  * @count:		target number of blocks to allocate
  * @errp:		error code
+ * @flag                allocate flags
  *
  * ext2_new_blocks uses a goal block to assist allocation.  If the goal is
  * free, or there is a free block within 32 blocks of the goal, that block
@@ -1202,7 +1203,7 @@ int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk,
  * This function also updates quota and i_blocks field.
  */
 ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
-		    unsigned long *count, int *errp)
+		    unsigned long *count, int *errp, unsigned int flag)
 {
 	struct buffer_head *bitmap_bh = NULL;
 	struct buffer_head *gdp_bh;
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 954fb82ab22c..36c8ed5dd0a0 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -398,6 +398,11 @@ struct ext2_inode {
 #define EXT2_ERRORS_PANIC		3	/* Panic */
 #define EXT2_ERRORS_DEFAULT		EXT2_ERRORS_CONTINUE
 
+/*
+ * Behaviour if use reservation window in ext2_new_blocks()
+ */
+#define EXT2_ALLOC_NORESERVE            0x1
+
 /*
  * Structure of the super block
  */
@@ -696,7 +701,7 @@ static inline struct ext2_inode_info *EXT2_I(struct inode *inode)
 extern int ext2_bg_has_super(struct super_block *sb, int group);
 extern unsigned long ext2_bg_num_gdb(struct super_block *sb, int group);
 extern ext2_fsblk_t ext2_new_blocks(struct inode *, unsigned long,
-				unsigned long *, int *);
+				unsigned long *, int *, unsigned int);
 extern int ext2_data_block_valid(struct ext2_sb_info *sbi, ext2_fsblk_t start_blk,
 				 unsigned int count);
 extern void ext2_free_blocks (struct inode *, unsigned long,
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index acbab27fe957..462b4e873dc7 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -415,7 +415,7 @@ static int ext2_alloc_blocks(struct inode *inode,
 	while (1) {
 		count = target;
 		/* allocating blocks for indirect blocks and direct blocks */
-		current_block = ext2_new_blocks(inode,goal,&count,err);
+		current_block = ext2_new_blocks(inode, goal, &count, err, 0);
 		if (*err)
 			goto failed_out;
 
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index e9546cc65db2..e076386a26f4 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -744,7 +744,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 						EXT2_I(inode)->i_block_group);
 			unsigned long count = 1;
 			int block = ext2_new_blocks(inode, goal, &count,
-						    &error);
+						    &error, 0);
 			if (error)
 				goto cleanup;
 			ea_idebug(inode, "creating block %d", block);
-- 
2.31.1

[PATCH v2 3/4] ext2: fix race between setxattr and write back

[Ye Bin]

There's a issue as follows:
Block Allocation Reservation Windows Map (ext2_try_to_allocate_with_rsv):
reservation window 0x000000006f105382 start: 0, end: 0
reservation window 0x000000008fd1a555 start: 1044, end: 1059
Window map complete.
kernel BUG at fs/ext2/balloc.c:1158!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
RIP: 0010:ext2_try_to_allocate_with_rsv.isra.0+0x15c4/0x1800
Call Trace:
 <TASK>
 ext2_new_blocks+0x935/0x1690
 ext2_new_block+0x73/0xa0
 ext2_xattr_set2+0x74f/0x1730
 ext2_xattr_set+0x12b6/0x2260
 ext2_xattr_user_set+0x9c/0x110
 __vfs_setxattr+0x139/0x1d0
 __vfs_setxattr_noperm+0xfc/0x370
 __vfs_setxattr_locked+0x205/0x2c0
 vfs_setxattr+0x19d/0x3b0
 do_setxattr+0xff/0x220
 setxattr+0x123/0x150
 path_setxattr+0x193/0x1e0
 __x64_sys_setxattr+0xc8/0x170
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Above issue may happens as follows:
        setxattr                             write back
ext2_xattr_set
  ext2_xattr_set2
    ext2_new_block
      ext2_new_blocks
        ext2_try_to_allocate_with_rsv
          alloc_new_reservation
          --> group=0 [0, 1023] rsv [1016, 1023]
                                        do_writepages
                                          mpage_writepages
                                            write_cache_pages
                                              __mpage_writepage
                                                ext2_get_block
                                                  ext2_get_blocks
                                                   ext2_alloc_branch
                                                    ext2_new_blocks
                                                     ext2_try_to_allocate_with_rsv
                                                       alloc_new_reservation
                                     -->group=1 [1024, 2047] rsv [1044, 1059]
          if ((my_rsv->rsv_start > group_last_block) ||
              (my_rsv->rsv_end < group_first_block)
              rsv_window_dump
              BUG();
Now ext2 mkwrite delay allocate new blocks. So there maybe allocate blocks when
do write back. However, there is no concurrent protection between
ext2_xattr_set() and do_writepages().
To solve about issue don't use reservation window when allocate block for xattr.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext2/balloc.c | 15 +++++++++------
 fs/ext2/xattr.c  |  4 ++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 99a19f2ecd6f..4ff1d831bb80 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1247,13 +1247,16 @@ ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
 	 * it's a regular file, and
 	 * the desired window size is greater than 0 (One could use ioctl
 	 * command EXT2_IOC_SETRSVSZ to set the window size to 0 to turn off
-	 * reservation on that particular file)
+	 * reservation on that particular file), and unspecified Do not use
+	 * reservation window.
 	 */
-	block_i = EXT2_I(inode)->i_block_alloc_info;
-	if (block_i) {
-		windowsz = block_i->rsv_window_node.rsv_goal_size;
-		if (windowsz > 0)
-			my_rsv = &block_i->rsv_window_node;
+	if (!(flag & EXT2_ALLOC_NORESERVE)) {
+		block_i = EXT2_I(inode)->i_block_alloc_info;
+		if (block_i) {
+			windowsz = block_i->rsv_window_node.rsv_goal_size;
+			if (windowsz > 0)
+				my_rsv = &block_i->rsv_window_node;
+		}
 	}
 
 	if (!ext2_has_free_blocks(sbi)) {
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index e076386a26f4..21725b8d04c8 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -743,8 +743,8 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 			ext2_fsblk_t goal = ext2_group_first_block_no(sb,
 						EXT2_I(inode)->i_block_group);
 			unsigned long count = 1;
-			int block = ext2_new_blocks(inode, goal, &count,
-						    &error, 0);
+			int block = ext2_new_blocks(inode, goal, &count, &error,
+						    EXT2_ALLOC_NORESERVE);
 			if (error)
 				goto cleanup;
 			ea_idebug(inode, "creating block %d", block);
-- 
2.31.1

[PATCH v2 4/4] ext2: dump current reservation window info

[Ye Bin]

There's report BUG in 'ext2_try_to_allocate_with_rsv()', although there's
now dump of all reservation windows information. But there's unknown which
window is being processed.So this is not helpful for locating the issue.
To better analyze the problem, dump the information about reservation window
that is being processed. And just bail with error instead of BUG here.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext2/balloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 4ff1d831bb80..f09f413030a5 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1131,8 +1131,13 @@ ext2_try_to_allocate_with_rsv(struct super_block *sb, unsigned int group,
 
 		if ((my_rsv->rsv_start > group_last_block) ||
 				(my_rsv->rsv_end < group_first_block)) {
+			ext2_error(sb, __func__,
+				   "Reservation out of group %u range goal %d fsb[%lu,%lu] rsv[%lu, %lu]",
+				   group, grp_goal, group_first_block,
+				   group_last_block, my_rsv->rsv_start,
+				   my_rsv->rsv_end);
 			rsv_window_dump(&EXT2_SB(sb)->s_rsv_window_root, 1);
-			BUG();
+			return -1;
 		}
 		ret = ext2_try_to_allocate(sb, group, bitmap_bh, grp_goal,
 					   &num, &my_rsv->rsv_window);
-- 
2.31.1

Re: [PATCH v2 0/4] ext2: fix race between setxattr and write back

[Jan Kara]

On Tue 15-08-23 19:26:08, Ye Bin wrote:
> Diff v2 vs v1:
> 1. Do not use reservation window when allocate block for xattr.
> 2. Remove BUG() in ext2_try_to_allocate_with_rsv().

Thanks! I did't a few smaller spelling fixups on commit but otherwise the
patches look good so I've added them to my tree.

								Honza

> 
> Ye Bin (4):
>   ext2: remove ext2_new_block()
>   ext2: introduce flag argument for ext2_new_blocks()
>   ext2: fix race between setxattr and write back
>   ext2: dump current reservation window info
> 
>  fs/ext2/balloc.c | 32 +++++++++++++++++---------------
>  fs/ext2/ext2.h   |  8 ++++++--
>  fs/ext2/inode.c  |  2 +-
>  fs/ext2/xattr.c  |  4 +++-
>  4 files changed, 27 insertions(+), 19 deletions(-)
> 
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR