linux-ext4 - Re: [PATCH 2/2] ext4: fix a potential assertion failure due to improperly dirtied buffer

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: <20240828114513.bzccvcalexwge4d7@quack3>
Date: Wed, 28 Aug 2024 13:45:13 +0200
From: Jan Kara <jack@...e.cz>
To: zhangshida <starzhangzsd@...il.com>
Cc: tytso@....edu, adilger.kernel@...ger.ca, jack@...e.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	zhangshida@...inos.cn, Baolin Liu <liubaolin@...inos.cn>,
	Jan Kara <jack@...e.cz>
Subject: Re: [PATCH 2/2] ext4: fix a potential assertion failure due to
 improperly dirtied buffer

On Fri 23-08-24 09:33:29, zhangshida wrote:
> From: Shida Zhang <zhangshida@...inos.cn>
> 
> On an old kernel version(4.19, ext3, data=journal, pagesize=64k),
> an assertion failure will occasionally be triggered by the line below:
> -----------
> jbd2_journal_commit_transaction
> {
> ...
> J_ASSERT_BH(bh, !buffer_dirty(bh));
> /*
> * The buffer on BJ_Forget list and not jbddirty means
> ...
> }
> -----------
> 
> The same condition may also be applied to the lattest kernel version.
> 
> When blocksize < pagesize and we truncate a file, there can be buffers in
> the mapping tail page beyond i_size. These buffers will be filed to
> transaction's BJ_Forget list by ext4_journalled_invalidatepage() during
> truncation. When the transaction doing truncate starts committing, we can
> grow the file again. This calls __block_write_begin() which allocates new
> blocks under these buffers in the tail page we go through the branch:
					     ^^ and we...

 
>                         if (buffer_new(bh)) {
>                                 clean_bdev_bh_alias(bh);
>                                 if (folio_test_uptodate(folio)) {
>                                         clear_buffer_new(bh);
>                                         set_buffer_uptodate(bh);
>                                         mark_buffer_dirty(bh);
>                                         continue;
>                                 }
>                                 ...
>                         }
> 
> Hence buffers on BJ_Forget list of the committing transaction get marked
> dirty and this triggers the jbd2 assertion.
> 
> Teach ext4_block_write_begin() to properly handle files with data
> journalling by avoiding dirtying them directly. Instead of
> folio_zero_new_buffers() we use ext4_journalled_zero_new_buffers() which
> takes care of handling journalling. We also don't need to mark new uptodate
> buffers as dirty in ext4_block_write_begin(). That will be either done
> either by block_commit_write() in case of success or by
> folio_zero_new_buffers() in case of failure.
> 
> Reported-by: Baolin Liu <liubaolin@...inos.cn>
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Shida Zhang <zhangshida@...inos.cn>

Looks mostly good. Just three small comments:

> @@ -1083,11 +1090,11 @@ int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  			err = get_block(inode, block, bh, 1);
>  			if (err)
>  				break;


> +			if (should_journal_data)
> +				do_journal_get_write_access(handle, inode, bh);

I'd move this inside the buffer_new() branch and add before it a comment:
			/*
			 * We may be zeroing partial buffers or all new
			 * buffers in case of failure. Prepare JBD2 for
			 * that.
			 */

>  			if (buffer_new(bh)) {
>  				if (folio_test_uptodate(folio)) {
> -					clear_buffer_new(bh);
>  					set_buffer_uptodate(bh);
> -					mark_buffer_dirty(bh);

Here I'd add comment:
				/*
				 * Unlike __block_write_begin() we leave
				 * dirtying of new uptodate buffers to
				 * ->write_end() time or
				 * folio_zero_new_buffers().
				 */

> @@ -1117,7 +1124,11 @@ int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  			err = -EIO;
>  	}
>  	if (unlikely(err)) {
> -		folio_zero_new_buffers(folio, from, to);
> +		if (should_journal_data)
> +			ext4_journalled_zero_new_buffers(handle, inode, folio,
> +							 from, to);

I've realized there's a small bug in ext4_journalled_zero_new_buffers()
that it calls write_end_fn() only if it zeroed a buffer. But for new
uptodate buffers we also need to call write_end_fn() to persist the
uptodate content (similarly as folio_zero_new_buffers() does it). So we
need another preparatory patch moving write_end_fn() in
ext4_journalled_zero_new_buffers() to be called also for uptodate pages.

> +		else
> +			folio_zero_new_buffers(folio, from, to);
>  	}
>  #ifdef CONFIG_FS_ENCRYPTION
>  	else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR