On Feb 5, 2020, at 10:30 AM, Jan Kara jack@suse.cz wrote:
DIR_INDEX has been introduced as a compat ext4 feature. That means that even kernels / tools that don't understand the feature may modify the filesystem. This works because for kernels not understanding indexed dir format, internal htree nodes appear just as empty directory entries. Index dir aware kernels then check the htree structure is still consistent before using the data. This all worked reasonably well until metadata checksums were introduced. The problem is that these effectively made DIR_INDEX only ro-compatible because internal htree nodes store checksums in a different place than normal directory blocks. Thus any modification ignorant to DIR_INDEX (or just clearing EXT4_INDEX_FL from the inode) will effectively cause checksum mismatch and trigger kernel errors. So we have to be more careful when dealing with indexed directories on filesystems with checksumming enabled.
- We just disallow loading and directory inodes with EXT4_INDEX_FL when
s/and/any/ ?
DIR_INDEX is not enabled. This is harsh but it should be very rare (it means someone disabled DIR_INDEX on existing filesystem and didn't run e2fsck), e2fsck can fix the problem, and we don't want to answer the difficult question: "Should we rather corrupt the directory more or should we ignore that DIR_INDEX feature is not set?"
Wouldn't it be better to continue allowing the directory to be read, but not modified? Otherwise, essentially, metadata_csum is making the filesystem _less_ robust rather than making it more robust. We don't _need_ the htree index to do a lookup in the directory.
- When we find out htree structure is corrupted (but the filesystem and
the directory should in support htrees), we continue just ignoring htree information for reading but we refuse to add new entries to the directory to avoid corrupting it more.
CC: stable@vger.kernel.org Fixes: dbe89444042a ("ext4: Calculate and verify checksums for htree nodes") Signed-off-by: Jan Kara jack@suse.cz
fs/ext4/dir.c | 14 ++++++++------ fs/ext4/ext4.h | 5 ++++- fs/ext4/inode.c | 13 +++++++++++++ fs/ext4/namei.c | 7 +++++++ 4 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 629a25d999f0..d33135308c1b 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4615,6 +4615,19 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, ret = -EFSCORRUPTED; goto bad_inode; }
- /*
* If dir_index is not enabled but there's dir with INDEX flag set,* we'd normally treat htree data as empty space. But with metadata* checksumming that corrupts checksums so forbid that.*/- if (!ext4_has_feature_dir_index(sb) && ext4_has_metadata_csum(sb) &&
ext4_test_inode_flag(inode, EXT4_INODE_INDEX)) {ext4_error_inode(inode, function, line, 0,"iget: Dir with htree data on filesystem ""without dir_index feature.");
Kernel style suggests error strings should not be line wrapped at 80 columns.
Cheers, Andreas