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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Mar 2023 11:05:06 +0200
From:   Jan Kara <jack@...e.cz>
To:     Baokun Li <libaokun1@...wei.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yangerkun@...wei.com, yukuai3@...wei.com, stable@...nel.org
Subject: Re: [PATCH v2 1/2] ext4: turning quotas off if mount failed after
 enable quotas

On Mon 27-03-23 10:27:02, 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@...nel.org
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

Looks good. Just one comment:

> +failed_mount10:
> +#ifdef CONFIG_QUOTA
> +	ext4_quota_off_umount(sb);
>  failed_mount9:
> +#endif  /* CONFIG_QUOTA */

How about dealing with this using __maybe_unused attribute instead. Like:

failed_mount9: __maybe_unused

That would be much easier to read...

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