linux-ext4 - Re: [PATCH] ext4: put grp related checks into ext4_mb_good

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 13 Aug 2020 20:48:50 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     brookxu <brookxu.cn@...il.com>, adilger.kernel@...ger.ca,
        tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: put grp related checks into ext4_mb_good_group()



On 8/7/20 7:31 PM, brookxu wrote:
> We will make these judgments in ext4_mb_good_group(), maybe there
> is no need to repeat judgments here.
> 
> Signed-off-by: Chunguang Xu <brookxu@...cent.com>

Nack. This could essentially cause performance issues.
But then maybe we should add a comment saying these extra checks are
intentionally done here without explicit ext4_lock_group() for
performance optimizations.

-ritesh



> ---
>   fs/ext4/mballoc.c | 16 ++--------------
>   1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4304113..84871f7 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2178,21 +2178,8 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
>   	struct ext4_group_info *grp = ext4_get_group_info(ac->ac_sb, group);
>   	struct super_block *sb = ac->ac_sb;
>   	bool should_lock = ac->ac_flags & EXT4_MB_STRICT_CHECK;
> -	ext4_grpblk_t free;
>   	int ret = 0;
> 
> -	if (should_lock)
> -		ext4_lock_group(sb, group);
> -	free = grp->bb_free;
> -	if (free == 0)
> -		goto out;
> -	if (cr <= 2 && free < ac->ac_g_ex.fe_len)
> -		goto out;
> -	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
> -		goto out;
> -	if (should_lock)
> -		ext4_unlock_group(sb, group);
> -
>   	/* We only do this if the grp has never been initialized */
>   	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
>   		ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS);
> @@ -2202,8 +2189,9 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
> 
>   	if (should_lock)
>   		ext4_lock_group(sb, group);
> +
>   	ret = ext4_mb_good_group(ac, group, cr);
> -out:
> +
>   	if (should_lock)
>   		ext4_unlock_group(sb, group);
>   	return ret;
>