On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote: > Eric Biggers <ebiggers@xxxxxxxxxx> writes: > > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > 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. > > 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. As for whether __ext4_iget() should check the casefold feature or ->s_encoding, we should simply go with the one that makes the code clearer, as per what I've said. I think it's casefold, but it could be ->s_encoding if others prefer. - Eric
