[PATCH v2 0/2] ext4: fix two bug_on in __es_tree_search

[PATCH v2 0/2] ext4: fix two bug_on in __es_tree_search

[Baokun Li]

V1->V2:
	In patch 2, when imode is not set to S_IFREG, the inode also needs
	to be initialized. Otherwise, the check can be bypassed, causing
	the BUG_ON. (found in the review by yangerkun)

Baokun Li (2):
  ext4: fix bug_on in __es_tree_search caused by wrong s_usr_quota_inum
  ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

 fs/ext4/ioctl.c | 2 +-
 fs/ext4/super.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] ext4: fix bug_on in __es_tree_search caused by wrong s_usr_quota_inum

[Baokun Li]

We got a issue as fllows:
==================================================================
 kernel BUG at fs/ext4/extents_status.c:202!
 invalid opcode: 0000 [#1] PREEMPT SMP
 CPU: 1 PID: 810 Comm: mount Not tainted 6.1.0-rc1-next-g9631525255e3 #352
 RIP: 0010:__es_tree_search.isra.0+0xb8/0xe0
 RSP: 0018:ffffc90001227900 EFLAGS: 00010202
 RAX: 0000000000000000 RBX: 0000000077512a0f RCX: 0000000000000000
 RDX: 0000000000000002 RSI: 0000000000002a10 RDI: ffff8881004cd0c8
 RBP: ffff888177512ac8 R08: 47ffffffffffffff R09: 0000000000000001
 R10: 0000000000000001 R11: 00000000000679af R12: 0000000000002a10
 R13: ffff888177512d88 R14: 0000000077512a10 R15: 0000000000000000
 FS: 00007f4bd76dbc40(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00005653bf993cf8 CR3: 000000017bfdf000 CR4: 00000000000006e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  <TASK>
  ext4_es_cache_extent+0xe2/0x210
  ext4_cache_extents+0xd2/0x110
  ext4_find_extent+0x5d5/0x8c0
  ext4_ext_map_blocks+0x9c/0x1d30
  ext4_map_blocks+0x431/0xa50
  ext4_getblk+0x82/0x340
  ext4_bread+0x14/0x110
  ext4_quota_read+0xf0/0x180
  v2_read_header+0x24/0x90
  v2_check_quota_file+0x2f/0xa0
  dquot_load_quota_sb+0x26c/0x760
  dquot_load_quota_inode+0xa5/0x190
  ext4_enable_quotas+0x14c/0x300
  __ext4_fill_super+0x31cc/0x32c0
  ext4_fill_super+0x115/0x2d0
  get_tree_bdev+0x1d2/0x360
  ext4_get_tree+0x19/0x30
  vfs_get_tree+0x26/0xe0
  path_mount+0x81d/0xfc0
  do_mount+0x8d/0xc0
  __x64_sys_mount+0xc0/0x160
  do_syscall_64+0x35/0x80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd
  </TASK>
==================================================================

Above issue may happen as follows:
-------------------------------------
ext4_fill_super
 ext4_orphan_cleanup
  ext4_enable_quotas
   ext4_quota_enable
    ext4_iget --> get error inode <5>
     ext4_ext_check_inode --> Wrong imode makes it escape inspection
     make_bad_inode(inode) --> EXT4_BOOT_LOADER_INO set imode
    dquot_load_quota_inode
     vfs_setup_quota_inode --> check pass
     dquot_load_quota_sb
      v2_check_quota_file
       v2_read_header
        ext4_quota_read
         ext4_bread
          ext4_getblk
           ext4_map_blocks
            ext4_ext_map_blocks
             ext4_find_extent
              ext4_cache_extents
               ext4_es_cache_extent
                __es_tree_search.isra.0
                 ext4_es_end --> Wrong extents trigger BUG_ON

In the above issue, s_usr_quota_inum is set to 5, but inode<5> contains
incorrect imode and disordered extents. Because 5 is EXT4_BOOT_LOADER_INO,
the ext4_ext_check_inode check in the ext4_iget function can be bypassed,
finally, the extents that are not checked trigger the BUG_ON in the
__es_tree_search function. To solve this issue, check whether qf_inode
obtained by ext4_iget is bad_inode.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7a57dadfe256..5a5f95c6a0cb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6907,9 +6907,9 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
 		return -EPERM;
 
 	qf_inode = ext4_iget(sb, qf_inums[type], EXT4_IGET_SPECIAL);
-	if (IS_ERR(qf_inode)) {
+	if (IS_ERR(qf_inode) || is_bad_inode(qf_inode)) {
 		ext4_error(sb, "Bad quota inode # %lu", qf_inums[type]);
-		return PTR_ERR(qf_inode);
+		return IS_ERR(qf_inode) ? PTR_ERR(qf_inode) : -EUCLEAN;
 	}
 
 	/* Don't account quota for quota files to avoid recursion */
-- 
2.31.1

Re: [PATCH v2 1/2] ext4: fix bug_on in __es_tree_search caused by wrong s_usr_quota_inum

[Jan Kara]

On Fri 21-10-22 12:07:30, Baokun Li wrote:
> We got a issue as fllows:
> ==================================================================
>  kernel BUG at fs/ext4/extents_status.c:202!
>  invalid opcode: 0000 [#1] PREEMPT SMP
>  CPU: 1 PID: 810 Comm: mount Not tainted 6.1.0-rc1-next-g9631525255e3 #352
>  RIP: 0010:__es_tree_search.isra.0+0xb8/0xe0
>  RSP: 0018:ffffc90001227900 EFLAGS: 00010202
>  RAX: 0000000000000000 RBX: 0000000077512a0f RCX: 0000000000000000
>  RDX: 0000000000000002 RSI: 0000000000002a10 RDI: ffff8881004cd0c8
>  RBP: ffff888177512ac8 R08: 47ffffffffffffff R09: 0000000000000001
>  R10: 0000000000000001 R11: 00000000000679af R12: 0000000000002a10
>  R13: ffff888177512d88 R14: 0000000077512a10 R15: 0000000000000000
>  FS: 00007f4bd76dbc40(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00005653bf993cf8 CR3: 000000017bfdf000 CR4: 00000000000006e0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   <TASK>
>   ext4_es_cache_extent+0xe2/0x210
>   ext4_cache_extents+0xd2/0x110
>   ext4_find_extent+0x5d5/0x8c0
>   ext4_ext_map_blocks+0x9c/0x1d30
>   ext4_map_blocks+0x431/0xa50
>   ext4_getblk+0x82/0x340
>   ext4_bread+0x14/0x110
>   ext4_quota_read+0xf0/0x180
>   v2_read_header+0x24/0x90
>   v2_check_quota_file+0x2f/0xa0
>   dquot_load_quota_sb+0x26c/0x760
>   dquot_load_quota_inode+0xa5/0x190
>   ext4_enable_quotas+0x14c/0x300
>   __ext4_fill_super+0x31cc/0x32c0
>   ext4_fill_super+0x115/0x2d0
>   get_tree_bdev+0x1d2/0x360
>   ext4_get_tree+0x19/0x30
>   vfs_get_tree+0x26/0xe0
>   path_mount+0x81d/0xfc0
>   do_mount+0x8d/0xc0
>   __x64_sys_mount+0xc0/0x160
>   do_syscall_64+0x35/0x80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>   </TASK>
> ==================================================================
> 
> Above issue may happen as follows:
> -------------------------------------
> ext4_fill_super
>  ext4_orphan_cleanup
>   ext4_enable_quotas
>    ext4_quota_enable
>     ext4_iget --> get error inode <5>
>      ext4_ext_check_inode --> Wrong imode makes it escape inspection
>      make_bad_inode(inode) --> EXT4_BOOT_LOADER_INO set imode
>     dquot_load_quota_inode
>      vfs_setup_quota_inode --> check pass
>      dquot_load_quota_sb
>       v2_check_quota_file
>        v2_read_header
>         ext4_quota_read
>          ext4_bread
>           ext4_getblk
>            ext4_map_blocks
>             ext4_ext_map_blocks
>              ext4_find_extent
>               ext4_cache_extents
>                ext4_es_cache_extent
>                 __es_tree_search.isra.0
>                  ext4_es_end --> Wrong extents trigger BUG_ON
> 
> In the above issue, s_usr_quota_inum is set to 5, but inode<5> contains
> incorrect imode and disordered extents. Because 5 is EXT4_BOOT_LOADER_INO,
> the ext4_ext_check_inode check in the ext4_iget function can be bypassed,
> finally, the extents that are not checked trigger the BUG_ON in the
> __es_tree_search function. To solve this issue, check whether qf_inode
> obtained by ext4_iget is bad_inode.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7a57dadfe256..5a5f95c6a0cb 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6907,9 +6907,9 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
>  		return -EPERM;
>  
>  	qf_inode = ext4_iget(sb, qf_inums[type], EXT4_IGET_SPECIAL);
> -	if (IS_ERR(qf_inode)) {
> +	if (IS_ERR(qf_inode) || is_bad_inode(qf_inode)) {
>  		ext4_error(sb, "Bad quota inode # %lu", qf_inums[type]);
> -		return PTR_ERR(qf_inode);
> +		return IS_ERR(qf_inode) ? PTR_ERR(qf_inode) : -EUCLEAN;
>  	}
>  
>  	/* Don't account quota for quota files to avoid recursion */
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/2] ext4: fix bug_on in __es_tree_search caused by wrong s_usr_quota_inum

[Jan Kara]

On Mon 24-10-22 16:10:53, Jan Kara wrote:
> On Fri 21-10-22 12:07:30, Baokun Li wrote:
> > We got a issue as fllows:
> > ==================================================================
> >  kernel BUG at fs/ext4/extents_status.c:202!
> >  invalid opcode: 0000 [#1] PREEMPT SMP
> >  CPU: 1 PID: 810 Comm: mount Not tainted 6.1.0-rc1-next-g9631525255e3 #352
> >  RIP: 0010:__es_tree_search.isra.0+0xb8/0xe0
> >  RSP: 0018:ffffc90001227900 EFLAGS: 00010202
> >  RAX: 0000000000000000 RBX: 0000000077512a0f RCX: 0000000000000000
> >  RDX: 0000000000000002 RSI: 0000000000002a10 RDI: ffff8881004cd0c8
> >  RBP: ffff888177512ac8 R08: 47ffffffffffffff R09: 0000000000000001
> >  R10: 0000000000000001 R11: 00000000000679af R12: 0000000000002a10
> >  R13: ffff888177512d88 R14: 0000000077512a10 R15: 0000000000000000
> >  FS: 00007f4bd76dbc40(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >  CR2: 00005653bf993cf8 CR3: 000000017bfdf000 CR4: 00000000000006e0
> >  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >  Call Trace:
> >   <TASK>
> >   ext4_es_cache_extent+0xe2/0x210
> >   ext4_cache_extents+0xd2/0x110
> >   ext4_find_extent+0x5d5/0x8c0
> >   ext4_ext_map_blocks+0x9c/0x1d30
> >   ext4_map_blocks+0x431/0xa50
> >   ext4_getblk+0x82/0x340
> >   ext4_bread+0x14/0x110
> >   ext4_quota_read+0xf0/0x180
> >   v2_read_header+0x24/0x90
> >   v2_check_quota_file+0x2f/0xa0
> >   dquot_load_quota_sb+0x26c/0x760
> >   dquot_load_quota_inode+0xa5/0x190
> >   ext4_enable_quotas+0x14c/0x300
> >   __ext4_fill_super+0x31cc/0x32c0
> >   ext4_fill_super+0x115/0x2d0
> >   get_tree_bdev+0x1d2/0x360
> >   ext4_get_tree+0x19/0x30
> >   vfs_get_tree+0x26/0xe0
> >   path_mount+0x81d/0xfc0
> >   do_mount+0x8d/0xc0
> >   __x64_sys_mount+0xc0/0x160
> >   do_syscall_64+0x35/0x80
> >   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >   </TASK>
> > ==================================================================
> > 
> > Above issue may happen as follows:
> > -------------------------------------
> > ext4_fill_super
> >  ext4_orphan_cleanup
> >   ext4_enable_quotas
> >    ext4_quota_enable
> >     ext4_iget --> get error inode <5>
> >      ext4_ext_check_inode --> Wrong imode makes it escape inspection
> >      make_bad_inode(inode) --> EXT4_BOOT_LOADER_INO set imode
> >     dquot_load_quota_inode
> >      vfs_setup_quota_inode --> check pass
> >      dquot_load_quota_sb
> >       v2_check_quota_file
> >        v2_read_header
> >         ext4_quota_read
> >          ext4_bread
> >           ext4_getblk
> >            ext4_map_blocks
> >             ext4_ext_map_blocks
> >              ext4_find_extent
> >               ext4_cache_extents
> >                ext4_es_cache_extent
> >                 __es_tree_search.isra.0
> >                  ext4_es_end --> Wrong extents trigger BUG_ON
> > 
> > In the above issue, s_usr_quota_inum is set to 5, but inode<5> contains
> > incorrect imode and disordered extents. Because 5 is EXT4_BOOT_LOADER_INO,
> > the ext4_ext_check_inode check in the ext4_iget function can be bypassed,
> > finally, the extents that are not checked trigger the BUG_ON in the
> > __es_tree_search function. To solve this issue, check whether qf_inode
> > obtained by ext4_iget is bad_inode.
> > 
> > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> 
> Looks good to me. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Hum, on a second thought: Would not it be better if vfs_setup_quota_inode()
actually checked for bad inode and refused it with EUCLEAN? That would
sound like a more generic approach (covers all filesystems) to this
problem.

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

Re: [PATCH v2 1/2] ext4: fix bug_on in __es_tree_search caused by wrong s_usr_quota_inum

[Baokun Li]

On 2022/10/24 22:16, Jan Kara wrote:
> On Mon 24-10-22 16:10:53, Jan Kara wrote:
>> On Fri 21-10-22 12:07:30, Baokun Li wrote:
>>> We got a issue as fllows:
>>> ==================================================================
>>>   kernel BUG at fs/ext4/extents_status.c:202!
>>>   invalid opcode: 0000 [#1] PREEMPT SMP
>>>   CPU: 1 PID: 810 Comm: mount Not tainted 6.1.0-rc1-next-g9631525255e3 #352
>>>   RIP: 0010:__es_tree_search.isra.0+0xb8/0xe0
>>>   RSP: 0018:ffffc90001227900 EFLAGS: 00010202
>>>   RAX: 0000000000000000 RBX: 0000000077512a0f RCX: 0000000000000000
>>>   RDX: 0000000000000002 RSI: 0000000000002a10 RDI: ffff8881004cd0c8
>>>   RBP: ffff888177512ac8 R08: 47ffffffffffffff R09: 0000000000000001
>>>   R10: 0000000000000001 R11: 00000000000679af R12: 0000000000002a10
>>>   R13: ffff888177512d88 R14: 0000000077512a10 R15: 0000000000000000
>>>   FS: 00007f4bd76dbc40(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
>>>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>   CR2: 00005653bf993cf8 CR3: 000000017bfdf000 CR4: 00000000000006e0
>>>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>   Call Trace:
>>>    <TASK>
>>>    ext4_es_cache_extent+0xe2/0x210
>>>    ext4_cache_extents+0xd2/0x110
>>>    ext4_find_extent+0x5d5/0x8c0
>>>    ext4_ext_map_blocks+0x9c/0x1d30
>>>    ext4_map_blocks+0x431/0xa50
>>>    ext4_getblk+0x82/0x340
>>>    ext4_bread+0x14/0x110
>>>    ext4_quota_read+0xf0/0x180
>>>    v2_read_header+0x24/0x90
>>>    v2_check_quota_file+0x2f/0xa0
>>>    dquot_load_quota_sb+0x26c/0x760
>>>    dquot_load_quota_inode+0xa5/0x190
>>>    ext4_enable_quotas+0x14c/0x300
>>>    __ext4_fill_super+0x31cc/0x32c0
>>>    ext4_fill_super+0x115/0x2d0
>>>    get_tree_bdev+0x1d2/0x360
>>>    ext4_get_tree+0x19/0x30
>>>    vfs_get_tree+0x26/0xe0
>>>    path_mount+0x81d/0xfc0
>>>    do_mount+0x8d/0xc0
>>>    __x64_sys_mount+0xc0/0x160
>>>    do_syscall_64+0x35/0x80
>>>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>>    </TASK>
>>> ==================================================================
>>>
>>> Above issue may happen as follows:
>>> -------------------------------------
>>> ext4_fill_super
>>>   ext4_orphan_cleanup
>>>    ext4_enable_quotas
>>>     ext4_quota_enable
>>>      ext4_iget --> get error inode <5>
>>>       ext4_ext_check_inode --> Wrong imode makes it escape inspection
>>>       make_bad_inode(inode) --> EXT4_BOOT_LOADER_INO set imode
>>>      dquot_load_quota_inode
>>>       vfs_setup_quota_inode --> check pass
>>>       dquot_load_quota_sb
>>>        v2_check_quota_file
>>>         v2_read_header
>>>          ext4_quota_read
>>>           ext4_bread
>>>            ext4_getblk
>>>             ext4_map_blocks
>>>              ext4_ext_map_blocks
>>>               ext4_find_extent
>>>                ext4_cache_extents
>>>                 ext4_es_cache_extent
>>>                  __es_tree_search.isra.0
>>>                   ext4_es_end --> Wrong extents trigger BUG_ON
>>>
>>> In the above issue, s_usr_quota_inum is set to 5, but inode<5> contains
>>> incorrect imode and disordered extents. Because 5 is EXT4_BOOT_LOADER_INO,
>>> the ext4_ext_check_inode check in the ext4_iget function can be bypassed,
>>> finally, the extents that are not checked trigger the BUG_ON in the
>>> __es_tree_search function. To solve this issue, check whether qf_inode
>>> obtained by ext4_iget is bad_inode.
>>>
>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Looks good to me. Feel free to add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
> Hum, on a second thought: Would not it be better if vfs_setup_quota_inode()
> actually checked for bad inode and refused it with EUCLEAN? That would
> sound like a more generic approach (covers all filesystems) to this
> problem.
>
> 								Honza

Hello Honza,

Totally agree with you! We initially added the check to 
vfs_setup_quota_inode(),
because the check for imode is here. However, in ext4_quota_enable, if 
qf_inode
is abnormal, "Bad quota inode..." is printed. Therefore, we feel that 
adding a check
here can help us quickly find out where the problem occurs.
Perhaps we should still add the check to vfs_setup_quota_inode() and 
also add the
print ino when ext4_enable_quotas fails.


Thank you for your review!
-- 
With Best Regards,
Baokun Li

[PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Baokun Li]

We got a issue as fllows:
==================================================================
 kernel BUG at fs/ext4/extents_status.c:203!
 invalid opcode: 0000 [#1] PREEMPT SMP
 CPU: 1 PID: 945 Comm: cat Not tainted 6.0.0-next-20221007-dirty #349
 RIP: 0010:ext4_es_end.isra.0+0x34/0x42
 RSP: 0018:ffffc9000143b768 EFLAGS: 00010203
 RAX: 0000000000000000 RBX: ffff8881769cd0b8 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: ffffffff8fc27cf7 RDI: 00000000ffffffff
 RBP: ffff8881769cd0bc R08: 0000000000000000 R09: ffffc9000143b5f8
 R10: 0000000000000001 R11: 0000000000000001 R12: ffff8881769cd0a0
 R13: ffff8881768e5668 R14: 00000000768e52f0 R15: 0000000000000000
 FS: 00007f359f7f05c0(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f359f5a2000 CR3: 000000017130c000 CR4: 00000000000006e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  <TASK>
  __es_tree_search.isra.0+0x6d/0xf5
  ext4_es_cache_extent+0xfa/0x230
  ext4_cache_extents+0xd2/0x110
  ext4_find_extent+0x5d5/0x8c0
  ext4_ext_map_blocks+0x9c/0x1d30
  ext4_map_blocks+0x431/0xa50
  ext4_mpage_readpages+0x48e/0xe40
  ext4_readahead+0x47/0x50
  read_pages+0x82/0x530
  page_cache_ra_unbounded+0x199/0x2a0
  do_page_cache_ra+0x47/0x70
  page_cache_ra_order+0x242/0x400
  ondemand_readahead+0x1e8/0x4b0
  page_cache_sync_ra+0xf4/0x110
  filemap_get_pages+0x131/0xb20
  filemap_read+0xda/0x4b0
  generic_file_read_iter+0x13a/0x250
  ext4_file_read_iter+0x59/0x1d0
  vfs_read+0x28f/0x460
  ksys_read+0x73/0x160
  __x64_sys_read+0x1e/0x30
  do_syscall_64+0x35/0x80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd
  </TASK>
==================================================================

In the above issue, ioctl invokes the swap_inode_boot_loader function to
swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
disordered extents, and i_nlink is set to 1. The extents check for inode in
the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
While links_count is set to 1, the extents are not initialized in
swap_inode_boot_loader. After the ioctl command is executed successfully,
the extents are swapped to inode<12>, in this case, run the `cat` command
to view inode<12>. And Bug_ON is triggered due to the incorrect extents.

When the boot loader inode is not initialized, its imode can be one of the
following:
1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
   set to S_IFREG.
2) the imode is good type but not S_IFREG.
3) the imode is S_IFREG.

The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
Therefore, when the boot loader inode is bad_inode or its imode is not
S_IFREG, initialize the inode to avoid triggering the BUG.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index ded535535b27..c41210706ea7 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -425,7 +425,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
 	/* Protect extent tree against block allocations via delalloc */
 	ext4_double_down_write_data_sem(inode, inode_bl);
 
-	if (inode_bl->i_nlink == 0) {
+	if (is_bad_inode(inode_bl) || !S_ISREG(inode_bl->i_mode)) {
 		/* this inode has never been used as a BOOT_LOADER */
 		set_nlink(inode_bl, 1);
 		i_uid_write(inode_bl, 0);
-- 
2.31.1

Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Jan Kara]

On Fri 21-10-22 12:07:31, Baokun Li wrote:
> We got a issue as fllows:
> ==================================================================
>  kernel BUG at fs/ext4/extents_status.c:203!
>  invalid opcode: 0000 [#1] PREEMPT SMP
>  CPU: 1 PID: 945 Comm: cat Not tainted 6.0.0-next-20221007-dirty #349
>  RIP: 0010:ext4_es_end.isra.0+0x34/0x42
>  RSP: 0018:ffffc9000143b768 EFLAGS: 00010203
>  RAX: 0000000000000000 RBX: ffff8881769cd0b8 RCX: 0000000000000000
>  RDX: 0000000000000000 RSI: ffffffff8fc27cf7 RDI: 00000000ffffffff
>  RBP: ffff8881769cd0bc R08: 0000000000000000 R09: ffffc9000143b5f8
>  R10: 0000000000000001 R11: 0000000000000001 R12: ffff8881769cd0a0
>  R13: ffff8881768e5668 R14: 00000000768e52f0 R15: 0000000000000000
>  FS: 00007f359f7f05c0(0000)GS:ffff88842fd00000(0000)knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f359f5a2000 CR3: 000000017130c000 CR4: 00000000000006e0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   <TASK>
>   __es_tree_search.isra.0+0x6d/0xf5
>   ext4_es_cache_extent+0xfa/0x230
>   ext4_cache_extents+0xd2/0x110
>   ext4_find_extent+0x5d5/0x8c0
>   ext4_ext_map_blocks+0x9c/0x1d30
>   ext4_map_blocks+0x431/0xa50
>   ext4_mpage_readpages+0x48e/0xe40
>   ext4_readahead+0x47/0x50
>   read_pages+0x82/0x530
>   page_cache_ra_unbounded+0x199/0x2a0
>   do_page_cache_ra+0x47/0x70
>   page_cache_ra_order+0x242/0x400
>   ondemand_readahead+0x1e8/0x4b0
>   page_cache_sync_ra+0xf4/0x110
>   filemap_get_pages+0x131/0xb20
>   filemap_read+0xda/0x4b0
>   generic_file_read_iter+0x13a/0x250
>   ext4_file_read_iter+0x59/0x1d0
>   vfs_read+0x28f/0x460
>   ksys_read+0x73/0x160
>   __x64_sys_read+0x1e/0x30
>   do_syscall_64+0x35/0x80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>   </TASK>
> ==================================================================
> 
> In the above issue, ioctl invokes the swap_inode_boot_loader function to
> swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
> disordered extents, and i_nlink is set to 1. The extents check for inode in
> the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
> While links_count is set to 1, the extents are not initialized in
> swap_inode_boot_loader. After the ioctl command is executed successfully,
> the extents are swapped to inode<12>, in this case, run the `cat` command
> to view inode<12>. And Bug_ON is triggered due to the incorrect extents.
> 
> When the boot loader inode is not initialized, its imode can be one of the
> following:
> 1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
>    set to S_IFREG.
> 2) the imode is good type but not S_IFREG.
> 3) the imode is S_IFREG.
> 
> The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
> Therefore, when the boot loader inode is bad_inode or its imode is not
> S_IFREG, initialize the inode to avoid triggering the BUG.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Grepping for calls to ext4_iget() in the ext4 code shows there are many
more places that will get unhappy (and crash) when ext4_iget() returns a
bad inode. In fact, I didn't find a place when returning bad inode would be
useful for anything. So why don't we just return EFSCORRUPTED instead of
returning a bad inode?

								Honza

> ---
>  fs/ext4/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index ded535535b27..c41210706ea7 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -425,7 +425,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
>  	/* Protect extent tree against block allocations via delalloc */
>  	ext4_double_down_write_data_sem(inode, inode_bl);
>  
> -	if (inode_bl->i_nlink == 0) {
> +	if (is_bad_inode(inode_bl) || !S_ISREG(inode_bl->i_mode)) {
>  		/* this inode has never been used as a BOOT_LOADER */
>  		set_nlink(inode_bl, 1);
>  		i_uid_write(inode_bl, 0);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Baokun Li]

On 2022/10/24 22:25, Jan Kara wrote:
> On Fri 21-10-22 12:07:31, Baokun Li wrote:
>> We got a issue as fllows:
>> ...
>>
>> In the above issue, ioctl invokes the swap_inode_boot_loader function to
>> swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
>> disordered extents, and i_nlink is set to 1. The extents check for inode in
>> the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
>> While links_count is set to 1, the extents are not initialized in
>> swap_inode_boot_loader. After the ioctl command is executed successfully,
>> the extents are swapped to inode<12>, in this case, run the `cat` command
>> to view inode<12>. And Bug_ON is triggered due to the incorrect extents.
>>
>> When the boot loader inode is not initialized, its imode can be one of the
>> following:
>> 1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
>>     set to S_IFREG.
>> 2) the imode is good type but not S_IFREG.
>> 3) the imode is S_IFREG.
>>
>> The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
>> Therefore, when the boot loader inode is bad_inode or its imode is not
>> S_IFREG, initialize the inode to avoid triggering the BUG.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Grepping for calls to ext4_iget() in the ext4 code shows there are many
> more places that will get unhappy (and crash) when ext4_iget() returns a
> bad inode. In fact, I didn't find a place when returning bad inode would be
> useful for anything. So why don't we just return EFSCORRUPTED instead of
> returning a bad inode?
>
> 								Honza

Hello Honza,

In ext4_iget(), the inode is marked as bad and returned only when ino is 
equal to
EXT4_BOOT_LOADER_INO. In the error branch bad_inode, although the inode is
marked as bad, the returned value is the corresponding error number.
The boot loader inode is not initialized during mkfs. Therefore, when 
ext4_iget() is
entered for the first time, imode of the inode is bad type. However, the
swap_inode_boot_loader() needs to obtain the inode for initialization 
and swap.
Therefore, a bad_inode is returned in ext4_iget.

Generally, ext4_iget() does not get the boot loader inode. Therefore, we 
only need
to pay attention to the special inodes that can be specified.
The following figure shows the check result:

1) usr_quota_inum/grp_quota_inum/prj_quota_inum
These inodes may be faulty. In the first patch, this situation is 
intercepted.
At the beginning, FUZZ found that the quota inode was faulty. Later, we 
found that
the operation function swap_inode_boot_loader() related to inode 5 was 
also faulty.

2) journal_inum
In ext4_get_journal_inode(), the system checks whether the imode is 
S_IFREG. Then,
the bmap in jbd2_journal_init_inode() checks whether the inode has 
a_ops->bmap
operation. The bad inode does not set the bmap operation, so there is no 
problem.

3) last_orphan
In ext4_orphan_get(), it checks if the imode is normal and if the inode 
is bad inode,
so there is no problem.

4) snapshot_inum
No place to use snapshot_inum was found in the kernel, so there is no 
kernel issue.


>> ---
>>   fs/ext4/ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>> index ded535535b27..c41210706ea7 100644
>> --- a/fs/ext4/ioctl.c
>> +++ b/fs/ext4/ioctl.c
>> @@ -425,7 +425,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
>>   	/* Protect extent tree against block allocations via delalloc */
>>   	ext4_double_down_write_data_sem(inode, inode_bl);
>>   
>> -	if (inode_bl->i_nlink == 0) {
>> +	if (is_bad_inode(inode_bl) || !S_ISREG(inode_bl->i_mode)) {
>>   		/* this inode has never been used as a BOOT_LOADER */
>>   		set_nlink(inode_bl, 1);
>>   		i_uid_write(inode_bl, 0);
>> -- 
>> 2.31.1
>>

Thank you for your review!
-- 
With Best Regards,
Baokun Li

Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Jan Kara]

On Tue 25-10-22 10:26:14, Baokun Li wrote:
> On 2022/10/24 22:25, Jan Kara wrote:
> > On Fri 21-10-22 12:07:31, Baokun Li wrote:
> > > We got a issue as fllows:
> > > ...
> > > 
> > > In the above issue, ioctl invokes the swap_inode_boot_loader function to
> > > swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
> > > disordered extents, and i_nlink is set to 1. The extents check for inode in
> > > the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
> > > While links_count is set to 1, the extents are not initialized in
> > > swap_inode_boot_loader. After the ioctl command is executed successfully,
> > > the extents are swapped to inode<12>, in this case, run the `cat` command
> > > to view inode<12>. And Bug_ON is triggered due to the incorrect extents.
> > > 
> > > When the boot loader inode is not initialized, its imode can be one of the
> > > following:
> > > 1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
> > >     set to S_IFREG.
> > > 2) the imode is good type but not S_IFREG.
> > > 3) the imode is S_IFREG.
> > > 
> > > The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
> > > Therefore, when the boot loader inode is bad_inode or its imode is not
> > > S_IFREG, initialize the inode to avoid triggering the BUG.
> > > 
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > Grepping for calls to ext4_iget() in the ext4 code shows there are many
> > more places that will get unhappy (and crash) when ext4_iget() returns a
> > bad inode. In fact, I didn't find a place when returning bad inode would be
> > useful for anything. So why don't we just return EFSCORRUPTED instead of
> > returning a bad inode?
> > 
> > 								Honza
> 
> Hello Honza,
> 
> In ext4_iget(), the inode is marked as bad and returned only when ino is
> equal to
> EXT4_BOOT_LOADER_INO. In the error branch bad_inode, although the inode is
> marked as bad, the returned value is the corresponding error number.
> The boot loader inode is not initialized during mkfs. Therefore, when
> ext4_iget() is
> entered for the first time, imode of the inode is bad type. However, the
> swap_inode_boot_loader() needs to obtain the inode for initialization and
> swap.
> Therefore, a bad_inode is returned in ext4_iget.
> 
> Generally, ext4_iget() does not get the boot loader inode. Therefore, we
> only need
> to pay attention to the special inodes that can be specified.
> The following figure shows the check result:
> 
> 1) usr_quota_inum/grp_quota_inum/prj_quota_inum
> These inodes may be faulty. In the first patch, this situation is
> intercepted.
> At the beginning, FUZZ found that the quota inode was faulty. Later, we
> found that
> the operation function swap_inode_boot_loader() related to inode 5 was also
> faulty.
> 
> 2) journal_inum
> In ext4_get_journal_inode(), the system checks whether the imode is S_IFREG.
> Then,
> the bmap in jbd2_journal_init_inode() checks whether the inode has
> a_ops->bmap
> operation. The bad inode does not set the bmap operation, so there is no
> problem.
> 
> 3) last_orphan
> In ext4_orphan_get(), it checks if the imode is normal and if the inode is
> bad inode,
> so there is no problem.
> 
> 4) snapshot_inum
> No place to use snapshot_inum was found in the kernel, so there is no kernel
> issue.

