[PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Baokun Li]

This patch and problem analysis is based on v4.19 LTS.
The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
is incorporated in v5.7-rc1. This patch avoids this problem by switching
to iomap in ext4_fiemap.

Hulk Robot reported a BUG on stable 4.19.252:
==================================================================
kernel BUG at fs/ext4/extents_status.c:762!
invalid opcode: 0000 [#1] SMP KASAN PTI
CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
RIP: 0010:ext4_es_cache_extent+0x30e/0x370
[...]
Call Trace:
 ext4_cache_extents+0x238/0x2f0
 ext4_find_extent+0x785/0xa40
 ext4_fiemap+0x36d/0xe90
 do_vfs_ioctl+0x6af/0x1200
[...]
==================================================================

Above issue may happen as follows:
-------------------------------------
           cpu1		    cpu2
_____________________|_____________________
do_vfs_ioctl
 ext4_ioctl
  ext4_ioctl_setflags
   ext4_ind_migrate
                        do_vfs_ioctl
                         ioctl_fiemap
                          ext4_fiemap
                           ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
                           ext4_fill_fiemap_extents
    down_write(&EXT4_I(inode)->i_data_sem);
    ext4_ext_check_inode
    ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
    memset(ei->i_data, 0, sizeof(ei->i_data))
    up_write(&EXT4_I(inode)->i_data_sem);
                            down_read(&EXT4_I(inode)->i_data_sem);
                            ext4_find_extent
                             ext4_cache_extents
                              ext4_es_cache_extent
                               BUG_ON(end < lblk)

We can easily reproduce this problem with the syzkaller testcase:
```
02:37:07 executing program 3:
r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
```

To solve this issue, we use __generic_block_fiemap() instead of
generic_block_fiemap() and add inode_lock_shared to avoid race condition.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 6c492fca60c4..38aaf48e94cb 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5198,13 +5198,18 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			return error;
 	}
 
+	inode_lock_shared(inode);
 	/* fallback to generic here if not in extents fmt */
-	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
-		return generic_block_fiemap(inode, fieinfo, start, len,
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
+		error = __generic_block_fiemap(inode, fieinfo, start, len,
 			ext4_get_block);
+		goto out_unlock;
+	}
 
-	if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS))
-		return -EBADR;
+	if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS)) {
+		error = -EBADR;
+		goto out_unlock;
+	}
 
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
 		error = ext4_xattr_fiemap(inode, fieinfo);
@@ -5225,6 +5230,8 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		error = ext4_fill_fiemap_extents(inode, start_blk,
 						 len_blks, fieinfo);
 	}
+out_unlock:
+	inode_unlock_shared(inode);
 	return error;
 }
 
-- 
2.31.1

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Greg KH]

On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
> This patch and problem analysis is based on v4.19 LTS.
> The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
> is incorporated in v5.7-rc1. This patch avoids this problem by switching
> to iomap in ext4_fiemap.
> 
> Hulk Robot reported a BUG on stable 4.19.252:
> ==================================================================
> kernel BUG at fs/ext4/extents_status.c:762!
> invalid opcode: 0000 [#1] SMP KASAN PTI
> CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
> RIP: 0010:ext4_es_cache_extent+0x30e/0x370
> [...]
> Call Trace:
>  ext4_cache_extents+0x238/0x2f0
>  ext4_find_extent+0x785/0xa40
>  ext4_fiemap+0x36d/0xe90
>  do_vfs_ioctl+0x6af/0x1200
> [...]
> ==================================================================
> 
> Above issue may happen as follows:
> -------------------------------------
>            cpu1		    cpu2
> _____________________|_____________________
> do_vfs_ioctl
>  ext4_ioctl
>   ext4_ioctl_setflags
>    ext4_ind_migrate
>                         do_vfs_ioctl
>                          ioctl_fiemap
>                           ext4_fiemap
>                            ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
>                            ext4_fill_fiemap_extents
>     down_write(&EXT4_I(inode)->i_data_sem);
>     ext4_ext_check_inode
>     ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
>     memset(ei->i_data, 0, sizeof(ei->i_data))
>     up_write(&EXT4_I(inode)->i_data_sem);
>                             down_read(&EXT4_I(inode)->i_data_sem);
>                             ext4_find_extent
>                              ext4_cache_extents
>                               ext4_es_cache_extent
>                                BUG_ON(end < lblk)
> 
> We can easily reproduce this problem with the syzkaller testcase:
> ```
> 02:37:07 executing program 3:
> r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
> ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
> mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
> r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
> ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
> ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
> ```
> 
> To solve this issue, we use __generic_block_fiemap() instead of
> generic_block_fiemap() and add inode_lock_shared to avoid race condition.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/ext4/extents.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

What is the git commit id of this change in Linus's tree?

If it is not in Linus's tree, why not?

confused,

greg k-h

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Baokun Li]

在 2022/7/15 22:10, Greg KH 写道:
> On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
>> This patch and problem analysis is based on v4.19 LTS.
>> The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
>> is incorporated in v5.7-rc1. This patch avoids this problem by switching
>> to iomap in ext4_fiemap.
>>
>> Hulk Robot reported a BUG on stable 4.19.252:
>> ==================================================================
>> kernel BUG at fs/ext4/extents_status.c:762!
>> invalid opcode: 0000 [#1] SMP KASAN PTI
>> CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
>> RIP: 0010:ext4_es_cache_extent+0x30e/0x370
>> [...]
>> Call Trace:
>>   ext4_cache_extents+0x238/0x2f0
>>   ext4_find_extent+0x785/0xa40
>>   ext4_fiemap+0x36d/0xe90
>>   do_vfs_ioctl+0x6af/0x1200
>> [...]
>> ==================================================================
>>
>> Above issue may happen as follows:
>> -------------------------------------
>>             cpu1		    cpu2
>> _____________________|_____________________
>> do_vfs_ioctl
>>   ext4_ioctl
>>    ext4_ioctl_setflags
>>     ext4_ind_migrate
>>                          do_vfs_ioctl
>>                           ioctl_fiemap
>>                            ext4_fiemap
>>                             ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
>>                             ext4_fill_fiemap_extents
>>      down_write(&EXT4_I(inode)->i_data_sem);
>>      ext4_ext_check_inode
>>      ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
>>      memset(ei->i_data, 0, sizeof(ei->i_data))
>>      up_write(&EXT4_I(inode)->i_data_sem);
>>                              down_read(&EXT4_I(inode)->i_data_sem);
>>                              ext4_find_extent
>>                               ext4_cache_extents
>>                                ext4_es_cache_extent
>>                                 BUG_ON(end < lblk)
>>
>> We can easily reproduce this problem with the syzkaller testcase:
>> ```
>> 02:37:07 executing program 3:
>> r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
>> ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
>> mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
>> r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
>> ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
>> ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
>> ```
>>
>> To solve this issue, we use __generic_block_fiemap() instead of
>> generic_block_fiemap() and add inode_lock_shared to avoid race condition.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   fs/ext4/extents.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
> What is the git commit id of this change in Linus's tree?
>
> If it is not in Linus's tree, why not?
>
> confused,
>
> greg k-h
> .

This patch does not exist in the Linus' tree.

This problem persists until the patch d3b6f23f7167("ext4: move 
ext4_fiemap to use iomap framework") is incorporated in v5.7-rc1.

However, this problem can be found by asserting  after patch 
4068664e3cd2 ("ext4: fix extent_status fragmentation for plain files") 
is incorporated into v5.6-rc1.

If someone don't want to convert ext4_fiemap to iomap, this patch may help.

I also CC linux-ext4 in hopes of getting some advice.

Feel free to improve it if something wrong.

Thanks a lot!

-- 
With Best Regards,
Baokun Li

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Greg KH]

On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
> 在 2022/7/15 22:10, Greg KH 写道:
> > On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
> > > This patch and problem analysis is based on v4.19 LTS.
> > > The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
> > > is incorporated in v5.7-rc1. This patch avoids this problem by switching
> > > to iomap in ext4_fiemap.
> > > 
> > > Hulk Robot reported a BUG on stable 4.19.252:
> > > ==================================================================
> > > kernel BUG at fs/ext4/extents_status.c:762!
> > > invalid opcode: 0000 [#1] SMP KASAN PTI
> > > CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
> > > RIP: 0010:ext4_es_cache_extent+0x30e/0x370
> > > [...]
> > > Call Trace:
> > >   ext4_cache_extents+0x238/0x2f0
> > >   ext4_find_extent+0x785/0xa40
> > >   ext4_fiemap+0x36d/0xe90
> > >   do_vfs_ioctl+0x6af/0x1200
> > > [...]
> > > ==================================================================
> > > 
> > > Above issue may happen as follows:
> > > -------------------------------------
> > >             cpu1		    cpu2
> > > _____________________|_____________________
> > > do_vfs_ioctl
> > >   ext4_ioctl
> > >    ext4_ioctl_setflags
> > >     ext4_ind_migrate
> > >                          do_vfs_ioctl
> > >                           ioctl_fiemap
> > >                            ext4_fiemap
> > >                             ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
> > >                             ext4_fill_fiemap_extents
> > >      down_write(&EXT4_I(inode)->i_data_sem);
> > >      ext4_ext_check_inode
> > >      ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
> > >      memset(ei->i_data, 0, sizeof(ei->i_data))
> > >      up_write(&EXT4_I(inode)->i_data_sem);
> > >                              down_read(&EXT4_I(inode)->i_data_sem);
> > >                              ext4_find_extent
> > >                               ext4_cache_extents
> > >                                ext4_es_cache_extent
> > >                                 BUG_ON(end < lblk)
> > > 
> > > We can easily reproduce this problem with the syzkaller testcase:
> > > ```
> > > 02:37:07 executing program 3:
> > > r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
> > > ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
> > > mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
> > > r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
> > > ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
> > > ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
> > > ```
> > > 
> > > To solve this issue, we use __generic_block_fiemap() instead of
> > > generic_block_fiemap() and add inode_lock_shared to avoid race condition.
> > > 
> > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > ---
> > >   fs/ext4/extents.c | 15 +++++++++++----
> > >   1 file changed, 11 insertions(+), 4 deletions(-)
> > What is the git commit id of this change in Linus's tree?
> > 
> > If it is not in Linus's tree, why not?
> > 
> > confused,
> > 
> > greg k-h
> > .
> 
> This patch does not exist in the Linus' tree.
> 
> This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
> to use iomap framework") is incorporated in v5.7-rc1.

Then why not ask for that change to be added instead?

thanks,

greg k-h

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Baokun Li]

在 2022/7/19 19:26, Greg KH 写道:
> On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
>> 在 2022/7/15 22:10, Greg KH 写道:
>>> On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
>>>> This patch and problem analysis is based on v4.19 LTS.
>>>> The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
>>>> is incorporated in v5.7-rc1. This patch avoids this problem by switching
>>>> to iomap in ext4_fiemap.
>>>>
>>>> Hulk Robot reported a BUG on stable 4.19.252:
>>>> ==================================================================
>>>> kernel BUG at fs/ext4/extents_status.c:762!
>>>> invalid opcode: 0000 [#1] SMP KASAN PTI
>>>> CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
>>>> RIP: 0010:ext4_es_cache_extent+0x30e/0x370
>>>> [...]
>>>> Call Trace:
>>>>    ext4_cache_extents+0x238/0x2f0
>>>>    ext4_find_extent+0x785/0xa40
>>>>    ext4_fiemap+0x36d/0xe90
>>>>    do_vfs_ioctl+0x6af/0x1200
>>>> [...]
>>>> ==================================================================
>>>>
>>>> Above issue may happen as follows:
>>>> -------------------------------------
>>>>              cpu1		    cpu2
>>>> _____________________|_____________________
>>>> do_vfs_ioctl
>>>>    ext4_ioctl
>>>>     ext4_ioctl_setflags
>>>>      ext4_ind_migrate
>>>>                           do_vfs_ioctl
>>>>                            ioctl_fiemap
>>>>                             ext4_fiemap
>>>>                              ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
>>>>                              ext4_fill_fiemap_extents
>>>>       down_write(&EXT4_I(inode)->i_data_sem);
>>>>       ext4_ext_check_inode
>>>>       ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
>>>>       memset(ei->i_data, 0, sizeof(ei->i_data))
>>>>       up_write(&EXT4_I(inode)->i_data_sem);
>>>>                               down_read(&EXT4_I(inode)->i_data_sem);
>>>>                               ext4_find_extent
>>>>                                ext4_cache_extents
>>>>                                 ext4_es_cache_extent
>>>>                                  BUG_ON(end < lblk)
>>>>
>>>> We can easily reproduce this problem with the syzkaller testcase:
>>>> ```
>>>> 02:37:07 executing program 3:
>>>> r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
>>>> ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
>>>> mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
>>>> r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
>>>> ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
>>>> ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
>>>> ```
>>>>
>>>> To solve this issue, we use __generic_block_fiemap() instead of
>>>> generic_block_fiemap() and add inode_lock_shared to avoid race condition.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>> ---
>>>>    fs/ext4/extents.c | 15 +++++++++++----
>>>>    1 file changed, 11 insertions(+), 4 deletions(-)
>>> What is the git commit id of this change in Linus's tree?
>>>
>>> If it is not in Linus's tree, why not?
>>>
>>> confused,
>>>
>>> greg k-h
>>> .
>> This patch does not exist in the Linus' tree.
>>
>> This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
>> to use iomap framework") is incorporated in v5.7-rc1.
> Then why not ask for that change to be added instead?
>
> thanks,
>
> greg k-h
> .

If we want to switch to the iomap framework, we need to analyze and 
integrate about 60 patches.

The workload may be greater than that of solving this problem alone.

Thank you!

Thanks a lot!

-- 
With Best Regards,
Baokun Li


-- 
With Best Regards,
Baokun Li

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Greg KH]

On Tue, Jul 19, 2022 at 08:15:13PM +0800, Baokun Li wrote:
> 在 2022/7/19 19:26, Greg KH 写道:
> > On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
> > > 在 2022/7/15 22:10, Greg KH 写道:
> > > > On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
> > > > > This patch and problem analysis is based on v4.19 LTS.
> > > > > The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
> > > > > is incorporated in v5.7-rc1. This patch avoids this problem by switching
> > > > > to iomap in ext4_fiemap.
> > > > > 
> > > > > Hulk Robot reported a BUG on stable 4.19.252:
> > > > > ==================================================================
> > > > > kernel BUG at fs/ext4/extents_status.c:762!
> > > > > invalid opcode: 0000 [#1] SMP KASAN PTI
> > > > > CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
> > > > > RIP: 0010:ext4_es_cache_extent+0x30e/0x370
> > > > > [...]
> > > > > Call Trace:
> > > > >    ext4_cache_extents+0x238/0x2f0
> > > > >    ext4_find_extent+0x785/0xa40
> > > > >    ext4_fiemap+0x36d/0xe90
> > > > >    do_vfs_ioctl+0x6af/0x1200
> > > > > [...]
> > > > > ==================================================================
> > > > > 
> > > > > Above issue may happen as follows:
> > > > > -------------------------------------
> > > > >              cpu1		    cpu2
> > > > > _____________________|_____________________
> > > > > do_vfs_ioctl
> > > > >    ext4_ioctl
> > > > >     ext4_ioctl_setflags
> > > > >      ext4_ind_migrate
> > > > >                           do_vfs_ioctl
> > > > >                            ioctl_fiemap
> > > > >                             ext4_fiemap
> > > > >                              ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
> > > > >                              ext4_fill_fiemap_extents
> > > > >       down_write(&EXT4_I(inode)->i_data_sem);
> > > > >       ext4_ext_check_inode
> > > > >       ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
> > > > >       memset(ei->i_data, 0, sizeof(ei->i_data))
> > > > >       up_write(&EXT4_I(inode)->i_data_sem);
> > > > >                               down_read(&EXT4_I(inode)->i_data_sem);
> > > > >                               ext4_find_extent
> > > > >                                ext4_cache_extents
> > > > >                                 ext4_es_cache_extent
> > > > >                                  BUG_ON(end < lblk)
> > > > > 
> > > > > We can easily reproduce this problem with the syzkaller testcase:
> > > > > ```
> > > > > 02:37:07 executing program 3:
> > > > > r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
> > > > > ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
> > > > > mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
> > > > > r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
> > > > > ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
> > > > > ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
> > > > > ```
> > > > > 
> > > > > To solve this issue, we use __generic_block_fiemap() instead of
> > > > > generic_block_fiemap() and add inode_lock_shared to avoid race condition.
> > > > > 
> > > > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > > > ---
> > > > >    fs/ext4/extents.c | 15 +++++++++++----
> > > > >    1 file changed, 11 insertions(+), 4 deletions(-)
> > > > What is the git commit id of this change in Linus's tree?
> > > > 
> > > > If it is not in Linus's tree, why not?
> > > > 
> > > > confused,
> > > > 
> > > > greg k-h
> > > > .
> > > This patch does not exist in the Linus' tree.
> > > 
> > > This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
> > > to use iomap framework") is incorporated in v5.7-rc1.
> > Then why not ask for that change to be added instead?
> > 
> > thanks,
> > 
> > greg k-h
> > .
> 
> If we want to switch to the iomap framework, we need to analyze and
> integrate about 60 patches.
> 
> The workload may be greater than that of solving this problem alone.

95% of the time we take a patch that is not in Linus's tree, it is buggy
and causes problems in the long run.  See what those 60 patches really
require and if this issue really does need all of that.

Or better yet, take the effort here and move off of 4.19 to a newer
kernel without this problem in it.  What is preventing you from doing
that today?  4.19 is not going to be around for forever, and will
probably not even be getting fixes for stuff like RETBLEED, so are you
_SURE_ you want to keep using it?

thanks,

greg k-h

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Theodore Ts'o]

On Tue, Jul 19, 2022 at 01:26:57PM +0200, Greg KH wrote:
> On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
> > This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
> > to use iomap framework") is incorporated in v5.7-rc1.
> 
> Then why not ask for that change to be added instead?

Switching over to use the iomap framework is a quite invasive change,
which is fraught with danager and potential performance regressions.
So it's really not something that would be considered safe for an LTS
kernel.

As an upstream developer I'd ask why are people trying to use a kernel
as old as 4.19, but RHEL has done more insane things than that.  Also,
I know what the answer is, and it's just too depressing for a nice
summer day like this.  :-)

       	  	    	     	       - Ted

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Theodore Ts'o]

On Tue, Jul 19, 2022 at 02:25:18PM +0200, Greg KH wrote:
> 
> 95% of the time we take a patch that is not in Linus's tree, it is buggy
> and causes problems in the long run.

So if we really want a 4.19 LTS specific patch, I'd be OK with signing
off on it from an ext4 perspective.... IF AND ONLY IF someone is
willing to tell me that they ran "kvm-xfstests -c ext4/all -g auto" or
the equivalent before and after applying the patch, and is willing to
certify that there are no test regressions.

Helpful links:

  * https://thunk.org/gce-xfstests
  * https://thunk.org/android-xfstests
  * Documentation links from https://github.com/tytso/xfstests-bld
      * https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
      * https://github.com/tytso/xfstests-bld/blob/master/Documentation/gce-xfstests.md

(Note that running "-c ext4/all -g auto" will take some 12+ hours if
the tests are run serially, which is why using gce-xfstests's
lightweight test manager to run the file system test configurations in
parallel is a big win.)

> Or better yet, take the effort here and move off of 4.19 to a newer
> kernel without this problem in it.  What is preventing you from doing
> that today?  4.19 is not going to be around for forever, and will
> probably not even be getting fixes for stuff like RETBLEED, so are you
> _SURE_ you want to keep using it?

Or yeah, maybe it's better/cheaper/time for you to move off of 4.19.  :-)

   	       	    	    	       - Ted

P.S.  If we go down this path, Greg K-H may also insist on getting the
bug fix to the 5.4 LTS kernel, so that a bug isn't just fixed in 4.19
LTS but not 5.4 LTS.  In which case, the same requirement of running
"-c ext4/all -g auto" and showing that there are no test regressions
is going to be a requirement for 5.4 LTS as well.

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Greg KH]

