[PATCH v3 0/2] ext4: fix quotas leak in __ext4_fill_super()

[PATCH v3 0/2] ext4: fix quotas leak in __ext4_fill_super()

[Baokun Li]

V1->V2:
	Add judgment for CONFIG_QUOTA to avoid warning
	"label 'failed_mount9' defined but not used".
	(Reported-by: kernel test robot <lkp@intel.com>)
V2->V3:
	By adding __maybe_unused to silence possible compilation
	complain in patch one, instead of adding the judgment.
	(Suggested by Jan Kara).

Baokun Li (2):
  ext4: turning quotas off if mount failed after enable quotas
  ext4: refactoring to use the unified helper ext4_quotas_off()

 fs/ext4/super.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH v3 1/2] ext4: turning quotas off if mount failed after enable quotas

[Baokun Li]

Yi found during a review of the patch "ext4: don't BUG on inconsistent
journal feature" that when ext4_mark_recovery_complete() returns an error
value, the error handling path does not turn off the enabled quotas,
which triggers the following kmemleak:

================================================================
unreferenced object 0xffff8cf68678e7c0 (size 64):
comm "mount", pid 746, jiffies 4294871231 (age 11.540s)
hex dump (first 32 bytes):
00 90 ef 82 f6 8c ff ff 00 00 00 00 41 01 00 00  ............A...
c7 00 00 00 bd 00 00 00 0a 00 00 00 48 00 00 00  ............H...
backtrace:
[<00000000c561ef24>] __kmem_cache_alloc_node+0x4d4/0x880
[<00000000d4e621d7>] kmalloc_trace+0x39/0x140
[<00000000837eee74>] v2_read_file_info+0x18a/0x3a0
[<0000000088f6c877>] dquot_load_quota_sb+0x2ed/0x770
[<00000000340a4782>] dquot_load_quota_inode+0xc6/0x1c0
[<0000000089a18bd5>] ext4_enable_quotas+0x17e/0x3a0 [ext4]
[<000000003a0268fa>] __ext4_fill_super+0x3448/0x3910 [ext4]
[<00000000b0f2a8a8>] ext4_fill_super+0x13d/0x340 [ext4]
[<000000004a9489c4>] get_tree_bdev+0x1dc/0x370
[<000000006e723bf1>] ext4_get_tree+0x1d/0x30 [ext4]
[<00000000c7cb663d>] vfs_get_tree+0x31/0x160
[<00000000320e1bed>] do_new_mount+0x1d5/0x480
[<00000000c074654c>] path_mount+0x22e/0xbe0
[<0000000003e97a8e>] do_mount+0x95/0xc0
[<000000002f3d3736>] __x64_sys_mount+0xc4/0x160
[<0000000027d2140c>] do_syscall_64+0x3f/0x90
================================================================

To solve this problem, we add a "failed_mount10" tag, and call
ext4_quota_off_umount() in this tag to release the enabled qoutas.

Fixes: 11215630aada ("ext4: don't BUG on inconsistent journal feature")
Cc: stable@kernel.org
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V1->V2:
        Add judgment for CONFIG_QUOTA to avoid warning
        "label 'failed_mount9' defined but not used".
        (Reported-by: kernel test robot <lkp@intel.com>)
V2->V3:
        By adding __maybe_unused to silence possible compilation
        complain in patch one, instead of adding the judgment.
        (Suggested by Jan Kara).

 fs/ext4/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e6d84c1e34a4..97addf5fd642 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5520,7 +5520,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		ext4_msg(sb, KERN_INFO, "recovery complete");
 		err = ext4_mark_recovery_complete(sb, es);
 		if (err)
-			goto failed_mount9;
+			goto failed_mount10;
 	}
 
 	if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
@@ -5539,7 +5539,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	return 0;
 
-failed_mount9:
+failed_mount10:
+	ext4_quota_off_umount(sb);
+failed_mount9: __maybe_unused
 	ext4_release_orphan_info(sb);
 failed_mount8:
 	ext4_unregister_sysfs(sb);
-- 
2.31.1

Re: [PATCH v3 1/2] ext4: turning quotas off if mount failed after enable quotas

