linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search
@ 2022-10-26  4:23 Baokun Li
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Baokun Li @ 2022-10-26  4:23 UTC (permalink / raw)
  To: linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, libaokun1

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)
V2->V3:
	a. add EXT4_IGET_BAD flag to prevent unexpected bad inode.
	b. check bad quota inode in vfs_setup_quota_inode() instead of in
	   ext4_quota_enable() for more generic approach to this problem.
	c. add helper to check quota inums.

Baokun Li (4):
  ext4: fix bug_on in __es_tree_search caused by bad quota inode
  ext4: add helper to check quota inums
  ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode
  ext4: fix bug_on in __es_tree_search caused by bad boot loader inode

 fs/ext4/ext4.h   |  3 ++-
 fs/ext4/inode.c  |  8 +++++++-
 fs/ext4/ioctl.c  |  5 +++--
 fs/ext4/super.c  | 28 +++++++++++++++++++++++++---
 fs/quota/dquot.c |  2 ++
 5 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode
  2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
@ 2022-10-26  4:23 ` Baokun Li
  2022-10-26  9:36   ` Jan Kara
                     ` (2 more replies)
  2022-10-26  4:23 ` [PATCH v3 2/4] ext4: add helper to check quota inums Baokun Li
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 15+ messages in thread
From: Baokun Li @ 2022-10-26  4:23 UTC (permalink / raw)
  To: linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, libaokun1, linux-fsdevel

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 the inode is
bad_inode in vfs_setup_quota_inode().

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

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 0427b44bfee5..f27faf5db554 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2324,6 +2324,8 @@ static int vfs_setup_quota_inode(struct inode *inode, int type)
 	struct super_block *sb = inode->i_sb;
 	struct quota_info *dqopt = sb_dqopt(sb);
 
+	if (is_bad_inode(inode))
+		return -EUCLEAN;
 	if (!S_ISREG(inode->i_mode))
 		return -EACCES;
 	if (IS_RDONLY(inode))
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/4] ext4: add helper to check quota inums
  2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
@ 2022-10-26  4:23 ` Baokun Li
  2022-10-26  9:38   ` Jan Kara
  2022-10-26 13:57   ` Jason Yan
  2022-10-26  4:23 ` [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode Baokun Li
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-10-26  4:23 UTC (permalink / raw)
  To: linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, libaokun1

Before quota is enabled, a check on the preset quota inums in
ext4_super_block is added to prevent wrong quota inodes from being loaded.
In addition, when the quota fails to be enabled, the quota type and quota
inum are printed to facilitate fault locating.

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

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 34b78f380968..0b4060d52d63 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6885,6 +6885,20 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
 	return err;
 }
 
+static inline bool ext4_check_quota_inum(int type, unsigned long qf_inum)
+{
+	switch (type) {
+	case USRQUOTA:
+		return qf_inum == EXT4_USR_QUOTA_INO;
+	case GRPQUOTA:
+		return qf_inum == EXT4_GRP_QUOTA_INO;
+	case PRJQUOTA:
+		return qf_inum >= EXT4_GOOD_OLD_FIRST_INO;
+	default:
+		BUG();
+	}
+}
+
 static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
 			     unsigned int flags)
 {
@@ -6901,9 +6915,16 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
 	if (!qf_inums[type])
 		return -EPERM;
 
+	if (!ext4_check_quota_inum(type, qf_inums[type])) {
+		ext4_error(sb, "Bad quota inum: %lu, type: %d",
+				qf_inums[type], type);
+		return -EUCLEAN;
+	}
+
 	qf_inode = ext4_iget(sb, qf_inums[type], EXT4_IGET_SPECIAL);
 	if (IS_ERR(qf_inode)) {
-		ext4_error(sb, "Bad quota inode # %lu", qf_inums[type]);
+		ext4_error(sb, "Bad quota inode: %lu, type: %d",
+				qf_inums[type], type);
 		return PTR_ERR(qf_inode);
 	}
 
@@ -6942,8 +6963,9 @@ int ext4_enable_quotas(struct super_block *sb)
 			if (err) {
 				ext4_warning(sb,
 					"Failed to enable quota tracking "
-					"(type=%d, err=%d). Please run "
-					"e2fsck to fix.", type, err);
+					"(type=%d, err=%d, ino=%lu). "
+					"Please run e2fsck to fix.", type,
+					err, qf_inums[type]);
 				for (type--; type >= 0; type--) {
 					struct inode *inode;
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode
  2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
  2022-10-26  4:23 ` [PATCH v3 2/4] ext4: add helper to check quota inums Baokun Li
@ 2022-10-26  4:23 ` Baokun Li
  2022-10-26  9:41   ` Jan Kara
  2022-10-26 13:59   ` Jason Yan
  2022-10-26  4:23 ` [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode Baokun Li
  2022-12-06 21:01 ` [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Theodore Ts'o
  4 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-10-26  4:23 UTC (permalink / raw)
  To: linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, libaokun1

There are many places that will get unhappy (and crash) when ext4_iget()
returns a bad inode. However, if iget the boot loader inode, allows a bad
inode to be returned, because the inode may not be initialized. This
mechanism can be used to bypass some checks and cause panic. To solve this
problem, we add a special iget flag EXT4_IGET_BAD. Only with this flag
we'd be returning bad inode from ext4_iget(), otherwise we always return
the error code if the inode is bad inode.(suggested by Jan Kara)

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/ext4.h  | 3 ++-
 fs/ext4/inode.c | 8 +++++++-
 fs/ext4/ioctl.c | 3 ++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d5453852f98..2b574b143bde 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2964,7 +2964,8 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
 typedef enum {
 	EXT4_IGET_NORMAL =	0,
 	EXT4_IGET_SPECIAL =	0x0001, /* OK to iget a system inode */
-	EXT4_IGET_HANDLE = 	0x0002	/* Inode # is from a handle */
+	EXT4_IGET_HANDLE = 	0x0002,	/* Inode # is from a handle */
+	EXT4_IGET_BAD =		0x0004  /* Allow to iget a bad inode */
 } ext4_iget_flags;
 
 extern struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ae3a836dd9d7..f767340229fd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5043,8 +5043,14 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb))
 		ext4_error_inode(inode, function, line, 0,
 				 "casefold flag without casefold feature");
-	brelse(iloc.bh);
+	if (is_bad_inode(inode) && !(flags & EXT4_IGET_BAD)) {
+		ext4_error_inode(inode, function, line, 0,
+				 "bad inode without EXT4_IGET_BAD flag");
+		ret = -EUCLEAN;
+		goto bad_inode;
+	}
 
+	brelse(iloc.bh);
 	unlock_new_inode(inode);
 	return inode;
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index ded535535b27..e0be8026c996 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -375,7 +375,8 @@ static long swap_inode_boot_loader(struct super_block *sb,
 	blkcnt_t blocks;
 	unsigned short bytes;
 
-	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO, EXT4_IGET_SPECIAL);
+	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO,
+			EXT4_IGET_SPECIAL | EXT4_IGET_BAD);
 	if (IS_ERR(inode_bl))
 		return PTR_ERR(inode_bl);
 	ei_bl = EXT4_I(inode_bl);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode
  2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
                   ` (2 preceding siblings ...)
  2022-10-26  4:23 ` [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode Baokun Li
@ 2022-10-26  4:23 ` Baokun Li
  2022-10-26 10:12   ` Jan Kara
  2022-10-26 14:05   ` Jason Yan
  2022-12-06 21:01 ` [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Theodore Ts'o
  4 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-10-26  4:23 UTC (permalink / raw)
  To: linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, libaokun1

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 e0be8026c996..8c7a4ae85e99 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -426,7 +426,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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
@ 2022-10-26  9:36   ` Jan Kara
  2022-10-26 13:56   ` Jason Yan
  2022-10-26 18:33   ` Chaitanya Kulkarni
  2 siblings, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-10-26  9:36 UTC (permalink / raw)
  To: Baokun Li
  Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
	linux-kernel, yi.zhang, yukuai3, linux-fsdevel

On Wed 26-10-22 12:23:07, 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 the inode is
> bad_inode in vfs_setup_quota_inode().
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Thanks! The patch looks good. Feel free to add:

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

								Honza

> ---
>  fs/quota/dquot.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 0427b44bfee5..f27faf5db554 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2324,6 +2324,8 @@ static int vfs_setup_quota_inode(struct inode *inode, int type)
>  	struct super_block *sb = inode->i_sb;
>  	struct quota_info *dqopt = sb_dqopt(sb);
>  
> +	if (is_bad_inode(inode))
> +		return -EUCLEAN;
>  	if (!S_ISREG(inode->i_mode))
>  		return -EACCES;
>  	if (IS_RDONLY(inode))
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/4] ext4: add helper to check quota inums
  2022-10-26  4:23 ` [PATCH v3 2/4] ext4: add helper to check quota inums Baokun Li
@ 2022-10-26  9:38   ` Jan Kara
  2022-10-26 13:57   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-10-26  9:38 UTC (permalink / raw)
  To: Baokun Li
  Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
	linux-kernel, yi.zhang, yukuai3

On Wed 26-10-22 12:23:08, Baokun Li wrote:
> Before quota is enabled, a check on the preset quota inums in
> ext4_super_block is added to prevent wrong quota inodes from being loaded.
> In addition, when the quota fails to be enabled, the quota type and quota
> inum are printed to facilitate fault locating.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Makes sense. Feel free to add:

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

								Honza

> ---
>  fs/ext4/super.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 34b78f380968..0b4060d52d63 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6885,6 +6885,20 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>  	return err;
>  }
>  
> +static inline bool ext4_check_quota_inum(int type, unsigned long qf_inum)
> +{
> +	switch (type) {
> +	case USRQUOTA:
> +		return qf_inum == EXT4_USR_QUOTA_INO;
> +	case GRPQUOTA:
> +		return qf_inum == EXT4_GRP_QUOTA_INO;
> +	case PRJQUOTA:
> +		return qf_inum >= EXT4_GOOD_OLD_FIRST_INO;
> +	default:
> +		BUG();
> +	}
> +}
> +
>  static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
>  			     unsigned int flags)
>  {
> @@ -6901,9 +6915,16 @@ static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
>  	if (!qf_inums[type])
>  		return -EPERM;
>  
> +	if (!ext4_check_quota_inum(type, qf_inums[type])) {
> +		ext4_error(sb, "Bad quota inum: %lu, type: %d",
> +				qf_inums[type], type);
> +		return -EUCLEAN;
> +	}
> +
>  	qf_inode = ext4_iget(sb, qf_inums[type], EXT4_IGET_SPECIAL);
>  	if (IS_ERR(qf_inode)) {
> -		ext4_error(sb, "Bad quota inode # %lu", qf_inums[type]);
> +		ext4_error(sb, "Bad quota inode: %lu, type: %d",
> +				qf_inums[type], type);
>  		return PTR_ERR(qf_inode);
>  	}
>  
> @@ -6942,8 +6963,9 @@ int ext4_enable_quotas(struct super_block *sb)
>  			if (err) {
>  				ext4_warning(sb,
>  					"Failed to enable quota tracking "
> -					"(type=%d, err=%d). Please run "
> -					"e2fsck to fix.", type, err);
> +					"(type=%d, err=%d, ino=%lu). "
> +					"Please run e2fsck to fix.", type,
> +					err, qf_inums[type]);
>  				for (type--; type >= 0; type--) {
>  					struct inode *inode;
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode
  2022-10-26  4:23 ` [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode Baokun Li
@ 2022-10-26  9:41   ` Jan Kara
  2022-10-26 13:59   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-10-26  9:41 UTC (permalink / raw)
  To: Baokun Li
  Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
	linux-kernel, yi.zhang, yukuai3

On Wed 26-10-22 12:23:09, Baokun Li wrote:
> There are many places that will get unhappy (and crash) when ext4_iget()
> returns a bad inode. However, if iget the boot loader inode, allows a bad
> inode to be returned, because the inode may not be initialized. This
> mechanism can be used to bypass some checks and cause panic. To solve this
> problem, we add a special iget flag EXT4_IGET_BAD. Only with this flag
> we'd be returning bad inode from ext4_iget(), otherwise we always return
> the error code if the inode is bad inode.(suggested by Jan Kara)
> 
> 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/ext4.h  | 3 ++-
>  fs/ext4/inode.c | 8 +++++++-
>  fs/ext4/ioctl.c | 3 ++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8d5453852f98..2b574b143bde 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2964,7 +2964,8 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
>  typedef enum {
>  	EXT4_IGET_NORMAL =	0,
>  	EXT4_IGET_SPECIAL =	0x0001, /* OK to iget a system inode */
> -	EXT4_IGET_HANDLE = 	0x0002	/* Inode # is from a handle */
> +	EXT4_IGET_HANDLE = 	0x0002,	/* Inode # is from a handle */
> +	EXT4_IGET_BAD =		0x0004  /* Allow to iget a bad inode */
>  } ext4_iget_flags;
>  
>  extern struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ae3a836dd9d7..f767340229fd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5043,8 +5043,14 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb))
>  		ext4_error_inode(inode, function, line, 0,
>  				 "casefold flag without casefold feature");
> -	brelse(iloc.bh);
> +	if (is_bad_inode(inode) && !(flags & EXT4_IGET_BAD)) {
> +		ext4_error_inode(inode, function, line, 0,
> +				 "bad inode without EXT4_IGET_BAD flag");
> +		ret = -EUCLEAN;
> +		goto bad_inode;
> +	}
>  
> +	brelse(iloc.bh);
>  	unlock_new_inode(inode);
>  	return inode;
>  
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index ded535535b27..e0be8026c996 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -375,7 +375,8 @@ static long swap_inode_boot_loader(struct super_block *sb,
>  	blkcnt_t blocks;
>  	unsigned short bytes;
>  
> -	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO, EXT4_IGET_SPECIAL);
> +	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO,
> +			EXT4_IGET_SPECIAL | EXT4_IGET_BAD);
>  	if (IS_ERR(inode_bl))
>  		return PTR_ERR(inode_bl);
>  	ei_bl = EXT4_I(inode_bl);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode
  2022-10-26  4:23 ` [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode Baokun Li
@ 2022-10-26 10:12   ` Jan Kara
  2022-10-26 14:05   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-10-26 10:12 UTC (permalink / raw)
  To: Baokun Li
  Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
	linux-kernel, yi.zhang, yukuai3

On Wed 26-10-22 12:23:10, 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>

Looks good to me. Feel free to add:

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

								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 e0be8026c996..8c7a4ae85e99 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -426,7 +426,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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
  2022-10-26  9:36   ` Jan Kara
@ 2022-10-26 13:56   ` Jason Yan
  2022-10-26 18:33   ` Chaitanya Kulkarni
  2 siblings, 0 replies; 15+ messages in thread
From: Jason Yan @ 2022-10-26 13:56 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, linux-fsdevel


On 2022/10/26 12:23, 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 the inode is
> bad_inode in vfs_setup_quota_inode().
> 
> Signed-off-by: Baokun Li<libaokun1@huawei.com>
> ---
>   fs/quota/dquot.c | 2 ++
>   1 file changed, 2 insertions(+)

Looks good,
Reviewed-by: Jason Yan <yanaijie@huawei.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/4] ext4: add helper to check quota inums
  2022-10-26  4:23 ` [PATCH v3 2/4] ext4: add helper to check quota inums Baokun Li
  2022-10-26  9:38   ` Jan Kara
@ 2022-10-26 13:57   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jason Yan @ 2022-10-26 13:57 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3


On 2022/10/26 12:23, Baokun Li wrote:
> Before quota is enabled, a check on the preset quota inums in
> ext4_super_block is added to prevent wrong quota inodes from being loaded.
> In addition, when the quota fails to be enabled, the quota type and quota
> inum are printed to facilitate fault locating.
> 
> Signed-off-by: Baokun Li<libaokun1@huawei.com>
> ---
>   fs/ext4/super.c | 28 +++++++++++++++++++++++++---
>   1 file changed, 25 insertions(+), 3 deletions(-)

Looks good,
Reviewed-by: Jason Yan <yanaijie@huawei.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode
  2022-10-26  4:23 ` [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode Baokun Li
  2022-10-26  9:41   ` Jan Kara
@ 2022-10-26 13:59   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jason Yan @ 2022-10-26 13:59 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3


On 2022/10/26 12:23, Baokun Li wrote:
> There are many places that will get unhappy (and crash) when ext4_iget()
> returns a bad inode. However, if iget the boot loader inode, allows a bad
> inode to be returned, because the inode may not be initialized. This
> mechanism can be used to bypass some checks and cause panic. To solve this
> problem, we add a special iget flag EXT4_IGET_BAD. Only with this flag
> we'd be returning bad inode from ext4_iget(), otherwise we always return
> the error code if the inode is bad inode.(suggested by Jan Kara)
> 
> Signed-off-by: Baokun Li<libaokun1@huawei.com>
> ---
>   fs/ext4/ext4.h  | 3 ++-
>   fs/ext4/inode.c | 8 +++++++-
>   fs/ext4/ioctl.c | 3 ++-
>   3 files changed, 11 insertions(+), 3 deletions(-)

Looks good,
Reviewed-by: Jason Yan <yanaijie@huawei.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode
  2022-10-26  4:23 ` [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode Baokun Li
  2022-10-26 10:12   ` Jan Kara
@ 2022-10-26 14:05   ` Jason Yan
  1 sibling, 0 replies; 15+ messages in thread
From: Jason Yan @ 2022-10-26 14:05 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3


On 2022/10/26 12:23, 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>
> ---
>   fs/ext4/ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Looks good,
Reviewed-by: Jason Yan <yanaijie@huawei.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode
  2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
  2022-10-26  9:36   ` Jan Kara
  2022-10-26 13:56   ` Jason Yan
@ 2022-10-26 18:33   ` Chaitanya Kulkarni
  2 siblings, 0 replies; 15+ messages in thread
From: Chaitanya Kulkarni @ 2022-10-26 18:33 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
	yukuai3, linux-fsdevel

On 10/25/2022 9:23 PM, 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 the inode is
> bad_inode in vfs_setup_quota_inode().
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>


The analysis and the provided call chain looks really good, only thing 
non-blocker is missing that maintainer might want it version history.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search
  2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
                   ` (3 preceding siblings ...)
  2022-10-26  4:23 ` [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode Baokun Li
@ 2022-12-06 21:01 ` Theodore Ts'o
  4 siblings, 0 replies; 15+ messages in thread
From: Theodore Ts'o @ 2022-12-06 21:01 UTC (permalink / raw)
  To: Baokun Li, linux-ext4
  Cc: Theodore Ts'o, linux-kernel, yi.zhang, ritesh.list,
	adilger.kernel, yukuai3, jack

On Wed, 26 Oct 2022 12:23:06 +0800, Baokun Li wrote:
> 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)
> V2->V3:
> 	a. add EXT4_IGET_BAD flag to prevent unexpected bad inode.
> 	b. check bad quota inode in vfs_setup_quota_inode() instead of in
> 	   ext4_quota_enable() for more generic approach to this problem.
> 	c. add helper to check quota inums.
> 
> [...]

Applied, thanks!

[1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode
      commit: c7e9666f28ba9bdeeb99fb0c60a27dbb88f452f4
[2/4] ext4: add helper to check quota inums
      commit: 9c4883f1b41181f2096e2ee4e98111008b77165c
[3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode
      commit: 10f525fda2faff81f6cfce2a6bc4b50a5254d9ea
[4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode
      commit: db14233edaf579153d8c92bf3a0ba27ceb87eabc

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-12-06 21:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26  4:23 [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Baokun Li
2022-10-26  4:23 ` [PATCH v3 1/4] ext4: fix bug_on in __es_tree_search caused by bad quota inode Baokun Li
2022-10-26  9:36   ` Jan Kara
2022-10-26 13:56   ` Jason Yan
2022-10-26 18:33   ` Chaitanya Kulkarni
2022-10-26  4:23 ` [PATCH v3 2/4] ext4: add helper to check quota inums Baokun Li
2022-10-26  9:38   ` Jan Kara
2022-10-26 13:57   ` Jason Yan
2022-10-26  4:23 ` [PATCH v3 3/4] ext4: add EXT4_IGET_BAD flag to prevent unexpected bad inode Baokun Li
2022-10-26  9:41   ` Jan Kara
2022-10-26 13:59   ` Jason Yan
2022-10-26  4:23 ` [PATCH v3 4/4] ext4: fix bug_on in __es_tree_search caused by bad boot loader inode Baokun Li
2022-10-26 10:12   ` Jan Kara
2022-10-26 14:05   ` Jason Yan
2022-12-06 21:01 ` [PATCH v3 0/4] ext4: fix two bug_on in __es_tree_search Theodore Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).