linux-ext4 - Re: [PATCH 2/5] jbd2: don't mark block as modified if the handle is out of credits

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: <F1BDFF96-5199-48C0-805E-EB1CA119B5FD@dilger.ca>
Date:   Mon, 18 Jun 2018 12:27:44 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        wen.xu@...ech.edu
Subject: Re: [PATCH 2/5] jbd2: don't mark block as modified if the handle is
 out of credits


> On Jun 17, 2018, at 9:22 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> Do not set the b_modified flag in block's journal head should not
> until after we're sure that jbd2_journal_dirty_metadat() will not
> abort with an error due to there not being enough space reserved in
> the jbd2 handle.

Looks like you partially edited this sentence after it was written,
but didn't get it back to coherency?

> Otherwise, future attempts to modify the buffer may lead a large
> number of spurious errors and warnings.

... lead *to* a large number ...

> 
> https://bugzilla.kernel.org/show_bug.cgi?id=200071
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> ---
> fs/jbd2/transaction.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 51dd68e67b0f..c0b66a7a795b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -1361,6 +1361,13 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
> 		if (jh->b_transaction == transaction &&
> 		    jh->b_jlist != BJ_Metadata) {
> 			jbd_lock_bh_state(bh);
> +			if (jh->b_transaction == transaction &&
> +			    jh->b_jlist != BJ_Metadata)
> +				pr_err("JBD2: assertion failure: h_type=%u "
> +				       "h_line_no=%u block_no=%llu jlist=%u\n",
> +				       handle->h_type, handle->h_line_no,
> +				       (unsigned long long) bh->b_blocknr,
> +				       jh->b_jlist);
> 			J_ASSERT_JH(jh, jh->b_transaction != transaction ||
> 					jh->b_jlist == BJ_Metadata);

Should the J_ASSERT_JH() be inside the second "if" block that is rechecked
under lock?  Otherwise, I don't see the point in getting the bh_state lock
since either jh->b_jlist can't change and there is no need to lock it, or
it *can* change and we shouldn't assert if the condition is no longer wrong
enough to print the error message.

> 			jbd_unlock_bh_state(bh);
> @@ -1380,11 +1387,11 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
> 		 * of the transaction. This needs to be done
> 		 * once a transaction -bzzz
> 		 */
> -		jh->b_modified = 1;
> 		if (handle->h_buffer_credits <= 0) {
> 			ret = -ENOSPC;
> 			goto out_unlock_bh;
> 		}
> +		jh->b_modified = 1;
> 		handle->h_buffer_credits--;
> 	}
> 
> --
> 2.18.0.rc0
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)