On Tue, Jul 19, 2022 at 02:57:18PM -0400, Theodore Ts'o wrote:
> P.S.  If we go down this path, Greg K-H may also insist on getting the
> bug fix to the 5.4 LTS kernel, so that a bug isn't just fixed in 4.19
> LTS but not 5.4 LTS.  In which case, the same requirement of running
> "-c ext4/all -g auto" and showing that there are no test regressions
> is going to be a requirement for 5.4 LTS as well.

Yes, if this issue is also in 5.4 or any newer kernels, it HAS to be
fixed there at the same time, or before, as we can not have anyone
upgrading from an older kernel to a newer one and having a regression.

thanks,

greg k-h

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Baokun Li]

在 2022/7/19 22:06, Theodore Ts'o 写道:
> On Tue, Jul 19, 2022 at 01:26:57PM +0200, Greg KH wrote:
>> On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
>>> This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
>>> to use iomap framework") is incorporated in v5.7-rc1.
>> Then why not ask for that change to be added instead?
> Switching over to use the iomap framework is a quite invasive change,
> which is fraught with danager and potential performance regressions.
> So it's really not something that would be considered safe for an LTS
> kernel.
>
> As an upstream developer I'd ask why are people trying to use a kernel
> as old as 4.19, but RHEL has done more insane things than that.  Also,
> I know what the answer is, and it's just too depressing for a nice
> summer day like this.  :-)
>
>         	  	    	     	       - Ted
> .

Indeed.

-- 
With Best Regards,
Baokun Li

Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4_fiemap

[Baokun Li]

在 2022/7/20 3:14, Greg KH 写道:
> On Tue, Jul 19, 2022 at 02:57:18PM -0400, Theodore Ts'o wrote:
>> P.S.  If we go down this path, Greg K-H may also insist on getting the
>> bug fix to the 5.4 LTS kernel, so that a bug isn't just fixed in 4.19
>> LTS but not 5.4 LTS.  In which case, the same requirement of running
>> "-c ext4/all -g auto" and showing that there are no test regressions
>> is going to be a requirement for 5.4 LTS as well.
> Yes, if this issue is also in 5.4 or any newer kernels, it HAS to be
> fixed there at the same time, or before, as we can not have anyone
> upgrading from an older kernel to a newer one and having a regression.
>
> thanks,
>
> greg k-h
> .

Indeed.

-- 
With Best Regards,
Baokun Li