diff mbox series

e2fsck: Always probe filesystem blocksize with simple io_manager

Message ID 87pml4lt5v.fsf_-_@collabora.com
State Accepted
Headers show
Series e2fsck: Always probe filesystem blocksize with simple io_manager | expand

Commit Message

Gabriel Krisman Bertazi April 25, 2022, 10:01 p.m. UTC 
"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(-)

Comments

Gabriel Krisman Bertazi June 2, 2022, 5:52 p.m. UTC | #1 
Gabriel Krisman Bertazi <krisman@collabora.com> writes:

> "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.
>

Hi Ted, any interest in picking this up?  quite a corner case of
e2fsprogs, but I think it simplifies that path a bit. :)

> 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);
Theodore Ts'o Aug. 11, 2022, 2:32 p.m. UTC | #2 
On Mon, 25 Apr 2022 18:01:00 -0400, Gabriel Krisman Bertazi wrote:
>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.

Applied, thanks!

[1/1] e2fsck: Always probe filesystem blocksize with simple io_manager
      commit: 0ae0e93624a933a1c6ea4e4680ff5d609f267a43

Best regards,
diff mbox series

Patch

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);