Hi Eric!
On Mon 16-11-20 20:28:32, Eric Biggers wrote:
> On Wed, Feb 05, 2020 at 06:30:25PM +0100, Jan Kara 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.
> >
> > 1) We just disallow loading and directory inodes with EXT4_INDEX_FL when
> > 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?"
> >
> > 2) 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@xxxxxxxxxxxxxxx
> > Fixes: dbe89444042a ("ext4: Calculate and verify checksums for htree nodes")
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > 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/ext4.h b/fs/ext4/ext4.h
> > index f8578caba40d..1fd6c1e2ce2a 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -2482,8 +2482,11 @@ void ext4_insert_dentry(struct inode *inode,
> > struct ext4_filename *fname);
> > static inline void ext4_update_dx_flag(struct inode *inode)
> > {
> > - if (!ext4_has_feature_dir_index(inode->i_sb))
> > + if (!ext4_has_feature_dir_index(inode->i_sb)) {
> > + /* ext4_iget() should have caught this... */
> > + WARN_ON_ONCE(ext4_has_feature_metadata_csum(inode->i_sb));
> > ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
> > + }
> > }
>
> This new WARN_ON_ONCE() gets triggered by the following commands:
>
> mkfs.ext4 -O ^dir_index /dev/vdc
> mount /dev/vdc /vdc
> mkdir /vdc/dir
>
> WARNING: CPU: 1 PID: 305 at fs/ext4/ext4.h:2700 add_dirent_to_buf+0x1d0/0x1e0 fs/ext4/namei.c:2039
> CPU: 1 PID: 305 Comm: mkdir Not tainted 5.10.0-rc4 #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ArchLinux 1.14.0-1 04/01/2014
> RIP: 0010:ext4_update_dx_flag fs/ext4/ext4.h:2700 [inline]
> RIP: 0010:add_dirent_to_buf+0x1d0/0x1e0 fs/ext4/namei.c:2038
> [...]
> Call Trace:
> ext4_add_entry+0x179/0x4d0 fs/ext4/namei.c:2248
> ext4_mkdir+0x1c0/0x320 fs/ext4/namei.c:2814
> vfs_mkdir+0xcc/0x130 fs/namei.c:3650
> do_mkdirat+0x81/0x120 fs/namei.c:3673
> __do_sys_mkdir fs/namei.c:3689 [inline]
>
> What is intended here? metadata_csum && ^dir_index is a weird combination,
> but it's technically valid, right?
Indeed the WARN_ON_ONCE() is wrong. It should also check that
EXT4_INODE_INDEX is set. The idea of the warning is that when we just clear
EXT4_INODE_INDEX flag, checksums will become invalid so generally that's
not desirable... I'll send a fix. Thanks for report!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
