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

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

[Jan Kara]

Hello,

here is a second version of patches to fix a race between conversion of UDF
file from inline format to the standard format and writeback resulting in
possible memory corruption. Review/Acks are welcome in particular for the
export of folio_prepare_writeback(). I'd like to push the fix to Linus soon as
it's kind of nasty issue.

Changes since v1:
* Fix crash in udf_writepages() when file had no folio in the mapping


								Honza
Previous versions:
Link: http://lore.kernel.org/r/20260323162617.2421-1-jack@suse.cz # v1

[PATCH v2 1/2] writeback: Export folio_prepare_writeback()

[Jan Kara]

The sequence of operations done in folio_prepare_writeback() is needed
practically by every writeback method. UDF is unable to easily use
writeback_iter() for inodes with inline format (due to complexities with
racing conversion and fallback) so export folio_prepare_writeback() for
it instead of opencoding it in UDF. Later we can use this helper from
other filesystems as well.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/writeback.h | 2 ++
 mm/page-writeback.c       | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index e530112c4b3a..3e5d215c8d27 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -357,6 +357,8 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb);
 
 struct folio *writeback_iter(struct address_space *mapping,
 		struct writeback_control *wbc, struct folio *folio, int *error);
+bool folio_prepare_writeback(struct address_space *mapping,
+		struct writeback_control *wbc, struct folio *folio);
 
 int do_writepages(struct address_space *mapping, struct writeback_control *wbc);
 void writeback_set_ratelimit(void);
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 601a5e048d12..ceb78798fc00 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2358,7 +2358,7 @@ void tag_pages_for_writeback(struct address_space *mapping,
 }
 EXPORT_SYMBOL(tag_pages_for_writeback);
 
-static bool folio_prepare_writeback(struct address_space *mapping,
+bool folio_prepare_writeback(struct address_space *mapping,
 		struct writeback_control *wbc, struct folio *folio)
 {
 	/*
@@ -2389,7 +2389,7 @@ static bool folio_prepare_writeback(struct address_space *mapping,
 
 	return true;
 }
-
+EXPORT_SYMBOL_GPL(folio_prepare_writeback);
 
 static pgoff_t wbc_end(struct writeback_control *wbc)
 {
-- 
2.51.0

[PATCH v2 2/2] 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 | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 7fae8002344a..a659cd7d7ec0 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -181,34 +181,38 @@ 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;
+
+		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_prepare_writeback(mapping, wbc, folio)) {
+			folio_unlock(folio);
+			return 0;
+		}
 		memcpy_from_file_folio(iinfo->i_data + iinfo->i_lenEAttr, folio,
 				0, i_size_read(inode));
 		folio_unlock(folio);
+		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 2/2] udf: Fix race between file type conversion and writeback

[Christoph Hellwig]

The race is real, but I don't like exporting the writeback internals
for it.

It seems like the somewhat cleaner version would be to have a version
of mpage_writepages that takes and extra callback called in the
writeback_iter() loop and allows bypassing mpage_write_folio when
say returning a positive value?

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

[Jan Kara]

On Tue 24-03-26 23:02:08, Christoph Hellwig wrote:
> The race is real, but I don't like exporting the writeback internals
> for it.
> 
> It seems like the somewhat cleaner version would be to have a version
> of mpage_writepages that takes and extra callback called in the
> writeback_iter() loop and allows bypassing mpage_write_folio when
> say returning a positive value?

With a single user I don't think that's really worth it. I think it's
easier to just opencode that small function in UDF. It won't be the first
filesystem doing the "is it dirty, wait for writeback, clear dirty bit"
dance on its own. Thanks for feedback!

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