[Jan Kara]

On Mon 27-03-23 22:16:29, Baokun Li wrote:
> Yi found during a review of the patch "ext4: don't BUG on inconsistent
> journal feature" that when ext4_mark_recovery_complete() returns an error
> value, the error handling path does not turn off the enabled quotas,
> which triggers the following kmemleak:
> 
> ================================================================
> unreferenced object 0xffff8cf68678e7c0 (size 64):
> comm "mount", pid 746, jiffies 4294871231 (age 11.540s)
> hex dump (first 32 bytes):
> 00 90 ef 82 f6 8c ff ff 00 00 00 00 41 01 00 00  ............A...
> c7 00 00 00 bd 00 00 00 0a 00 00 00 48 00 00 00  ............H...
> backtrace:
> [<00000000c561ef24>] __kmem_cache_alloc_node+0x4d4/0x880
> [<00000000d4e621d7>] kmalloc_trace+0x39/0x140
> [<00000000837eee74>] v2_read_file_info+0x18a/0x3a0
> [<0000000088f6c877>] dquot_load_quota_sb+0x2ed/0x770
> [<00000000340a4782>] dquot_load_quota_inode+0xc6/0x1c0
> [<0000000089a18bd5>] ext4_enable_quotas+0x17e/0x3a0 [ext4]
> [<000000003a0268fa>] __ext4_fill_super+0x3448/0x3910 [ext4]
> [<00000000b0f2a8a8>] ext4_fill_super+0x13d/0x340 [ext4]
> [<000000004a9489c4>] get_tree_bdev+0x1dc/0x370
> [<000000006e723bf1>] ext4_get_tree+0x1d/0x30 [ext4]
> [<00000000c7cb663d>] vfs_get_tree+0x31/0x160
> [<00000000320e1bed>] do_new_mount+0x1d5/0x480
> [<00000000c074654c>] path_mount+0x22e/0xbe0
> [<0000000003e97a8e>] do_mount+0x95/0xc0
> [<000000002f3d3736>] __x64_sys_mount+0xc4/0x160
> [<0000000027d2140c>] do_syscall_64+0x3f/0x90
> ================================================================
> 
> To solve this problem, we add a "failed_mount10" tag, and call
> ext4_quota_off_umount() in this tag to release the enabled qoutas.
> 
> Fixes: 11215630aada ("ext4: don't BUG on inconsistent journal feature")
> Cc: stable@kernel.org
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
> V1->V2:
>         Add judgment for CONFIG_QUOTA to avoid warning
>         "label 'failed_mount9' defined but not used".
>         (Reported-by: kernel test robot <lkp@intel.com>)
> V2->V3:
>         By adding __maybe_unused to silence possible compilation
>         complain in patch one, instead of adding the judgment.
>         (Suggested by Jan Kara).
> 
>  fs/ext4/super.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index e6d84c1e34a4..97addf5fd642 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5520,7 +5520,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		ext4_msg(sb, KERN_INFO, "recovery complete");
>  		err = ext4_mark_recovery_complete(sb, es);
>  		if (err)
> -			goto failed_mount9;
> +			goto failed_mount10;
>  	}
>  
>  	if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
> @@ -5539,7 +5539,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	return 0;
>  
> -failed_mount9:
> +failed_mount10:
> +	ext4_quota_off_umount(sb);
> +failed_mount9: __maybe_unused
>  	ext4_release_orphan_info(sb);
>  failed_mount8:
>  	ext4_unregister_sysfs(sb);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 2/2] ext4: refactoring to use the unified helper ext4_quotas_off()

[Baokun Li]

Rename ext4_quota_off_umount() to ext4_quotas_off(), and add type
parameter to replace open code in ext4_enable_quotas().

Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
V1->V2:
	Adapting to the changes in PATCH 1/2.
V2->V3:
	Adapting to the changes in PATCH 1/2.

 fs/ext4/super.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 97addf5fd642..75bc263c2d92 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1157,12 +1157,12 @@ static void dump_orphan_list(struct super_block *sb, struct ext4_sb_info *sbi)
 #ifdef CONFIG_QUOTA
 static int ext4_quota_off(struct super_block *sb, int type);
 
-static inline void ext4_quota_off_umount(struct super_block *sb)
+static inline void ext4_quotas_off(struct super_block *sb, int type)
 {
-	int type;
+	BUG_ON(type > EXT4_MAXQUOTAS);
 
 	/* Use our quota_off function to clear inode flags etc. */
-	for (type = 0; type < EXT4_MAXQUOTAS; type++)
+	for (type--; type >= 0; type--)
 		ext4_quota_off(sb, type);
 }
 
@@ -1178,7 +1178,7 @@ static inline char *get_qf_name(struct super_block *sb,
 					 lockdep_is_held(&sb->s_umount));
 }
 #else
-static inline void ext4_quota_off_umount(struct super_block *sb)
+static inline void ext4_quotas_off(struct super_block *sb, int type)
 {
 }
 #endif
@@ -1209,7 +1209,7 @@ static void ext4_put_super(struct super_block *sb)
 			 &sb->s_uuid);
 
 	ext4_unregister_li_request(sb);
-	ext4_quota_off_umount(sb);
+	ext4_quotas_off(sb, EXT4_MAXQUOTAS);
 
 	flush_work(&sbi->s_error_work);
 	destroy_workqueue(sbi->rsv_conversion_wq);
@@ -5540,7 +5540,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	return 0;
 
 failed_mount10:
-	ext4_quota_off_umount(sb);
+	ext4_quotas_off(sb, EXT4_MAXQUOTAS);
 failed_mount9: __maybe_unused
 	ext4_release_orphan_info(sb);
 failed_mount8:
@@ -7012,20 +7012,8 @@ int ext4_enable_quotas(struct super_block *sb)
 					"(type=%d, err=%d, ino=%lu). "
 					"Please run e2fsck to fix.", type,
 					err, qf_inums[type]);
-				for (type--; type >= 0; type--) {
-					struct inode *inode;
-
-					inode = sb_dqopt(sb)->files[type];
-					if (inode)
-						inode = igrab(inode);
-					dquot_quota_off(sb, type);
-					if (inode) {
-						lockdep_set_quota_inode(inode,
-							I_DATA_SEM_NORMAL);
-						iput(inode);
-					}
-				}
 
+				ext4_quotas_off(sb, type);
 				return err;
 			}
 		}
-- 
2.31.1

Re: [PATCH v3 0/2] ext4: fix quotas leak in __ext4_fill_super()

[Baokun Li]

A gentle ping.

On 2023/3/27 22:16, Baokun Li wrote:
> V1->V2:
> 	Add judgment for CONFIG_QUOTA to avoid warning
> 	"label 'failed_mount9' defined but not used".
> 	(Reported-by: kernel test robot <lkp@intel.com>)
> V2->V3:
> 	By adding __maybe_unused to silence possible compilation
> 	complain in patch one, instead of adding the judgment.
> 	(Suggested by Jan Kara).
>
> Baokun Li (2):
>    ext4: turning quotas off if mount failed after enable quotas
>    ext4: refactoring to use the unified helper ext4_quotas_off()
>
>   fs/ext4/super.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
>

Re: [PATCH v3 0/2] ext4: fix quotas leak in __ext4_fill_super()

[Theodore Ts'o]

On Mon, 27 Mar 2023 22:16:28 +0800, Baokun Li wrote:
> V1->V2:
> 	Add judgment for CONFIG_QUOTA to avoid warning
> 	"label 'failed_mount9' defined but not used".
> 	(Reported-by: kernel test robot <lkp@intel.com>)
> V2->V3:
> 	By adding __maybe_unused to silence possible compilation
> 	complain in patch one, instead of adding the judgment.
> 	(Suggested by Jan Kara).
> 
> [...]

Applied, thanks!

[1/2] ext4: turning quotas off if mount failed after enable quotas
      commit: e39a0c8d9cc298c5a4afdeb416441091ae3f1fcb
[2/2] ext4: refactoring to use the unified helper ext4_quotas_off()
      commit: 63ec000b35a3c6cbb42b2e545c7c18f3d36159d6

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