Message-ID: <z52ea53du2k66du24ju4yetqm72e6pvtcbwkrjf4oomw2feffq@355vymdndrxn>
Date: Thu, 2 Jan 2025 16:58:14 +0100
From: Jan Kara <jack@...e.cz>
To: Baokun Li <libaokun1@...wei.com>
Cc: "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>, 
	Theodore Ts'o <tytso@....edu>, Jan Kara <jack@...e.cz>, Christian Brauner <brauner@...nel.org>, 
	sunyongjian1@...wei.com, Yang Erkun <yangerkun@...wei.com>
Subject: Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?

On Mon 30-12-24 15:27:01, Baokun Li wrote:
> After commit d3476f3dad4a (“ext4: don't set SB_RDONLY after filesystem
> errors”) in v6.12-rc1, the “errors=remount-ro” mode no longer sets
> SB_RDONLY on errors, which results in us seeing the filesystem is still
> in rw state after errors. The issue fixed by this patch is reported as
> CVE-2024-50191.
> 
> https://lore.kernel.org/all/2024110851-CVE-2024-50191-f31c@gregkh/
> 
> This has actually changed the remount-ro semantics. We have some fault
> injection test cases where we unmount the filesystem after detecting
> a ro state and then check for consistency. Our customer has a similar
> scenario. In "errors=remount-ro" mode, some operations are performed
> after the file system becomes read-only.

I'm sorry to hear that your fault injection has been broken by my changes.

> We reported similar issues to the community in 2020,
> https://lore.kernel.org/all/20210104160457.GG4018@quack2.suse.cz/
> Jan Kara provides a simple and effective patch. This patch somehow
> didn't end up merged into upstream, but this patch has been merged into
> our internal version for a couple years now and it works fine, is it
> possible to revert the patch that no longer sets SB_RDONLY and use
> the patch in the link above?

Well, the problem with filesystem freezing was just the immediate trigger
for the changes. But the setting of SB_RDONLY bit by ext4 behind the VFS'
back (as VFS is generally in charge of manipulating that bit and you must
hold s_umount for that which we cannot get in ext4 when handling errors)
was always problematic and I'm almost sure syzbot would find more problems
with that than just fs freezing. As such I don't think we should really
return to doing that in ext4 but we need to find other ways how to make
your error injection to work...

> What's worse is that after commit
>   95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
> was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
> ext4_handle_error(). This causes the file system to not be read-only
> when an error is triggered in "errors=remount-ro" mode, because
> EXT4_FLAGS_SHUTDOWN prevents both writing and reading.

Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
removal. What do you exactly mean by "causes the file system to not be
read-only"? We still return EROFS where we used to, we disallow writing as
we used to. Can you perhaps give an example what changed with this commit?

> I'm not sure
> if this is the intended behavior. But if the intention is to shut down
> the file system upon an error, I feel it would be better to add an
> "errors=shutdown" option.

The point was not really to create new "errors=" mode but rather implement
the "don't modify the filesystem after we spot error" behavior without
touching the SB_RDONLY flag.

So how does your framework detect that the filesystem has failed with
errors=remount-ro? By parsing /proc/mounts or otherwise querying current
filesystem mount options? Would it be acceptable for you to look at some
other mount option (e.g. "shutdown") to detect that state? We could easily
implement that.

I'm sorry again for causing you trouble.

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

Powered by blists - more mailing lists