Re: [PATCH] ext4: skip extra isize expansion while unmounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for a TOCTOU race condition that can lead to a NULL pointer dereference during unmount.
--

--- Patch [1]: [PATCH] ext4: skip extra isize expansion while unmounting ---
commit b6a44628f998028bf5040431bbd5fcff301de3c8
Author: Hemanth Selam <hemanth.selam@gmail.com>

ext4: skip extra isize expansion while unmounting
    
This patch attempts to fix a warning in ext4_xattr_inode_create reached
during unmount. It declines to expand the extra isize when s_root is
cleared during the dcache shrink but before SB_ACTIVE is cleared.

Closes: https://syzkaller.appspot.com/bug?extid=4b03894b6ec5753ddf24

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bd4b778df9eb2..6e523a5c8230e 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6598,8 +6598,14 @@ static int ext4_try_to_expand_extra_isize(struct inode *inode,
[ ... ]
> +	 * Skip it while unmounting as well.  shrink_dcache_for_umount()
> +	 * clears s_root before generic_shutdown_super() clears SB_ACTIVE, and
> +	 * the last iput() of a lazytime inode in that window redirties it and
> +	 * lands here.  Moving xattrs out to a block then needs a new EA inode,
> +	 * which ext4_xattr_inode_create() refuses without s_root.
>  	 */
> -	if (unlikely(!(inode->i_sb->s_flags & SB_ACTIVE)))
> +	if (unlikely(!(inode->i_sb->s_flags & SB_ACTIVE) || !inode->i_sb->s_root))
>  		return -EBUSY;
>  
>  	/*

[Severity: High]
This isn't a bug introduced by this patch, but does checking s_root here
without synchronization leave a race open during unmount?

Background workqueues like rsv_conversion_wq and s_ea_inode_work can
process inodes concurrently with unmount. They are not flushed until
sync_filesystem() and ext4_put_super(), which run after
shrink_dcache_for_umount() clears s_root.

If a background thread sees a non-NULL s_root here, could the unmount
thread concurrently clear s_root right after? The background thread would
then proceed into ext4_xattr_inode_create():

fs/ext4/xattr.c:ext4_xattr_inode_create() {
    ...
    ea_inode = ext4_new_inode(handle, inode->i_sb->s_root->d_inode,
                              S_IFREG | 0600, NULL, inode->i_ino + 1, owner,
                              EXT4_EA_INODE_FL);
    ...
}

If s_root is cleared by the unmount thread just before this dereference,
could this result in a NULL pointer dereference on s_root->d_inode? Or if
s_root is cleared right before the warning check inside
ext4_xattr_inode_create(), wouldn't we still hit the original warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824094512.2436628-1-hemanth.selam@gmail.com?part=1