linux-ext4 - Re: [PATCH] jbd2: make sure jh have b_transaction set in refile/unlink

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] [day] [month] [year] [list]

Message-ID: <20200617121500.GA29763@quack2.suse.cz>
Date:   Wed, 17 Jun 2020 14:15:00 +0200
From:   Jan Kara <jack@...e.cz>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     linux-ext4@...r.kernel.org, Jan Kara <jack@...e.com>
Subject: Re: [PATCH] jbd2: make sure jh have b_transaction set in
 refile/unlink_buffer

On Wed 17-06-20 11:10:31, Lukas Czerner wrote:
> Callers of __jbd2_journal_unfile_buffer() and
> __jbd2_journal_refile_buffer() assume that the b_transaction is set. In
> fact if it's not, we can end up with journal_head refcounting errors
> leading to crash much later that might be very hard to track down. Add
> asserts to make sure that is the case.
> 
> We also make sure that b_next_transaction is NULL in
> __jbd2_journal_unfile_buffer() since the callers expect that as well and
> we should not get into that stage in this state anyway, leading to
> problems later on if we do.
> 
> Tested with fstests.
> 
> Signed-off-by: Lukas Czerner <lczerner@...hat.com>

Thanks! The patch looks good to me. You can add:

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

								Honza

> ---
>  fs/jbd2/transaction.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index e91aad3637a2..e65e0aca2826 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -2026,6 +2026,9 @@ static void __jbd2_journal_temp_unlink_buffer(struct journal_head *jh)
>   */
>  static void __jbd2_journal_unfile_buffer(struct journal_head *jh)
>  {
> +	J_ASSERT_JH(jh, jh->b_transaction != NULL);
> +	J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
> +
>  	__jbd2_journal_temp_unlink_buffer(jh);
>  	jh->b_transaction = NULL;
>  }
> @@ -2572,6 +2575,13 @@ bool __jbd2_journal_refile_buffer(struct journal_head *jh)
>  
>  	was_dirty = test_clear_buffer_jbddirty(bh);
>  	__jbd2_journal_temp_unlink_buffer(jh);
> +
> +	/*
> +	 * b_transaction must be set, otherwise the new b_transaction won't
> +	 * be holding jh reference
> +	 */
> +	J_ASSERT_JH(jh, jh->b_transaction != NULL);
> +
>  	/*
>  	 * We set b_transaction here because b_next_transaction will inherit
>  	 * our jh reference and thus __jbd2_journal_file_buffer() must not
> -- 
> 2.21.3
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR