Re: [PATCH 0/2] ext4: fix race conditions and clean up locking of inline data writes

[Jan Kara]

On Tue 30-06-26 15:28:10, Aditya Srivastava wrote:
> From: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>
> 
> This patch series addresses the remaining race conditions and locking
> issues involved with inline data writes, implementing the clean
> state-communication design suggested by Jan Kara.
> 
> Previously, `ext4_write_end()`, `ext4_journalled_write_end()`, and
> `ext4_da_write_end()` checked the inode state and the inline data flag
> directly to decide whether to finish writing inline data or to fall
> back to block writes. This is highly susceptible to TOCTOU race
> conditions where concurrent memory-mapped page faults
> (`ext4_page_mkwrite()`) can convert the inline data to an extent
> between `write_begin` and `write_end`. Since block buffers were not
> allocated in the inline path during `write_begin`, such fallbacks
> resulted in kernel crashes and NULL pointer dereferences because
> `folio_buffers(folio)` was NULL.
> 
> The series cleans up and resolves these issues in two distinct steps:
> 
> 1) Patch 1 introduces state tracking via the standard `fsdata`
>    parameter. By marking whether a write was prepared as inline
>    (`EXT4_WRITE_DATA_INLINE`) directly in the private per-write
>    `fsdata` during `write_begin`, the corresponding `write_end`
>    handlers can reliably decide whether to call
>    `ext4_write_inline_data_end()` or complete a normal extent write.
>    This eliminates the race-prone checks on the live inode state and
>    gets rid of crude fallback/retry hacks.
> 
> 2) Patch 2 replaces a potential kernel panic
>    (`BUG_ON(!ext4_has_inline_data(inode))`) inside
>    `ext4_write_inline_data_end()` with a graceful retry error path.
>    If a concurrent conversion clears the inline flag right after the
>    `write_end` checks pass but before the xattr semaphore is acquired,
>    we gracefully release all held resources and return 0 (VFS retry) to
>    let the VFS safely retry the write from scratch.
> 
> The series compiles clean against the latest linux-next/ext4 tree.

This caught my eye: Do you actually do real testing of your patches? Like
using fstests / kvm-xfstests? Every patch author for ext4 is supposed to do
that before submitting his changes...

								Honza

> Aditya Prakash Srivastava (2):
>   ext4: use fsdata to track inline data write state
>   ext4: replace BUG_ON with graceful retry in ext4_write_inline_data_end
> 
>  fs/ext4/ext4.h   |  1 +
>  fs/ext4/inline.c | 14 +++++++++++++-
>  fs/ext4/inode.c  | 22 +++++++++++++---------
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
> -- 
> 2.47.3
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR