Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@collabora.com>
To:     "Theodore Ts'o" <tytso@mit.edu>
Cc:     Peter Urbanec <linux-ext4.vger.kernel.org@urbanec.net>,
        linux-ext4@vger.kernel.org
Subject: [PATCH] e2fsck: Always probe filesystem blocksize with simple
 io_manager
Organization: Collabora
References: <493bbaea-b0d3-4f8e-20fb-5fb330a128a3@urbanec.net>
        <YlniK5dd1wV2bCXi@mit.edu>
Date:   Mon, 25 Apr 2022 18:01:00 -0400
In-Reply-To: <YlniK5dd1wV2bCXi@mit.edu> (Theodore Ts'o's message of "Fri, 15
        Apr 2022 17:22:51 -0400")
Message-ID: <87pml4lt5v.fsf_-_@collabora.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

"Theodore Ts'o" <tytso@mit.edu> writes:

> So the failure of "e2fsck -f -C 0 -b 32768 -z /root/e2fsck.e2undo
> /dev/md0" appears to be a bug where e2fsck doesn't work correctly with
> an undo file when using a backup superblock.  I can replicate this
> using these commands:
>
> 	mke2fs -q -t ext4 /tmp/foo.img 2G
> 	e2fsck -b 32768 -z /tmp/undo /tmp/foo.img
>
> Running e2fsck without the -z option succeeds.  The combination of the
> -b and -z option seems to be broken.  As a workaround, I would suggest
> doing is to try running e2fsck with -n, which will open the block
> device read-only, e.g. "e2fsck -b 32768 -n /dev/mdXX".  If the changes
> e2fsck look safe, then you can run e2fsck without the -n option.

Ted,

I think this is a fix for the combination of -b and -z.

Thanks,

>8

Combining superblock (-b) with undo file (-z) fails iff the block size
is not specified (-B) and is different from the first blocksize probed
in try_open_fs (1k).  The reason is as follows:

try_open_fs will probe different blocksizes if none is provided on the
command line. It is done by opening and closing the filesystem until it
finds a blocksize that makes sense. This is fine for all io_managers,
but undo_io creates the undo file with that blocksize during
ext2fs_open.  Once try_open_fs realizes it had the wrong blocksize and
retries with a different blocksize, undo_io will read the previously
created file and think it's corrupt for this filesystem.

Ideally, undo_io would know this is a probe and would fix the undo file.
It is not simple, though, because it would require undo_io to know the
file was just created by the probe code, since an undo file survives
through different fsck sessions.  We'd have to pass this information
around somehow.  This seems like a complex change to solve a corner
case.

Instead, this patch changes the blocksize probe to always use the
unix_io_manager. This way, we safely probe for the blocksize without
side effects.  Once the blocksize is known, we can safely reopen the
filesystem under the proper io_manager.

An easily reproducer for this issue (from Ted, adapted by me) is:

 mke2fs -b 4k -q -t ext4 /tmp/foo.img 2G
 e2fsck -b 32768 -z /tmp/undo /tmp/foo.img

Reported-by: Peter Urbanec <linux-ext4.vger.kernel.org@urbanec.net>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 e2fsck/unix.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index ae231f93deb7..341b484e6ede 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1171,25 +1171,32 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
 	errcode_t retval;
 
 	*ret_fs = NULL;
-	if (ctx->superblock && ctx->blocksize) {
-		retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
-				      flags, ctx->superblock, ctx->blocksize,
-				      io_ptr, ret_fs);
-	} else if (ctx->superblock) {
-		int blocksize;
-		for (blocksize = EXT2_MIN_BLOCK_SIZE;
-		     blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
-			if (*ret_fs) {
-				ext2fs_free(*ret_fs);
-				*ret_fs = NULL;
+
+	if (ctx->superblock) {
+		unsigned long blocksize = ctx->blocksize;
+
+		if (!blocksize) {
+			for (blocksize = EXT2_MIN_BLOCK_SIZE;
+			     blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
+
+				retval = ext2fs_open2(ctx->filesystem_name,
+						      ctx->io_options, flags,
+						      ctx->superblock, blocksize,
+						      unix_io_manager, ret_fs);
+				if (*ret_fs) {
+					ext2fs_free(*ret_fs);
+					*ret_fs = NULL;
+				}
+				if (!retval)
+					break;
 			}
-			retval = ext2fs_open2(ctx->filesystem_name,
-					      ctx->io_options, flags,
-					      ctx->superblock, blocksize,
-					      io_ptr, ret_fs);
-			if (!retval)
-				break;
+			if (retval)
+				return retval;
 		}
+
+		retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
+				      flags, ctx->superblock, blocksize,
+				      io_ptr, ret_fs);
 	} else
 		retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
 				      flags, 0, 0, io_ptr, ret_fs);
-- 
2.35.1