linux-ext4 - Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

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: <e5a3187c-b333-ec5b-0d8b-a02934a37b06@huaweicloud.com>
Date: Wed, 26 Jun 2024 21:24:13 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, Alexander Coffin
 <alex.coffin@...icrobots.com>, stable@...r.kernel.org,
 Ted Tso <tytso@....edu>, chengzhihao1@...wei.com
Subject: Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

On 2024/6/26 19:22, Jan Kara wrote:
> On Wed 26-06-24 15:38:42, Zhang Yi wrote:
>> On 2024/6/25 1:01, Jan Kara wrote:
>>> Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
>>> t_outstanding_credits") started to account descriptor blocks into
>>> transactions outstanding credits. However it didn't appropriately
>>> decrease the maximum amount of credits available to userspace. Thus if
>>> the filesystem requests a transaction smaller than
>>> j_max_transaction_buffers but large enough that when descriptor blocks
>>> are added the size exceeds j_max_transaction_buffers, we confuse
>>> add_transaction_credits() into thinking previous handles have grown the
>>> transaction too much and enter infinite journal commit loop in
>>> start_this_handle() -> add_transaction_credits() trying to create
>>> transaction with enough credits available.
>>
>> I understand that the incorrect max transaction limit in
>> start_this_handle() could lead to infinite loop in
>> start_this_handle()-> add_transaction_credits() with large enough
>> userspace credits (from j_max_transaction_buffers - overheads to
>> j_max_transaction_buffers), but I don't get how could it lead to ran
>> out of space in the journal commit traction? IIUC, below codes in
>> add_transaction_credits() could make sure that we have enough space
>> when committing traction:
>>
>> static int add_transaction_credits()
>> {
>> ...
>> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
>> 		...
>> 		return 1;
>> 		...
>> 	}
>> ...
>> }
>>
>> I can't open and download the image Alexander gave, so I can't get to
>> the bottom of this issue, please let me know what happened with
>> jbd2_journal_next_log_block().
> 
> Sure. So what was exactly happening is a loop like this:
> 
> start_this_handle()
>   blocks = 252 (handle->h_total_credits)
>   - starts a new transaction
>     - t_outstanding_credits set to 6 to account for the commit block and
>       descriptor blocks
>   add_transaction_credits(journal, 252, 0)
>      needed = atomic_add_return(252, &t->t_outstanding_credits);
>      if (needed > journal->j_max_transaction_buffers) {
> 	/* Yes, this is exceeded due to descriptor blocks being in
> 	 * t_outstanding_credits */
>         ...
>         wait_transaction_locked(journal);
> 	  - this commits an empty transaction - contains only the commit
> 	    block
>         return 1
>   goto repeat
> 
> So we commit single block transactions in a loop until we exhaust all the
> journal space. The condition in add_transaction_credits() whether there's
> enough space in the journal is never reached so we don't ever push the
> journal tail to make space in the journal.
>     

mm-hm, ha, yeah, this will lead to submit an empty transaction in each loop,
but I still have one question, although the journal tail can't be updated
in add_transaction_credits(), I think the journal tail should be updated in
jbd2_journal_commit_transaction()->jbd2_update_log_tail() since we don't
add empty transactions to journal->j_checkpoint_transactions list, the loop
in jbd2_journal_commit_transaction() should like this:

...
jbd2_journal_commit_transaction()
  update_tail = jbd2_journal_get_log_tail()
    //journal->j_checkpoint_transactions should be NULL, tid is the
    //committing transaction's tid, which should be large than the
    //tail tid since the second loop, so this should be true after
    //the second loop
  if (freed < journal->j_max_transaction_buffers)
    update_tail = 0;
    //update_tail should be true after j_max_transaction_buffers loop

jbd2_update_log_tail()  //j_free should be increased in each
                        //j_max_transaction_buffers loop
  if (tid_gt(tid, journal->j_tail_sequence)) //it's true
  __jbd2_update_log_tail()
    journal->j_free += freed; //update log tail and increase j_free
                              //j_max_transaction_buffers blocks
...

As I understand it, the journal space can't be exhausted, I don't know how
it happened, am I missing something?

Thanks,
Yi.