| 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
| ||
|
Message-ID: <20251015011505.GD6170@frogsfrogsfrogs>
Date: Tue, 14 Oct 2025 18:15:05 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Dave Dykstra <dave.dykstra@...n.ch>
Cc: linux-ext4@...r.kernel.org,
Dave Dykstra <2129743+DrDaveD@...rs.noreply.github.com>
Subject: Re: [PATCH] fuse2fs: reopen filesystem read-write for read-only
journal recovery
On Fri, Oct 10, 2025 at 04:47:35PM -0500, Dave Dykstra wrote:
> This changes the strategy added in c7f2688540d95e7f2cbcd178f8ff62ebe079faf7
> for recovery of journals when read-only is requested. That strategy always
> opened the filesystem file read-write first, in case there was a journal to
> recover. A big problem with that strategy was that the user might not have
> write access to the file. The new strategy with read-only mounts is to
> open the filesystem read-only first, then if there is a journal that needs
> recovery, attempt to reopen it read-write. If that works and the journal
> is recovered, reopen it again read-only.
>
> - Fixes https://github.com/tytso/e2fsprogs/issues/244
>
> Signed-off-by: Dave Dykstra <2129743+DrDaveD@...rs.noreply.github.com>
> ---
> misc/fuse2fs.c | 62 ++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
> index cb5620c7..30a46976 100644
> --- a/misc/fuse2fs.c
> +++ b/misc/fuse2fs.c
> @@ -4607,8 +4607,7 @@ int main(int argc, char *argv[])
> FILE *orig_stderr = stderr;
> char extra_args[BUFSIZ];
> int ret;
> - int flags = EXT2_FLAG_64BITS | EXT2_FLAG_THREADS | EXT2_FLAG_EXCLUSIVE |
> - EXT2_FLAG_RW;
> + int flags = EXT2_FLAG_64BITS | EXT2_FLAG_THREADS | EXT2_FLAG_EXCLUSIVE;
>
> memset(&fctx, 0, sizeof(fctx));
> fctx.magic = FUSE2FS_MAGIC;
> @@ -4689,6 +4688,8 @@ int main(int argc, char *argv[])
>
> /* Start up the fs (while we still can use stdout) */
> ret = 2;
> + if (!fctx.ro)
ro and EXT2_FLAG_RW are not the same thing!
The rw/ro arguments are passed through to the kernel; they determine
whether or not user programs can write to the directory tree. That's up
to the kernel.
EXT2_FLAG_RW determines if the filesystem can be written to at all.
It's set by default, but it is cleared if the user passes norecovery
as an option or the block device can't be opened.
You can have a ro mount of a EXT2_FLAG_RW filesystem. That means that
the filesystem can modify/reorganize itself, even if users can't write
to files.
> + flags |= EXT2_FLAG_RW;
> char options[50];
> sprintf(options, "offset=%lu", fctx.offset);
> if (fctx.directio)
> @@ -4751,8 +4752,12 @@ int main(int argc, char *argv[])
> * ext4 can't do COW of shared blocks, so if the feature is enabled,
> * we must force ro mode.
> */
> - if (ext2fs_has_feature_shared_blocks(global_fs->super))
> + if (ext2fs_has_feature_shared_blocks(global_fs->super) && !fctx.ro) {
> + log_printf(&fctx, "%s\n",
> + _("Mounting read-only because shared blocks feature is enabled."));
> fctx.ro = 1;
> + /* Note that EXT2_FLAG_RW is left set */
> + }
Yeah, the logging here could be improved.
>
> if (ext2fs_has_feature_journal_needs_recovery(global_fs->super)) {
> if (fctx.norecovery) {
> @@ -4761,6 +4766,27 @@ int main(int argc, char *argv[])
> fctx.ro = 1;
> global_fs->flags &= ~EXT2_FLAG_RW;
> } else {
> + if (!(flags & EXT2_FLAG_RW)) {
I don't like this, because we should open the filesystem with
EXT2_FLAG_RW set by default and only downgrade to !EXT2_FLAG_RW if we
can't open it...
> + /* Attempt to re-open read-write */
> + err = ext2fs_close(global_fs);
> + if (err)
> + com_err(argv[0], err,
> + "while closing filesystem");
> + global_fs = NULL;
...if the close fails, you just leaked the old global_fs context.
ext2fs_close_free is what you want (and yes that's a bug in fuse2fs).
> + flags |= EXT2_FLAG_RW;
> + err = ext2fs_open2(fctx.device, options, flags,
> + 0, 0, unix_io_manager,
> + &global_fs);
> + if (err) {
> + err_printf(&fctx, "%s.\n",
> + error_message(err));
> + err_printf(&fctx, "%s\n",
> + _("Journal needs recovery but filesystem cannot be reopened read-write."));
> + err_printf(&fctx, "%s\n",
> + _("Please run e2fsck -fy."));
> + goto out;
> + }
...and also, if you re-do ext2fs_open2(), you then have to re-check all
the feature support bits above because we unlocked the filesystem
device, which means its contents could have been replaced completely
in the interim.
Also note that I have a /very large/ pile of fuse2fs improvements and
rewrites and cleanups that are out for review on the list; you might
want to look at those first.
--D
> + }
> log_printf(&fctx, "%s\n", _("Recovering journal."));
> err = ext2fs_run_ext3_journal(&global_fs);
> if (err) {
> @@ -4772,12 +4798,32 @@ int main(int argc, char *argv[])
> ext2fs_clear_feature_journal_needs_recovery(global_fs->super);
> ext2fs_mark_super_dirty(global_fs);
> }
> + } else if (fctx.ro && !(flags & EXT2_FLAG_RW)) {
> + log_printf(&fctx, "%s\n", _("Mounting read-only."));
> }
>
> - if (global_fs->flags & EXT2_FLAG_RW) {
> + if (fctx.ro && (flags & EXT2_FLAG_RW)) {
> + /* Re-open read-only */
> + err = ext2fs_close(global_fs);
> + if (err)
> + com_err(argv[0], err, "while closing filesystem");
> + global_fs = NULL;
> + flags &= ~EXT2_FLAG_RW;
> + err = ext2fs_open2(fctx.device, options, flags, 0, 0,
> + unix_io_manager, &global_fs);
> + if (err) {
> + err_printf(&fctx, "%s.\n", error_message(err));
> + err_printf(&fctx, "%s\n",
> + _("Failed to remount read-only."));
> + goto out;
> + }
> + log_printf(&fctx, "%s\n", _("Remounted read-only."));
> + }
> +
> + if (!fctx.ro) {
> if (ext2fs_has_feature_journal(global_fs->super))
> log_printf(&fctx, "%s",
> - _("Warning: fuse2fs does not support using the journal.\n"
> + _("Warning: fuse2fs does not support writing the journal.\n"
> "There may be file system corruption or data loss if\n"
> "the file system is not gracefully unmounted.\n"));
> err = ext2fs_read_inode_bitmap(global_fs);
> @@ -4833,8 +4879,10 @@ int main(int argc, char *argv[])
> if (fctx.no_default_opts == 0)
> fuse_opt_add_arg(&args, extra_args);
>
> - if (fctx.ro)
> + if (fctx.ro) {
> + /* This is in case ro was implied above and not passed in */
> fuse_opt_add_arg(&args, "-oro");
> + }
>
> if (fctx.fakeroot) {
> #ifdef HAVE_MOUNT_NODEV
> @@ -4892,7 +4940,6 @@ int main(int argc, char *argv[])
> ret = 0;
> break;
> }
> -out:
> if (ret & 1) {
> fprintf(orig_stderr, "%s\n",
> _("Mount failed due to unrecognized options. Check dmesg(1) for details."));
> @@ -4903,6 +4950,7 @@ out:
> _("Mount failed while opening filesystem. Check dmesg(1) for details."));
> fflush(orig_stderr);
> }
> +out:
> if (global_fs) {
> err = ext2fs_close(global_fs);
> if (err)
> --
> 2.43.5
>
>
Powered by blists - more mailing lists