linux-ext4 - Re: [PATCH] ext4/mballoc: No need to generate from free list

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]

Message-Id: <877cocgqei.fsf@doe.com>
Date:   Wed, 27 Sep 2023 10:32:13 +0530
From:   Ritesh Harjani (IBM) <ritesh.list@...il.com>
To:     Jan Kara <jack@...e.cz>, Wang Jianjian <wangjianjian0@...mail.com>
Cc:     linux-ext4@...r.kernel.org, aneesh.kumar@...ux.vnet.ibm.com
Subject: Re: [PATCH] ext4/mballoc: No need to generate from free list

Jan Kara <jack@...e.cz> writes:

> On Thu 24-08-23 23:56:31, Wang Jianjian wrote:
>> Commit 7a2fcbf7f85('ext4: don't use blocks freed but
>> not yet committed in buddy cache init) walk the rbtree of
>> freed data and mark them free in buddy to avoid reuse them
>> before journal committing them, However, it is unnecessary to
>> do that, because we have extra page references to buddy and bitmap
>> pages, they will be released iff journal has committed and after
>> process freed data.
>
> So do you mean that buddy bitmap cannot be freed until the transaction that
> frees the blocks in it commits? Indeed ext4_mb_free_metadata() grabs buddy
> page references and ext4_free_data_in_buddy() drops them. 
>
> Perhaps I'd rephrase the changelog as:
>
> Commit 7a2fcbf7f85 ("ext4: don't use blocks freed but not yet committed in
> buddy cache init") added a code to mark as used blocks in the list of not yet
> committed freed blocks during initialization of a buddy page. However
> ext4_mb_free_metadata() makes sure buddy page is already loaded and takes a
> reference to it so it cannot happen that ext4_mb_init_cache() is called
> when efd list is non-empty. Just remove the
> ext4_mb_generate_from_freelist() call.
>

The above changelog is very clear. Thanks!

The observation made in this patch is very subtle. Indeed we cannot have
ext4_mb_init_cache() get called for a group as long we have an entry in
grp->bb_free_root because it holds the reference to the buddy and
incore-bitmap pages. These entry gets freed up after a transaction
commit is completed via callback ext4_process_freed_data() -> ext4_free_data_in_buddy()

Nice catch. It's from 2009 :)

>> @@ -1274,7 +1272,6 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp)
>>  
>>  			/* mark all preallocated blks used in in-core bitmap */
>>  			ext4_mb_generate_from_pa(sb, data, group);
>> -			ext4_mb_generate_from_freelist(sb, data, group);
>
> And just to be sure I'd add here:
>
> 			WARN_ON_ONCE(!RB_EMPTY_ROOT(&grp->bb_free_root));
>

Right. That might catch any wrong use.


-ritesh