Re: [PATCH v4] ext4: fix kernel BUG in ext4_write_inline_data_end

[Jan Kara]

On Tue 09-06-26 06:20:05, Aditya Prakash Srivastava wrote:
> When the data=journal mount option is used, the ext4_journalled_write_end()
> function incorrectly calls ext4_write_inline_data_end() without checking
> if the EXT4_STATE_MAY_INLINE_DATA flag is still set on the inode.
> 
> If a previous attempt to convert the inline data to an extent failed (e.g.
> due to ENOSPC), the EXT4_STATE_MAY_INLINE_DATA flag is cleared, but
> the EXT4_INODE_INLINE_DATA flag remains set. In this scenario, the next
> call to ext4_write_begin() will not prepare the inline data xattr for
> writing, but ext4_journalled_write_end() will incorrectly attempt to write
> to it, triggering a BUG_ON(pos + len > EXT4_I(inode)->i_inline_size) in
> ext4_write_inline_data() since i_inline_size was not expanded.
> 
> Additionally, two separate TOCTOU race conditions exist due to concurrent
> ext4_page_mkwrite() execution:
> 1) A concurrent ext4_page_mkwrite() can execute ext4_convert_inline_data()
> between write_begin and write_end, clearing the inline flags. Since block
> buffers were not allocated in write_begin, this results in a NULL pointer
> dereference in the write_end fallback paths because folio_buffers(folio) is
> NULL.
> 2) If ext4_convert_inline_data() clears the flags exactly after the inline
> flags checks pass in write_end, but before ext4_write_inline_data_end()
> acquires the xattr semaphore, the subsequent check will hit a panic via
> BUG_ON(!ext4_has_inline_data(inode)).

Yes, locking of inline data writes is broken (and difficult to fix). Your
v1 patch was actually simple and obvious improvement of the situation.
These additional fixes belong into separate patches.

> Fix these issues completely by:
> 1) Having write_end functions (ext4_write_end(),
> ext4_journalled_write_end(), and ext4_da_do_write_end()) return 0
> (VFS retry) if they fall through to the block fallback path and detect
> that folio_buffers(folio) is NULL, after safely stopping any active
> journal handle (protecting against a NULL handle panic in
> ext4_put_nojournal()).
> 2) Replacing BUG_ON(!ext4_has_inline_data(inode)) inside
> ext4_write_inline_data_end() with a graceful error path. If the inline flag
> is cleared after locking the xattr, we unlock the xattr, release the iloc,
> unlock/put the folio, stop the journal, and return 0 to trigger a retry.
> 
> Reported-by: syzbot+0c89d865531d053abb2d@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
> Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
> Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>
> ---
> v4:
>   - Address critical TOCTOU race condition (reported by Sashiko AI review):
>     * Scenario: A buffered write holds the folio lock and evaluates the inline
>       flags checks in write_end to true. Before it enters or locks the xattr_sem
>       in ext4_write_inline_data_end(), a concurrent memory-mapped page fault
>       (ext4_page_mkwrite()) converts the inline data to an extent. This page fault
>       bypasses the folio lock (since ext4_convert_inline_data() runs lockless),
>       acquires the xattr_sem, and clears the inline flags. When the buffered write
>       resumes and enters ext4_write_inline_data_end(), it acquires the xattr_sem
>       and immediately triggers BUG_ON(!ext4_has_inline_data(inode)) causing a
>       kernel panic.
>     * Fix: Replace the BUG_ON() with a graceful error-handling retry path that
>       releases all resources (locks/buffers/folios/journals) and returns 0.
> v3:
>   - Fix journal handle leak and NULL handle crash (reported by Sashiko AI review):
>     * Scenario 1 (leak): During a delayed allocation write (ext4_da_write_begin),
>       inline data was prepared and a transaction handle started. If a concurrent
>       page fault converts the inline data before write_end, ext4_da_write_end()
>       falls through to ext4_da_do_write_end(). If the fallback check for
>       !folio_buffers(folio) returns 0 to retry without calling ext4_journal_stop(),
>       the transaction handle is leaked open-ended, eventually hanging the filesystem.
>     * Scenario 2 (crash): If we blindly call ext4_journal_stop() on a NULL handle
>       (e.g., when no transaction was started because we never took the inline path),
>       __ext4_journal_stop() delegates to ext4_put_nojournal(NULL) which triggers
>       BUG_ON(ref_cnt == 0), panicking the kernel.
>     * Fix: Retrieve the active handle in ext4_da_do_write_end() and stop it
>       if non-NULL. Also explicitly check "if (handle)" before calling
>       ext4_journal_stop() in ext4_write_end() and ext4_journalled_write_end().
> v2:
>   - Address TOCTOU race condition (reported by Sashiko AI review):
>     * Scenario: A concurrent ext4_page_mkwrite() converts inline data to extents
>       and clears the flags between ext4_write_begin() and write_end(). The
>       write_end function falls through to the block fallback path. Since block
>       buffers were not allocated in write_begin (because it took the inline path),
>       folio_buffers(folio) is NULL, causing a NULL pointer dereference in
>       ext4_journalled_zero_new_buffers() or ext4_walk_page_buffers(), or silent
>       data loss in the standard write path.
>     * Fix: Have the write_end functions return 0 if folio_buffers(folio) is NULL,
>       triggering a safe VFS-level retry. On the next write attempt, the inline
>       flags will be detected as cleared, and blocks/buffers will be properly allocated.
>  fs/ext4/inline.c |  9 ++++++++-
>  fs/ext4/inode.c  | 24 ++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 8045e4ff270c..161136e84661 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -812,7 +812,14 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
>  			goto out;
>  		}
>  		ext4_write_lock_xattr(inode, &no_expand);
> -		BUG_ON(!ext4_has_inline_data(inode));
> +		if (unlikely(!ext4_has_inline_data(inode))) {
> +			ext4_write_unlock_xattr(inode, &no_expand);
> +			brelse(iloc.bh);
> +			folio_unlock(folio);
> +			folio_put(folio);
> +			ext4_journal_stop(handle);
> +			return 0;
> +		}

This deserves a comment before the 'if' that we could have raced with
ext4_page_mkwrite() converting the inode and so we just retry the whole
write.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index c2c2d6ac7f3d..bc2688e03c19 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1455,6 +1455,14 @@ static int ext4_write_end(const struct kiocb *iocb,
>  		return ext4_write_inline_data_end(inode, pos, len, copied,
>  						  folio);
>  
> +	if (unlikely(!folio_buffers(folio))) {
> +		folio_unlock(folio);
> +		folio_put(folio);
> +		if (handle)
> +			ext4_journal_stop(handle);
> +		return 0;
> +	}
> +

Ouch, this is a crude hack. I think much cleaner solution would be for
ext4_write_begin() to set in fsdata in what state it prepared the inode
(inline, extent based) - we already use that mechanism to communicate some
state for delayed allocations. Then ->write_end handler will use fsdata
(not inode state) to determine what function to call. IMHO the code will be
much more obvious that way.

> @@ -3231,7 +3248,10 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	if (unlikely(!folio_buffers(folio))) {
>  		folio_unlock(folio);
>  		folio_put(folio);
> -		return -EIO;
> +		handle = ext4_journal_current_handle();
> +		if (handle)
> +			ext4_journal_stop(handle);
> +		return 0;
>  	}
>  	/*
>  	 * block_write_end() will mark the inode as dirty with I_DIRTY_PAGES

Huh, what is this about? It definitely looks very suspicious...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR