Ocfs2 devel - Mar 2023 - [PATCH] ocfs2: Fix data corruption after failed write

When buffered write fails to copy data into underlying page cache page,
ocfs2_write_end_nolock() just zeroes out and dirties the page. This can
leave dirty page beyond EOF and if page writeback tries to write this
page before write succeeds and expands i_size, page gets into
inconsistent state where page dirty bit is clear but buffer dirty bits
stay set resulting in page data never getting written and so data copied
to the page is lost. Fix the problem by invalidating page beyond EOF
after failed write.

Fixes: 6dbf7bb55598 ("fs: Don't invalidate page buffers in
block_write_full_page()")
CC: stable at vger.kernel.org
Signed-off-by: Jan Kara <jack at suse.cz>
---
 fs/ocfs2/aops.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 1d65f6ef00ca..0394505fdce3 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1977,11 +1977,26 @@ int ocfs2_write_end_nolock(struct address_space
*mapping,
 	}
 
 	if (unlikely(copied < len) && wc->w_target_page) {
+		loff_t new_isize;
+
 		if (!PageUptodate(wc->w_target_page))
 			copied = 0;
 
-		ocfs2_zero_new_buffers(wc->w_target_page, start+copied,
-				       start+len);
+		new_isize = max_t(loff_t, i_size_read(inode), pos + copied);
+		if (new_isize > page_offset(wc->w_target_page))
+			ocfs2_zero_new_buffers(wc->w_target_page, start+copied,
+					       start+len);
+		else {
+			/*
+			 * When page is fully beyond new isize (data copy
+			 * failed), do not bother zeroing the page. Invalidate
+			 * it instead so that writeback does not get confused
+			 * put page & buffer dirty bits into inconsistent
+			 * state.
+			 */
+			block_invalidate_folio(page_folio(wc->w_target_page),
+						0, PAGE_SIZE);
+		}
 	}
 	if (wc->w_target_page)
 		flush_dcache_page(wc->w_target_page);
-- 
2.35.3

On 3/2/23 11:38 PM, Jan Kara wrote:
> When buffered write fails to copy data into underlying page cache page,
> ocfs2_write_end_nolock() just zeroes out and dirties the page. This can
> leave dirty page beyond EOF and if page writeback tries to write this
> page before write succeeds and expands i_size, page gets into
> inconsistent state where page dirty bit is clear but buffer dirty bits
> stay set resulting in page data never getting written and so data copied
> to the page is lost. Fix the problem by invalidating page beyond EOF
> after failed write.
> 
> Fixes: 6dbf7bb55598 ("fs: Don't invalidate page buffers in
block_write_full_page()")
> CC: stable at vger.kernel.org
> Signed-off-by: Jan Kara <jack at suse.cz>
Looks good.
Reviewed-by: Joseph Qi <joseph.qi at linux.alibaba.com>
> ---
>  fs/ocfs2/aops.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 1d65f6ef00ca..0394505fdce3 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1977,11 +1977,26 @@ int ocfs2_write_end_nolock(struct address_space
*mapping,
>  	}
>  
>  	if (unlikely(copied < len) && wc->w_target_page) {
> +		loff_t new_isize;
> +
>  		if (!PageUptodate(wc->w_target_page))
>  			copied = 0;
>  
> -		ocfs2_zero_new_buffers(wc->w_target_page, start+copied,
> -				       start+len);
> +		new_isize = max_t(loff_t, i_size_read(inode), pos + copied);
> +		if (new_isize > page_offset(wc->w_target_page))
> +			ocfs2_zero_new_buffers(wc->w_target_page, start+copied,
> +					       start+len);
> +		else {
> +			/*
> +			 * When page is fully beyond new isize (data copy
> +			 * failed), do not bother zeroing the page. Invalidate
> +			 * it instead so that writeback does not get confused
> +			 * put page & buffer dirty bits into inconsistent
> +			 * state.
> +			 */
> +			block_invalidate_folio(page_folio(wc->w_target_page),
> +						0, PAGE_SIZE);
> +		}
>  	}
>  	if (wc->w_target_page)
>  		flush_dcache_page(wc->w_target_page);