Re: [BUG+PATCH] mke2fs: Inode checksum does not match inode while creating orphan file

Re: [BUG+PATCH] mke2fs: Inode checksum does not match inode while creating orphan file

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Thu 24-08-23 14:29:50, Sven Zühlsdorf wrote:
> Hi,
> 
> using version 1.7.0 (commit 25ad8a43) I encountered a bug in mke2fs, as
> creating new filesystems now fails on some devices:
> > # mke2fs -t ext4 /dev/nvme0n1p2
> > mke2fs 1.47.0 (5-Feb-2023)
> > Discarding device blocks: done
> > Creating filesystem with 4194304 4k blocks and 1048576 inodes
> > Filesystem UUID: d379784e-c92d-431f-b527-c4088299a914
> > Superblock backups stored on blocks:
> > 	32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632, 2654208,
> > 	4096000
> > 
> > Allocating group tables: done
> > Writing inode tables: done
> > Creating journal (32768 blocks): done
> > mke2fs: Inode checksum does not match inode while creating orphan file
> 
> As a workaround, disabling the new orphan_file feature (i.e. `mke2fs -O
> ^orphan_file ...') allows mke2fs to succeed.
> 
> I could trace this to ext2fs_create_orphan_file's call to ext2fs_read_inode
> which reads the inode from disk, disregarding that the inode may have just
> been created and not written to disk yet.
> On devices where discarding produces NULs this isn't an issue, since
> ext2fs_read_inode will read a zeroed-out struct inode and the checksum
> verification function has a special case for the checksum still being zero.
> I assume enabling orphan_file after a file system has been in use for some
> time could run into this bug as well, but I haven't verified that.
> On some of the devices we use, however, discard results in random looking
> data, leading to above checksum failure.
> 
> From other places creating inodes I cobbled together the attached patch,
> allowing mke2fs to succceed; a subsequent forced fsck succeeds with no
> issues as well.

Thanks for the analysis and the fix! It looks good, I've just somewhat
fixed up whitespace (end of lines got somehow garbled for me) and also
removed the pointless inode reading in ext2fs_create_orphan_file(). The
result is attached.

								Honza

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

[-- Attachment #2: 0001-ext2fs-Fix-initialization-of-orphan-file.patch --]
[-- Type: text/x-patch, Size: 2976 bytes --]

From aafde8a3ce496f06fcb33909631bbe72a5e8d84e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sven=20Z=C3=BChlsdorf?= <sven.zuehlsdorf@vigem.de>
Date: Thu, 24 Aug 2023 21:56:29 +0200
Subject: [PATCH] ext2fs: Fix initialization of orphan file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On block devices where discard produces random looking data instead of
NULs, creating a file system fails:

mke2fs 1.47.0 (5-Feb-2023)
Discarding device blocks: done
Creating filesystem with 4194304 4k blocks and 1048576 inodes
Filesystem UUID: d379784e-c92d-431f-b527-c4088299a914
Superblock backups stored on blocks:
      32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632, 2654208,
      4096000

Allocating group tables: done
Writing inode tables: done
Creating journal (32768 blocks): done
mke2fs: Inode checksum does not match inode while creating orphan file

This is caused by ext2fs_create_orphan_file's call to
ext2fs_read_inode() which is reading effectively uninitialized data, as
the inode created just above hasn't been written to disk yet.  Devices
where discarding produces NULs are not affected as ext2fs_read_inode()
returns a zeroed-out struct ext2_inode, for which the checksum
calculation has a special case masking this bug.

Make sure the inode is not read when it is not yet written.

Fixes: 1d551c68 ("libext2fs: Support for orphan file feature")
Signed-off-by: Sven Zühlsdorf <sven.zuehlsdorf@vigem.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 lib/ext2fs/orphan.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/ext2fs/orphan.c b/lib/ext2fs/orphan.c
index e25f20ca247c..be27a086679b 100644
--- a/lib/ext2fs/orphan.c
+++ b/lib/ext2fs/orphan.c
@@ -134,24 +134,23 @@ errcode_t ext2fs_create_orphan_file(ext2_filsys fs, blk_t num_blocks)
 			return err;
 		ext2fs_inode_alloc_stats2(fs, ino, +1, 0);
 		ext2fs_mark_ib_dirty(fs);
-	}
-
-	err = ext2fs_read_inode(fs, ino, &inode);
-	if (err)
-		return err;
-	if (EXT2_I_SIZE(&inode)) {
-		err = ext2fs_truncate_orphan_file(fs);
+	} else {
+		err = ext2fs_read_inode(fs, ino, &inode);
 		if (err)
 			return err;
+		if (EXT2_I_SIZE(&inode)) {
+			err = ext2fs_truncate_orphan_file(fs);
+			if (err)
+				return err;
+		}
 	}
 
 	memset(&inode, 0, sizeof(struct ext2_inode));
-	if (ext2fs_has_feature_extents(fs->super)) {
+	if (ext2fs_has_feature_extents(fs->super))
 		inode.i_flags |= EXT4_EXTENTS_FL;
-		err = ext2fs_write_inode(fs, ino, &inode);
-		if (err)
-			return err;
-	}
+	err = ext2fs_write_new_inode(fs, ino, &inode);
+	if (err)
+		return err;
 
 	err = ext2fs_get_mem(fs->blocksize, &buf);
 	if (err)
@@ -194,7 +193,7 @@ errcode_t ext2fs_create_orphan_file(ext2_filsys fs, blk_t num_blocks)
 			(unsigned long long)fs->blocksize * num_blocks);
 	if (err)
 		goto out;
-	err = ext2fs_write_new_inode(fs, ino, &inode);
+	err = ext2fs_write_inode(fs, ino, &inode);
 	if (err)
 		goto out;
 
-- 
2.35.3