[PATCH v2] ext4: fix race in ext4_convert_inline_data() leading to BUG_ON in writepages

[Yun Zhou]

ext4_convert_inline_data() has two unsafe lockless fast paths:

1. if (!ext4_has_inline_data(inode)) {
       ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
       return 0;
   }

2. if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
       filemap_flush(inode->i_mapping);
       if (!ext4_has_inline_data(inode))
           return 0;
   }

Both paths can observe a transient state from a concurrent
ext4_convert_inline_data_nolock() that has called
ext4_destroy_inline_data_nolock() (clearing both the inline data inode
flag and MAY_INLINE_DATA) but then fails in ext4_map_blocks() and
restores inline data via ext4_restore_inline_data() (re-setting both
flags).

A racing thread in ext4_page_mkwrite() sees !has_inline_data and
!MAY_INLINE_DATA (the transient destroy-before-restore state), concludes
that no conversion is needed, and proceeds to block_page_mkwrite()
which creates dirty pages.  Meanwhile the first thread restores inline
data and MAY_INLINE_DATA.  This leaves dirty pages coexisting with
inline data + MAY_INLINE_DATA, triggering the BUG_ON in
ext4_do_writepages().

A similar race exists with ext4_da_convert_inline_data_to_extent():
it copies inline data to page cache and clears MAY_INLINE_DATA under
a read lock.  After releasing the lock, a concurrent
ext4_convert_inline_data_nolock() can restore MAY_INLINE_DATA, again
leaving dirty pages with MAY_INLINE_DATA set.

Fix this by:
1. Removing both unsafe lockless fast paths entirely.  For filesystems
   without the inline_data feature, return 0 immediately.  For all
   other cases, take xattr_sem write lock to check state.
2. Only calling ext4_convert_inline_data_nolock() when both
   has_inline_data AND MAY_INLINE_DATA are set.  When has_inline_data
   is set but MAY_INLINE_DATA is clear, a DA conversion is already in
   progress -- just return 0 and let writeback finish the job.  This
   prevents destroy+restore from racing with the DA convert path.
3. Clearing MAY_INLINE_DATA inside the lock only when has_inline_data
   is confirmed false.

Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
Reported-by: syzbot+d1da16f03614058fdc48@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=d1da16f03614058fdc48
Signed-off-by: Yun Zhou <yun.zhou@xxxxxxxxxxxxx>
---
v2:
- Reworked fix: root cause confirmed via ftrace.
- Removed both unsafe lockless fast paths, serialize under xattr_sem.
- Skip convert_nolock when MAY_INLINE_DATA is clear (DA convert in
  progress) to prevent destroy+restore from re-setting MAY.

 fs/ext4/inline.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 8045e4ff270c..87fee117dcc1 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1959,22 +1959,8 @@ int ext4_convert_inline_data(struct inode *inode)
 	handle_t *handle;
 	struct ext4_iloc iloc;
 
-	if (!ext4_has_inline_data(inode)) {
-		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
+	if (!ext4_has_feature_inline_data(inode->i_sb))
 		return 0;
-	} else if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
-		/*
-		 * Inode has inline data but EXT4_STATE_MAY_INLINE_DATA is
-		 * cleared. This means we are in the middle of moving of
-		 * inline data to delay allocated block. Just force writeout
-		 * here to finish conversion.
-		 */
-		error = filemap_flush(inode->i_mapping);
-		if (error)
-			return error;
-		if (!ext4_has_inline_data(inode))
-			return 0;
-	}
 
 	needed_blocks = ext4_chunk_trans_extent(inode, 1);
 
@@ -1990,8 +1976,22 @@ int ext4_convert_inline_data(struct inode *inode)
 	}
 
 	ext4_write_lock_xattr(inode, &no_expand);
-	if (ext4_has_inline_data(inode))
+	if (ext4_has_inline_data(inode) &&
+	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
 		error = ext4_convert_inline_data_nolock(handle, inode, &iloc);
+	/*
+	 * If has_inline_data is set but MAY_INLINE_DATA is clear, a
+	 * concurrent ext4_da_convert_inline_data_to_extent() has already
+	 * copied the data to page cache and will finish conversion via
+	 * writeback -- skip convert_nolock to avoid destroy+restore
+	 * re-setting MAY_INLINE_DATA behind its back.
+	 *
+	 * Clear MAY_INLINE_DATA if inline data is gone (convert succeeded
+	 * or was already completed).  Do not clear it if convert failed
+	 * and inline data was restored.
+	 */
+	if (!ext4_has_inline_data(inode))
+		ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
 	ext4_write_unlock_xattr(inode, &no_expand);
 	ext4_journal_stop(handle);
 out_free:
-- 
2.43.0