linux-kernel - Re: [PATCH 3/5] ext4: abort journal on data writeback failure if in data

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: <20241220103617.xkqmwkmk5inlq3dz@quack3>
Date: Fri, 20 Dec 2024 11:36:17 +0100
From: Jan Kara <jack@...e.cz>
To: libaokun@...weicloud.com
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
	jack@...e.cz, linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
	yangerkun@...wei.com, Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 3/5] ext4: abort journal on data writeback failure if in
 data_err=abort mode

On Fri 20-12-24 14:07:55, libaokun@...weicloud.com wrote:
> From: Baokun Li <libaokun1@...wei.com>
> 
> If we mount an ext4 fs with data_err=abort option, it should abort on
> file data write error. But if the extent is unwritten, we won't add a
> JI_WAIT_DATA bit to the inode, so jbd2 won't wait for the inode's data
> to be written back and check the inode mapping for errors. The data
> writeback failures are not sensed unless the log is watched or fsync
> is called.
> 
> Therefore, when data_err=abort is enabled, the journal is aborted when
> an I/O error is detected in ext4_end_io_end() to make users who are
> concerned about the contents of the file happy.
> 
> Signed-off-by: Baokun Li <libaokun1@...wei.com>

I'm not opposed to this change but I think we should better define the
expectations around data_err=abort. For example the dependency on
data=ordered is kind of strange and the current semantics of data_err=abort
are hard to understand for admins (since they are mostly implementation
defined). For example if IO error happens on data overwrites, the
filesystem will not be aborted because we don't bother tracking such data
as ordered (for performance reasons). Since you've apparently talked to people
using this option: What are their expectations about the option?

								Honza


> ---
>  fs/ext4/page-io.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 6054ec27fb48..058bf4660d7b 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -175,6 +175,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
>  {
>  	struct inode *inode = io_end->inode;
>  	handle_t *handle = io_end->handle;
> +	struct super_block *sb = inode->i_sb;
>  	int ret = 0;
>  
>  	ext4_debug("ext4_end_io_nolock: io_end 0x%p from inode %lu,list->next 0x%p,"
> @@ -190,11 +191,15 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
>  		ret = -EIO;
>  		if (handle)
>  			jbd2_journal_free_reserved(handle);
> +		if (test_opt(sb, DATA_ERR_ABORT) &&
> +		    !ext4_is_quota_file(inode) &&
> +		    ext4_should_order_data(inode))
> +			jbd2_journal_abort(EXT4_SB(sb)->s_journal, ret);
>  	} else {
>  		ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
>  	}
> -	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
> -		ext4_msg(inode->i_sb, KERN_EMERG,
> +	if (ret < 0 && !ext4_forced_shutdown(sb)) {
> +		ext4_msg(sb, KERN_EMERG,
>  			 "failed to convert unwritten extents to written "
>  			 "extents -- potential data loss!  "
>  			 "(inode %lu, error %d)", inode->i_ino, ret);
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR