ext4: Fix stale data read exposure problem with DIO read/page_mkwrite

Some filesystems (e.g. ext4) need to know in it's writeback path, that whether DIO is in progress or not. This info may be needed to avoid the stale data exposure race with DIO reads.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/iomap/direct-io.c | 17 +++++++++++++----

 1 file changed, 13 insertions(+), 4 deletions(-)

fs/iomap/direct-io.c +13 -4

index 23837926c0c5..d1c159bd3854 100644
@@ -468,9 +468,18 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		flags |= IOMAP_NOWAIT;
 	}
 
+	/*
+	 * Call inode_dio_begin() before we write out and wait for writeback to
+	 * complete. This may be needed by some filesystems to prevent race
+	 * like stale data exposure by DIO reads.
+	 */
+	inode_dio_begin(inode);
+	/* So that i_dio_count is incremented before below operation */
+	smp_mb__after_atomic();
+
 	ret = filemap_write_and_wait_range(mapping, pos, end);
 	if (ret)
-		goto out_free_dio;
+		goto out_end_dio;
 
 	/*
 	 * Try to invalidate cache pages for the range we're direct

@@ -488,11 +497,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	    !inode->i_sb->s_dio_done_wq) {
 		ret = sb_init_dio_done_wq(inode->i_sb);
 		if (ret < 0)
-			goto out_free_dio;
+			goto out_end_dio;
 	}
 
-	inode_dio_begin(inode);
-
 	blk_start_plug(&plug);
 	do {
 		ret = iomap_apply(inode, pos, count, flags, ops, dio,

@@ -568,6 +575,8 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 
 	return iomap_dio_complete(dio);
 
+out_end_dio:
+	inode_dio_end(inode);
 out_free_dio:
 	kfree(dio);
 	return ret;

Signature · 1 line

2.21.0

Quoted 3 lines · Ritesh Harjani

Some filesystems (e.g. ext4) need to know in it's writeback path, that whether DIO is in progress or not. This info may be needed to avoid the stale data exposure race with DIO reads.

Does XFS have this problem too?

Admittedly dio read during mmap write is probably not well supported. ;)

Well, XFS always performs buffered writeback using unwritten extents so at least the immediate problem of stale data exposure ext4 has does not happen there AFAICT.

Quoted 4 lines · Jan Kara

Well, XFS always performs buffered writeback using unwritten extents so at least the immediate problem of stale data exposure ext4 has does not happen there AFAICT.

Currently XFS never uses unwritten extents when converting delalloc extents.

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · Jan Kara

Well, XFS always performs buffered writeback using unwritten extents so at least the immediate problem of stale data exposure ext4 has does not happen there AFAICT.

Quoted 2 lines

Currently XFS never uses unwritten extents when converting delalloc extents.

I see, it is a long time since I last looked at that part of XFS code. So then I think XFS might be prone to the same kind of race and data exposure as I outlined in [1]...

Some filesystems (e.g. ext4) need to know in it's writeback path, that whether DIO is in progress or not. This info may be needed to avoid the stale data exposure race with DIO reads.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/iomap/direct-io.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/iomap/direct-io.c +0 -0

index 23837926c0c5..d1c159bd3854 100644
@@ -468,9 +468,18 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		flags |= IOMAP_NOWAIT;

Using i_dio_count for any kind of detection is bogus. If you want to pass flags to the writeback code please do so explicitly through struct writeback_control.

We want to detect in the writeback path whether there's direct IO (read) currently running for the inode. Not for the writeback issued from iomap_dio_rw() but for any arbitrary writeback that iomap_dio_rw() can be racing with - so struct writeback_control won't help. Now if you want to see the ugly details why this hack is needed, see my other email to Ritesh in this thread with details of the race.

Quoted 6 lines · Jan Kara

We want to detect in the writeback path whether there's direct IO (read) currently running for the inode. Not for the writeback issued from iomap_dio_rw() but for any arbitrary writeback that iomap_dio_rw() can be racing with - so struct writeback_control won't help. Now if you want to see the ugly details why this hack is needed, see my other email to Ritesh in this thread with details of the race.

How do we get other writeback after iomap_dio_rw wrote everything out?

Either way I'm trying to kill i_dio_count as it has all kinds of problems, see the patch sent out earlier today.

Currently there is a small race window where ext4 tries to allocate a written block for mapped files and if DIO read is in progress, then this may result into stale data read exposure problem.

This patch fixes the mentioned issue by:
1. For non-delalloc path, page_mkwrite will use unwritten blocks by default for extent based files.

2. For delalloc path, we check if DIO is in progress during writeback. If yes, then we use unwritten blocks method to avoid this race.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/inode.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

fs/ext4/inode.c +5 -11

index d035acab5b2a..07f66782335b 100644
@@ -1529,6 +1529,7 @@ struct mpage_da_data {
 	struct ext4_map_blocks map;
 	struct ext4_io_submit io_submit;	/* IO submission data */
 	unsigned int do_map:1;
+	bool dio_in_progress:1;
 };
 
 static void mpage_release_unused_pages(struct mpage_da_data *mpd,

@@ -2359,7 +2360,7 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 			   EXT4_GET_BLOCKS_METADATA_NOFAIL |
 			   EXT4_GET_BLOCKS_IO_SUBMIT;
 	dioread_nolock = ext4_should_dioread_nolock(inode);
-	if (dioread_nolock)
+	if (dioread_nolock || mpd->dio_in_progress)
 		get_blocks_flags |= EXT4_GET_BLOCKS_IO_CREATE_EXT;
 	if (map->m_flags & (1 << BH_Delay))
 		get_blocks_flags |= EXT4_GET_BLOCKS_DELALLOC_RESERVE;

@@ -2367,7 +2368,8 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 	err = ext4_map_blocks(handle, inode, map, get_blocks_flags);
 	if (err < 0)
 		return err;
-	if (dioread_nolock && (map->m_flags & EXT4_MAP_UNWRITTEN)) {
+	if ((dioread_nolock || mpd->dio_in_progress) &&
+	    (map->m_flags & EXT4_MAP_UNWRITTEN)) {
 		if (!mpd->io_submit.io_end->handle &&
 		    ext4_handle_valid(handle)) {
 			mpd->io_submit.io_end->handle = handle->h_rsv_handle;

@@ -2626,6 +2628,7 @@ static int ext4_writepages(struct address_space *mapping,
 	bool done;
 	struct blk_plug plug;
 	bool give_up_on_write = false;
+	bool dio_in_progress = false;
 
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
 		return -EIO;

@@ -2680,15 +2683,6 @@ static int ext4_writepages(struct address_space *mapping,
 		ext4_journal_stop(handle);
 	}
 
-	if (ext4_should_dioread_nolock(inode)) {
-		/*
-		 * We may need to convert up to one extent per block in
-		 * the page and we may dirty the inode.
-		 */
-		rsv_blocks = 1 + ext4_chunk_trans_blocks(inode,
-						PAGE_SIZE >> inode->i_blkbits);
-	}
-
 	if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
 		range_whole = 1;
 

@@ -2712,6 +2706,26 @@ static int ext4_writepages(struct address_space *mapping,
 	done = false;
 	blk_start_plug(&plug);
 

Quoted 75 lines · Ritesh Harjani

Currently there is a small race window where ext4 tries to allocate a written block for mapped files and if DIO read is in progress, then this may result into stale data read exposure problem.

This patch fixes the mentioned issue by:
1. For non-delalloc path, page_mkwrite will use unwritten blocks by default for extent based files.

2. For delalloc path, we check if DIO is in progress during writeback. If yes, then we use unwritten blocks method to avoid this race.

Signed-off-by Ritesh Harjani <[email protected]>

---
  fs/ext4/inode.c | 45 ++++++++++++++++++++++++++++++++-------------
  1 file changed, 32 insertions(+), 13 deletions(-)

fs/ext4/inode.c +5 -11

index d035acab5b2a..07f66782335b 100644
@@ -1529,6 +1529,7 @@ struct mpage_da_data {
  	struct ext4_map_blocks map;
  	struct ext4_io_submit io_submit;	/* IO submission data */
  	unsigned int do_map:1;
+	bool dio_in_progress:1;
  };
 
  static void mpage_release_unused_pages(struct mpage_da_data *mpd,

@@ -2359,7 +2360,7 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
  			   EXT4_GET_BLOCKS_METADATA_NOFAIL |
  			   EXT4_GET_BLOCKS_IO_SUBMIT;
  	dioread_nolock = ext4_should_dioread_nolock(inode);
-	if (dioread_nolock)
+	if (dioread_nolock || mpd->dio_in_progress)
  		get_blocks_flags |= EXT4_GET_BLOCKS_IO_CREATE_EXT;
  	if (map->m_flags & (1 << BH_Delay))
  		get_blocks_flags |= EXT4_GET_BLOCKS_DELALLOC_RESERVE;

@@ -2367,7 +2368,8 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
  	err = ext4_map_blocks(handle, inode, map, get_blocks_flags);
  	if (err < 0)
  		return err;
-	if (dioread_nolock && (map->m_flags & EXT4_MAP_UNWRITTEN)) {
+	if ((dioread_nolock || mpd->dio_in_progress) &&
+	    (map->m_flags & EXT4_MAP_UNWRITTEN)) {
  		if (!mpd->io_submit.io_end->handle &&
  		    ext4_handle_valid(handle)) {
  			mpd->io_submit.io_end->handle = handle->h_rsv_handle;

@@ -2626,6 +2628,7 @@ static int ext4_writepages(struct address_space *mapping,
  	bool done;
  	struct blk_plug plug;
  	bool give_up_on_write = false;
+	bool dio_in_progress = false;
 
  	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
  		return -EIO;

@@ -2680,15 +2683,6 @@ static int ext4_writepages(struct address_space *mapping,
  		ext4_journal_stop(handle);
  	}
 
-	if (ext4_should_dioread_nolock(inode)) {
-		/*
-		 * We may need to convert up to one extent per block in
-		 * the page and we may dirty the inode.
-		 */
-		rsv_blocks = 1 + ext4_chunk_trans_blocks(inode,
-						PAGE_SIZE >> inode->i_blkbits);
-	}
-
  	if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
  		range_whole = 1;
 

@@ -2712,6 +2706,26 @@ static int ext4_writepages(struct address_space *mapping,
  	done = false;
  	blk_start_plug(&plug);
 

fs/ext4/inode.c static int ext4_writepages(struct address_space *mapping, · line 2718

Quoted 10 lines

fs/ext4/inode.c +10 -0

+	/*
+	 * If DIO is in progress, then we use unwritten blocks for allocation.
+	 * This is to avoid a small window of race (stale read) with
+	 * ext4_page_mkwrite path in delalloc case & with DIO read in parallel.
+	 *
+	 * Let's check for i_dio_count after we have tagged pages for writeback.
+	 */
+	smp_mb__before_atomic();
+	dio_in_progress = !!atomic_read(&inode->i_dio_count);
+	mpd.dio_in_progress = dio_in_progress;

Two problems here:

1) smp_mb__before_atomic() does not work with atomic_read(). This kind of barrier works only with read-modify-write kinds of atomic operations like atomic_inc(). See Documentation/atomic_t.txt for more details.

2) Even if the barrier worked, this is still too early for the check. Consider the following race:

Task 1 - flusher Task 2 - dio read Task 3 - fault
ext4_writepages()
atomic_read(&inode->i_dio_count) -> 0
...

				iomap_dio_rw()
				  inode_dio_begin()
				  filemap_write_and_wait_range()
				  ...
							ext4_page_mkwrite()
							  fills hole at index I

...
mpage_prepare_extent_to_map()
finds dirty page at index I - tagging
not in effect because this is WB_SYNC_NONE writeback so we look for PAGECACHE_TAG_DIRTY mpage_map_and_submit_extent()
- allocates block for page I ext4_iomap_begin() finds block under offset I submit_bio()
- reads stale data

And what I wanted to use to stop this race is page lock / page writeback bit on page 'I' because filemap_write_and_wait_range() called from iomap_dio_rw() ends up waiting for both if the page is seen as dirty. For

this to work, you need to check inode->i_dio_count after you hold the page locks for written range - i.e., after mpage_prepare_extent_to_map(). And

that means you always have to have rsv_blocks set when starting a transaction because you don't know in advance whether you'll need them or not.

Currently there is a small race window as pointed out by Jan [2] where, when ext4 tries to allocate a written block for mapped files and if DIO read is in progress, then this may result into stale data read exposure problem.

Quoted 3 lines

Currently there is a small race window as pointed out by Jan [2] where, when ext4 tries to allocate a written block for mapped files and if DIO read is in progress, then this may result into stale data read exposure problem.

Do we have a test case for the problem?

Please add at very least the fsdevel and xfs lists to iomap patches.