Simplify rejection of unexpected casefold inode flag

From: Eric Biggers <[email protected]>

It is invalid for the casefold inode flag to be set without the casefold superblock feature flag also being set. e2fsck already considers this case to be invalid and handles it by offering to clear the casefold flag on the inode. __ext4_iget() also already considered this to be invalid, sort of, but it only got so far as logging an error message; it didn't actually reject the inode. Make it reject the inode so that other code doesn't have to handle this case. This matches what f2fs does.

Note: we could check 's_encoding != NULL' instead of ext4_has_feature_casefold(). This would make the check robust against the casefold feature being enabled by userspace writing to the page cache of the mounted block device. However, it's unsolvable in general for filesystems to be robust against concurrent writes to the page cache of the mounted block device. Though this very particular scenario involving the casefold feature is solvable, we should not pretend that we can support this model, so let's just check the casefold feature. tune2fs already forbids enabling casefold on a mounted filesystem.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/ext4/inode.c +4 -1

index 43775a6ca505..390dedbb7e8a 100644
@@ -4940,9 +4940,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 				 "iget: bogus i_mode (%o)", inode->i_mode);
 		goto bad_inode;
 	}
-	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb))
+	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb)) {
 		ext4_error_inode(inode, function, line, 0,
 				 "casefold flag without casefold feature");
+		ret = -EFSCORRUPTED;
+		goto bad_inode;
+	}
 	if ((err_str = check_igot_inode(inode, flags)) != NULL) {
 		ext4_error_inode(inode, function, line, 0, err_str);
 		ret = -EFSCORRUPTED;

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>

It is invalid for the casefold inode flag to be set without the casefold superblock feature flag also being set. e2fsck already considers this case to be invalid and handles it by offering to clear the casefold flag on the inode. __ext4_iget() also already considered this to be invalid, sort of, but it only got so far as logging an error message; it didn't actually reject the inode. Make it reject the inode so that other code doesn't have to handle this case. This matches what f2fs does.

Quoted 9 lines

Note: we could check 's_encoding != NULL' instead of ext4_has_feature_casefold(). This would make the check robust against the casefold feature being enabled by userspace writing to the page cache of the mounted block device. However, it's unsolvable in general for filesystems to be robust against concurrent writes to the page cache of the mounted block device. Though this very particular scenario involving the casefold feature is solvable, we should not pretend that we can support this model, so let's just check the casefold feature. tune2fs already forbids enabling casefold on a mounted filesystem.

just because we can't fix the general issue for the entire filesystem doesn't mean this case *must not* ever be addressed. What is the advantage of making the code less robust against the syzbot code? Just check sb->s_encoding and be safe later knowing the unicode map is available.

Quoted 22 lines · Gabriel Krisman Bertazi

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>

It is invalid for the casefold inode flag to be set without the casefold superblock feature flag also being set. e2fsck already considers this case to be invalid and handles it by offering to clear the casefold flag on the inode. __ext4_iget() also already considered this to be invalid, sort of, but it only got so far as logging an error message; it didn't actually reject the inode. Make it reject the inode so that other code doesn't have to handle this case. This matches what f2fs does.

Quoted 9 lines

Note: we could check 's_encoding != NULL' instead of ext4_has_feature_casefold(). This would make the check robust against the casefold feature being enabled by userspace writing to the page cache of the mounted block device. However, it's unsolvable in general for filesystems to be robust against concurrent writes to the page cache of the mounted block device. Though this very particular scenario involving the casefold feature is solvable, we should not pretend that we can support this model, so let's just check the casefold feature. tune2fs already forbids enabling casefold on a mounted filesystem.

Quoted 6 lines

just because we can't fix the general issue for the entire filesystem doesn't mean this case *must not* ever be addressed. What is the advantage of making the code less robust against the syzbot code? Just check sb->s_encoding and be safe later knowing the unicode map is available.

Just to make sure, it sounds like you agree that the late checks of ->s_encoding are not needed and only __ext4_iget() should handle it, right? That simplifies the code so it is obviously beneficial if we can do it.

This series makes unexpected casefold flags on inodes be consistently rejected early on so that additional validation isn't needed later on during random filesystem operations. For additional context, refer to the discussion on patch 1 of
https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u

Applies to v6.5-rc6

[...]