[PATCH v2] udf: Fix race between file type conversion and writeback

[PATCH v2] udf: Fix race between file type conversion and writeback

[Jan Kara]

udf_setsize() can race with udf_writepages() as follows:

udf_setsize()			udf_writepages()
				  if (iinfo->i_alloc_type ==
						ICBTAG_FLAG_AD_IN_ICB)
  err = udf_expand_file_adinicb(inode);
  err = udf_extend_file(inode, newsize);
				    udf_adinicb_writepages()
				      memcpy_from_file_folio() - crash
					because inode size is too big.

Fix the problem by rechecking file type under folio lock in
udf_writepages() which properly serializes with
udf_expand_file_adinicb(). Since it is quite difficult to implement this
locking with current writeback_iter() logic, let's just opencode the
logic necessary to prepare (the only) folio the inode can have for
writeback.

Reported-by: Jianzhou Zhao <luckd0g@163.com>
Link: https://lore.kernel.org/all/f622c01.67ac.19cdbdd777d.Coremail.luckd0g@163.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 53 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

Changes since v1:
* opencode folio_prepare_writeback() in UDF instead of exporting it

I'll merge this fix through my tree.

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 7fae8002344a..c85b0239660c 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -181,34 +181,47 @@ static void udf_write_failed(struct address_space *mapping, loff_t to)
 	}
 }
 
-static int udf_adinicb_writepages(struct address_space *mapping,
-		      struct writeback_control *wbc)
+static int udf_writepages(struct address_space *mapping,
+			  struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;
 	struct udf_inode_info *iinfo = UDF_I(inode);
-	struct folio *folio = NULL;
-	int error = 0;
 
-	while ((folio = writeback_iter(mapping, wbc, folio, &error))) {
-		BUG_ON(!folio_test_locked(folio));
-		BUG_ON(folio->index != 0);
+	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
+		struct folio *folio;
+		bool dirty = false;
+
+		folio = filemap_lock_folio(mapping, 0);
+		if (IS_ERR(folio))
+			return 0;
+		/*
+		 * Recheck inode type under folio lock when we are protected
+		 * against udf_expand_file_adinicb(). Bail to standard writeback
+		 * path if file got expanded.
+		 */
+		if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
+			folio_unlock(folio);
+			goto mpage_writeback;
+		}
+		if (!folio_test_dirty(folio))
+			goto unlock_folio;
+		if (folio_test_writeback(folio)) {
+			if (wbc->sync_mode == WB_SYNC_NONE)
+				goto unlock_folio;
+			folio_wait_writeback(folio);
+		}
+		if (!folio_clear_dirty_for_io(folio))
+			goto unlock_folio;
 		memcpy_from_file_folio(iinfo->i_data + iinfo->i_lenEAttr, folio,
 				0, i_size_read(inode));
+		dirty = true;
+unlock_folio:
 		folio_unlock(folio);
+		if (dirty)
+			mark_inode_dirty(inode);
+		return 0;
 	}
-
-	mark_inode_dirty(inode);
-	return 0;
-}
-
-static int udf_writepages(struct address_space *mapping,
-			  struct writeback_control *wbc)
-{
-	struct inode *inode = mapping->host;
-	struct udf_inode_info *iinfo = UDF_I(inode);
-
-	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
-		return udf_adinicb_writepages(mapping, wbc);
+mpage_writeback:
 	return mpage_writepages(mapping, wbc, udf_get_block_wb);
 }
 
-- 
2.51.0

Re: [PATCH v2] udf: Fix race between file type conversion and writeback

[Christoph Hellwig]

On Wed, Mar 25, 2026 at 05:22:10PM +0100, Jan Kara wrote:
> udf_setsize() can race with udf_writepages() as follows:
> 
> udf_setsize()			udf_writepages()
> 				  if (iinfo->i_alloc_type ==
> 						ICBTAG_FLAG_AD_IN_ICB)
>   err = udf_expand_file_adinicb(inode);
>   err = udf_extend_file(inode, newsize);
> 				    udf_adinicb_writepages()
> 				      memcpy_from_file_folio() - crash
> 					because inode size is too big.
> 
> Fix the problem by rechecking file type under folio lock in
> udf_writepages() which properly serializes with
> udf_expand_file_adinicb(). Since it is quite difficult to implement this
> locking with current writeback_iter() logic, let's just opencode the
> logic necessary to prepare (the only) folio the inode can have for
> writeback.

Still not a fan of open coding this logic, which is even worse than
the previous export.  Inline data is a relatively common case in
various file systems, and we should be able to handle it in common
code.  So having a callback in mpage_writepages seems much better,
but if you detest that we could just export mpage_write_folio and
mpage_bio_submit_write (and make mpage_data public) instead.  I think
that's significantly worse than just adding the callback, but still
much better than open coding more writeback internals, something I
want to get away from.

Re: [PATCH v2] udf: Fix race between file type conversion and writeback

[Jan Kara]

On Wed 25-03-26 22:47:40, Christoph Hellwig wrote:
> On Wed, Mar 25, 2026 at 05:22:10PM +0100, Jan Kara wrote:
> > udf_setsize() can race with udf_writepages() as follows:
> > 
> > udf_setsize()			udf_writepages()
> > 				  if (iinfo->i_alloc_type ==
> > 						ICBTAG_FLAG_AD_IN_ICB)
> >   err = udf_expand_file_adinicb(inode);
> >   err = udf_extend_file(inode, newsize);
> > 				    udf_adinicb_writepages()
> > 				      memcpy_from_file_folio() - crash
> > 					because inode size is too big.
> > 
> > Fix the problem by rechecking file type under folio lock in
> > udf_writepages() which properly serializes with
> > udf_expand_file_adinicb(). Since it is quite difficult to implement this
> > locking with current writeback_iter() logic, let's just opencode the
> > logic necessary to prepare (the only) folio the inode can have for
> > writeback.
> 
> Still not a fan of open coding this logic, which is even worse than
> the previous export.  Inline data is a relatively common case in
> various file systems, and we should be able to handle it in common
> code.

I agree inline data is relatively common but as far as I was checking UDF
is the only filesystem using mpage_writepages() that is supporting inline
data (well, OCFS2 does as well but it doesn't bother with inline data
writeback - it handles only write(2) to files with inline data which modify
on disk content directly, they expand the file to extents in case of mmap).
So a special hook in mpage_writepages() for UDF looks like a bit of an
overkill to me.

> So having a callback in mpage_writepages seems much better,
> but if you detest that we could just export mpage_write_folio and
> mpage_bio_submit_write (and make mpage_data public) instead.  I think
> that's significantly worse than just adding the callback, but still
> much better than open coding more writeback internals, something I
> want to get away from.

I agree the callback is better than making all that mpage infrastructure
public. I still think exporting folio_prepare_writeback() would be less
burden than the callback but frankly I don't think it matters enough to
argue further. I just want to get that race fixed :).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR