[PATCH 0/2] fix race between setxattr and write back

[PATCH 0/2] fix race between setxattr and write back

[Ye Bin]

Ye Bin (2):
  ext2: fix race between setxattr and write back
  ext2: dump current reservation window info before BUG

 fs/ext2/balloc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/2] 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 hold '&ei->truncate_mutex' lock when new block for xattr.

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

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index c8049c90323d..039f655febfd 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1432,8 +1432,14 @@ ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
 ext2_fsblk_t ext2_new_block(struct inode *inode, unsigned long goal, int *errp)
 {
 	unsigned long count = 1;
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	ext2_fsblk_t ret;
+
+	mutex_lock(&ei->truncate_mutex);
+	ret = ext2_new_blocks(inode, goal, &count, errp);
+	mutex_unlock(&ei->truncate_mutex);
 
-	return ext2_new_blocks(inode, goal, &count, errp);
+	return ret;
 }
 
 #ifdef EXT2FS_DEBUG
-- 
2.31.1

[PATCH 2/2] ext2: dump current reservation window info before BUG

[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.

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

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 039f655febfd..0132b8af546e 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1131,6 +1131,11 @@ 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__,
+				   "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();
 		}
-- 
2.31.1

Re: [PATCH 1/2] ext2: fix race between setxattr and write back

[Jan Kara]

On Fri 11-08-23 11:38:56, Ye Bin wrote:
> 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 hold '&ei->truncate_mutex' lock when new block for xattr.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Thanks for the patch! I agree the handling of reservation window and its
use for block allocation needs to be protected by ei->i_truncate_mutex.
However in this particular case of xattr allocation where we want to
allocate just one block which is completely independent of file data, I'd
rather choose to make ext2_new_blocks() ignore the reservation window (set
my_rsv to NULL). There's already a logic at the beginning of
ext2_new_blocks() deciding whether to use the reservation window or not and
we could extend it - probably by adding flags argument to it a introducing
a NORESERVE flag.

Also as a preparatory patch, I'd just remove ext2_new_block() and opencode
it in the xattr code since it has only that one user anyway.

								Honza



> ---
>  fs/ext2/balloc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index c8049c90323d..039f655febfd 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -1432,8 +1432,14 @@ ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
>  ext2_fsblk_t ext2_new_block(struct inode *inode, unsigned long goal, int *errp)
>  {
>  	unsigned long count = 1;
> +	struct ext2_inode_info *ei = EXT2_I(inode);
> +	ext2_fsblk_t ret;
> +
> +	mutex_lock(&ei->truncate_mutex);
> +	ret = ext2_new_blocks(inode, goal, &count, errp);
> +	mutex_unlock(&ei->truncate_mutex);
>  
> -	return ext2_new_blocks(inode, goal, &count, errp);
> +	return ret;
>  }
>  
>  #ifdef EXT2FS_DEBUG
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] ext2: dump current reservation window info before BUG

[Jan Kara]

On Fri 11-08-23 11:38:57, Ye Bin wrote:
> 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.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  fs/ext2/balloc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index 039f655febfd..0132b8af546e 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -1131,6 +1131,11 @@ 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__,
> +				   "Out of group %u range goal %d fsb[%lu,%lu] rsv[%lu, %lu]",
                                   ^^^ "Reservation out of group ...."

> +				   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();
>  		}

And we could just bail with error instead of BUG here when we are already
changing the code...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] ext2: fix race between setxattr and write back

[yebin (H)]

On 2023/8/14 20:46, Jan Kara wrote:
> On Fri 11-08-23 11:38:56, Ye Bin wrote:
>> 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 hold '&ei->truncate_mutex' lock when new block for xattr.
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Thanks for the patch! I agree the handling of reservation window and its
> use for block allocation needs to be protected by ei->i_truncate_mutex.
> However in this particular case of xattr allocation where we want to
> allocate just one block which is completely independent of file data, I'd
> rather choose to make ext2_new_blocks() ignore the reservation window (set
> my_rsv to NULL). There's already a logic at the beginning of
> ext2_new_blocks() deciding whether to use the reservation window or not and
> we could extend it - probably by adding flags argument to it a introducing
> a NORESERVE flag.
>
> Also as a preparatory patch, I'd just remove ext2_new_block() and opencode
> it in the xattr code since it has only that one user anyway.
>
> 								Honza
>
Thanks for your suggestion.  I will send new version according to your 
suggestion.
>
>> ---
>>   fs/ext2/balloc.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
>> index c8049c90323d..039f655febfd 100644
>> --- a/fs/ext2/balloc.c
>> +++ b/fs/ext2/balloc.c
>> @@ -1432,8 +1432,14 @@ ext2_fsblk_t ext2_new_blocks(struct inode *inode, ext2_fsblk_t goal,
>>   ext2_fsblk_t ext2_new_block(struct inode *inode, unsigned long goal, int *errp)
>>   {
>>   	unsigned long count = 1;
>> +	struct ext2_inode_info *ei = EXT2_I(inode);
>> +	ext2_fsblk_t ret;
>> +
>> +	mutex_lock(&ei->truncate_mutex);
>> +	ret = ext2_new_blocks(inode, goal, &count, errp);
>> +	mutex_unlock(&ei->truncate_mutex);
>>   
>> -	return ext2_new_blocks(inode, goal, &count, errp);
>> +	return ret;
>>   }
>>   
>>   #ifdef EXT2FS_DEBUG
>> -- 
>> 2.31.1
>>