Thanks for detailed explanation! Now I agree you have actually covered all
the cases. But since EXT4_BOOT_LOADER_INO has this special behavior maybe
it would be more robust to create a special iget flag for it? Like
EXT4_IGET_BAD? And only with this flag we'd be returning bad inode from
ext4_iget(), otherwise we always return the error code?

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

Re: [PATCH v2 2/2] ext4: fix bug_on in __es_tree_search caused by wrong boot loader inode

[Baokun Li]

On 2022/10/25 17:18, Jan Kara wrote:
> On Tue 25-10-22 10:26:14, Baokun Li wrote:
>> On 2022/10/24 22:25, Jan Kara wrote:
>>> On Fri 21-10-22 12:07:31, Baokun Li wrote:
>>>> We got a issue as fllows:
>>>> ...
>>>>
>>>> In the above issue, ioctl invokes the swap_inode_boot_loader function to
>>>> swap inode<5> and inode<12>. However, inode<5> contain incorrect imode and
>>>> disordered extents, and i_nlink is set to 1. The extents check for inode in
>>>> the ext4_iget function can be bypassed bacause 5 is EXT4_BOOT_LOADER_INO.
>>>> While links_count is set to 1, the extents are not initialized in
>>>> swap_inode_boot_loader. After the ioctl command is executed successfully,
>>>> the extents are swapped to inode<12>, in this case, run the `cat` command
>>>> to view inode<12>. And Bug_ON is triggered due to the incorrect extents.
>>>>
>>>> When the boot loader inode is not initialized, its imode can be one of the
>>>> following:
>>>> 1) the imode is a bad type, which is marked as bad_inode in ext4_iget and
>>>>      set to S_IFREG.
>>>> 2) the imode is good type but not S_IFREG.
>>>> 3) the imode is S_IFREG.
>>>>
>>>> The BUG_ON may be triggered by bypassing the check in cases 1 and 2.
>>>> Therefore, when the boot loader inode is bad_inode or its imode is not
>>>> S_IFREG, initialize the inode to avoid triggering the BUG.
>>>>
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>> Grepping for calls to ext4_iget() in the ext4 code shows there are many
>>> more places that will get unhappy (and crash) when ext4_iget() returns a
>>> bad inode. In fact, I didn't find a place when returning bad inode would be
>>> useful for anything. So why don't we just return EFSCORRUPTED instead of
>>> returning a bad inode?
>>>
>>> 								Honza
>> Hello Honza,
>>
>> In ext4_iget(), the inode is marked as bad and returned only when ino is
>> equal to
>> EXT4_BOOT_LOADER_INO. In the error branch bad_inode, although the inode is
>> marked as bad, the returned value is the corresponding error number.
>> The boot loader inode is not initialized during mkfs. Therefore, when
>> ext4_iget() is
>> entered for the first time, imode of the inode is bad type. However, the
>> swap_inode_boot_loader() needs to obtain the inode for initialization and
>> swap.
>> Therefore, a bad_inode is returned in ext4_iget.
>>
>> Generally, ext4_iget() does not get the boot loader inode. Therefore, we
>> only need
>> to pay attention to the special inodes that can be specified.
>> The following figure shows the check result:
>>
>> 1) usr_quota_inum/grp_quota_inum/prj_quota_inum
>> These inodes may be faulty. In the first patch, this situation is
>> intercepted.
>> At the beginning, FUZZ found that the quota inode was faulty. Later, we
>> found that
>> the operation function swap_inode_boot_loader() related to inode 5 was also
>> faulty.
>>
>> 2) journal_inum
>> In ext4_get_journal_inode(), the system checks whether the imode is S_IFREG.
>> Then,
>> the bmap in jbd2_journal_init_inode() checks whether the inode has
>> a_ops->bmap
>> operation. The bad inode does not set the bmap operation, so there is no
>> problem.
>>
>> 3) last_orphan
>> In ext4_orphan_get(), it checks if the imode is normal and if the inode is
>> bad inode,
>> so there is no problem.
>>
>> 4) snapshot_inum
>> No place to use snapshot_inum was found in the kernel, so there is no kernel
>> issue.
> Thanks for detailed explanation! Now I agree you have actually covered all
> the cases. But since EXT4_BOOT_LOADER_INO has this special behavior maybe
> it would be more robust to create a special iget flag for it? Like
> EXT4_IGET_BAD? And only with this flag we'd be returning bad inode from
> ext4_iget(), otherwise we always return the error code?
>
> 								Honza
Adding a special iget tag sounds great！
Thank you very much for your review！
I will send a patch V2 with the changes suggested by you.

-- 
With Best Regards,
Baokun Li
.