[Ocfs2-devel] [PATCH 0/2] ocfs2: Truncate data corruption fix

[Ocfs2-devel] [PATCH 0/2] ocfs2: Truncate data corruption fix

[Jan Kara]

Hello,

as further testing has shown, the commit 5314454ea3f ("ocfs2: fix data
corruption after conversion from inline format") didn't fix all the data
corruption issues the customer started observing after 6dbf7bb55598 ("fs: Don't
invalidate page buffers in block_write_full_page()"). This time I have tracked
them down to two bugs in ocfs2 truncation code. One bug (truncating page cache
before clearing tail cluster and setting i_size) could cause data corruption
even before 6dbf7bb55598, but before that commit it needed a race with page
fault, after 6dbf7bb55598 it started to be pretty deterministic. Another
bug (zeroing pages beyond old i_size) used to be harmless inefficiency before
commit 6dbf7bb55598. But after commit 6dbf7bb55598 in combination with the
first bug it resulted in deterministic data corruption. Although fixing only
the first problem is needed to stop data corruption, I've fixed both issues
to make the code more robust.

								Honza

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Jan Kara]

ocfs2_truncate_file() did unmap invalidate page cache pages before
zeroing partial tail cluster and setting i_size. Thus some pages could
be left (and likely have left if the cluster zeroing happened) in the
page cache beyond i_size after truncate finished letting user possibly
see stale data once the file was extended again. Also the tail cluster
zeroing was not guaranteed to finish before truncate finished causing
possible stale data exposure. The problem started to be particularly
easy to hit after commit 6dbf7bb55598 "fs: Don't invalidate page buffers
in block_write_full_page()" stopped invalidation of pages beyond i_size
from page writeback path.

Fix these problems by unmapping and invalidating pages in the page cache
after the i_size is reduced and tail cluster is zeroed out.

CC: stable@vger.kernel.org
Fixes: ccd979bdbce9 ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 54d7843c0211..fc5f780fa235 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -476,10 +476,11 @@ int ocfs2_truncate_file(struct inode *inode,
 	 * greater than page size, so we have to truncate them
 	 * anyway.
 	 */
-	unmap_mapping_range(inode->i_mapping, new_i_size + PAGE_SIZE - 1, 0, 1);
-	truncate_inode_pages(inode->i_mapping, new_i_size);
 
 	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
+		unmap_mapping_range(inode->i_mapping,
+				    new_i_size + PAGE_SIZE - 1, 0, 1);
+		truncate_inode_pages(inode->i_mapping, new_i_size);
 		status = ocfs2_truncate_inline(inode, di_bh, new_i_size,
 					       i_size_read(inode), 1);
 		if (status)
@@ -498,6 +499,9 @@ int ocfs2_truncate_file(struct inode *inode,
 		goto bail_unlock_sem;
 	}
 
+	unmap_mapping_range(inode->i_mapping, new_i_size + PAGE_SIZE - 1, 0, 1);
+	truncate_inode_pages(inode->i_mapping, new_i_size);
+
 	status = ocfs2_commit_truncate(osb, inode, di_bh);
 	if (status < 0) {
 		mlog_errno(status);
-- 
2.26.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 2/2] ocfs2: Do not zero pages beyond i_size

[Jan Kara]

ocfs2_zero_range_for_truncate() can try to zero pages beyond current
inode size despite the fact that underlying blocks should be already
zeroed out and writeback will skip writing such pages anyway. Avoid the
pointless work.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/alloc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 5d9ae17bd443..ff5ff0e7de44 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -6921,13 +6921,12 @@ static int ocfs2_grab_eof_pages(struct inode *inode, loff_t start, loff_t end,
 }
 
 /*
- * Zero the area past i_size but still within an allocated
- * cluster. This avoids exposing nonzero data on subsequent file
- * extends.
+ * Zero partial cluster for a hole punch or truncate. This avoids exposing
+ * nonzero data on subsequent file extends.
  *
  * We need to call this before i_size is updated on the inode because
  * otherwise block_write_full_page() will skip writeout of pages past
- * i_size. The new_i_size parameter is passed for this reason.
+ * i_size.
  */
 int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
 				  u64 range_start, u64 range_end)
@@ -6945,6 +6944,15 @@ int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
 	if (!ocfs2_sparse_alloc(OCFS2_SB(sb)))
 		return 0;
 
+	/*
+	 * Avoid zeroing pages fully beyond current i_size. It is pointless as
+	 * underlying blocks of those pages should be already zeroed out and
+	 * page writeback will skip them anyway.
+	 */
+	range_end = min_t(u64, range_end, i_size_read(inode));
+	if (range_start >= range_end)
+		return 0;
+
 	pages = kcalloc(ocfs2_pages_per_cluster(sb),
 			sizeof(struct page *), GFP_NOFS);
 	if (pages == NULL) {
@@ -6953,9 +6961,6 @@ int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
 		goto out;
 	}
 
-	if (range_start == range_end)
-		goto out;
-
 	ret = ocfs2_extent_map_get_blocks(inode,
 					  range_start >> sb->s_blocksize_bits,
 					  &phys, NULL, &ext_flags);
-- 
2.26.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Joseph Qi]

Hi Jan,

On 10/25/21 11:13 PM, Jan Kara wrote:
> ocfs2_truncate_file() did unmap invalidate page cache pages before
> zeroing partial tail cluster and setting i_size. Thus some pages could
> be left (and likely have left if the cluster zeroing happened) in the
> page cache beyond i_size after truncate finished letting user possibly
> see stale data once the file was extended again. Also the tail cluster

I don't quite understand the case. 
truncate_inode_pages() will truncate pages from new_i_size to i_size,
and the following ocfs2_orphan_for_truncate() will zero range and then
update i_size for inode as well as dinode.
So once truncate finished, how stale data exposing happens? Or do you
mean a race case between the above two steps?

Thanks,
Joseph 

> zeroing was not guaranteed to finish before truncate finished causing
> possible stale data exposure. The problem started to be particularly
> easy to hit after commit 6dbf7bb55598 "fs: Don't invalidate page buffers
> in block_write_full_page()" stopped invalidation of pages beyond i_size
> from page writeback path.
> 
> Fix these problems by unmapping and invalidating pages in the page cache
> after the i_size is reduced and tail cluster is zeroed out.

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Joseph Qi]

On 10/28/21 3:09 PM, Joseph Qi wrote:
> Hi Jan,
> 
> On 10/25/21 11:13 PM, Jan Kara wrote:
>> ocfs2_truncate_file() did unmap invalidate page cache pages before
>> zeroing partial tail cluster and setting i_size. Thus some pages could
>> be left (and likely have left if the cluster zeroing happened) in the
>> page cache beyond i_size after truncate finished letting user possibly
>> see stale data once the file was extended again. Also the tail cluster
> 
> I don't quite understand the case. 
> truncate_inode_pages() will truncate pages from new_i_size to i_size,
> and the following ocfs2_orphan_for_truncate() will zero range and then
> update i_size for inode as well as dinode.
> So once truncate finished, how stale data exposing happens? Or do you
> mean a race case between the above two steps?
> 
Or do you mean ocfs2_zero_range_for_truncate() will grab and zero eof
pages? Though it depends on block_write_full_page() to write out, the
pages are zeroed now. Still a little confused...

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Jan Kara]

On Thu 28-10-21 15:09:08, Joseph Qi wrote:
> Hi Jan,
> 
> On 10/25/21 11:13 PM, Jan Kara wrote:
> > ocfs2_truncate_file() did unmap invalidate page cache pages before
> > zeroing partial tail cluster and setting i_size. Thus some pages could
> > be left (and likely have left if the cluster zeroing happened) in the
> > page cache beyond i_size after truncate finished letting user possibly
> > see stale data once the file was extended again. Also the tail cluster
> 
> I don't quite understand the case. 
> truncate_inode_pages() will truncate pages from new_i_size to i_size,
> and the following ocfs2_orphan_for_truncate() will zero range and then
> update i_size for inode as well as dinode.
> So once truncate finished, how stale data exposing happens? Or do you
> mean a race case between the above two steps?

Sorry, I was not quite accurate in the above paragraph. There are several
ways how stale pages in the pagecache can cause problems.

1) Because i_size is reduced after truncating page cache, page fault can
happen after truncating page cache and zeroing pages but before reducing i_size.
This will in allow user to arbitrarily modify pages that are used for
writing zeroes into the cluster tail and after file extension these data
will become visible.

2) The tail cluster zeroing in ocfs2_orphan_for_truncate() can actually try
to write zeroed pages above i_size (e.g. if we have 4k blocksize, 64k
clustersize, and do truncate(f, 4k) on a 4k file). This will cause exactly
same problems as already described in commit 5314454ea3f "ocfs2: fix data
corruption after conversion from inline format".

Hope it is clearer now.

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

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Joseph Qi]

On 11/1/21 7:31 PM, Jan Kara wrote:
> On Thu 28-10-21 15:09:08, Joseph Qi wrote:
>> Hi Jan,
>>
>> On 10/25/21 11:13 PM, Jan Kara wrote:
>>> ocfs2_truncate_file() did unmap invalidate page cache pages before
>>> zeroing partial tail cluster and setting i_size. Thus some pages could
>>> be left (and likely have left if the cluster zeroing happened) in the
>>> page cache beyond i_size after truncate finished letting user possibly
>>> see stale data once the file was extended again. Also the tail cluster
>>
>> I don't quite understand the case. 
>> truncate_inode_pages() will truncate pages from new_i_size to i_size,
>> and the following ocfs2_orphan_for_truncate() will zero range and then
>> update i_size for inode as well as dinode.
>> So once truncate finished, how stale data exposing happens? Or do you
>> mean a race case between the above two steps?
> 
> Sorry, I was not quite accurate in the above paragraph. There are several
> ways how stale pages in the pagecache can cause problems.
> 
> 1) Because i_size is reduced after truncating page cache, page fault can
> happen after truncating page cache and zeroing pages but before reducing i_size.
> This will in allow user to arbitrarily modify pages that are used for
> writing zeroes into the cluster tail and after file extension these data
> will become visible.
> 
> 2) The tail cluster zeroing in ocfs2_orphan_for_truncate() can actually try
> to write zeroed pages above i_size (e.g. if we have 4k blocksize, 64k
> clustersize, and do truncate(f, 4k) on a 4k file). This will cause exactly
> same problems as already described in commit 5314454ea3f "ocfs2: fix data
> corruption after conversion from inline format".
> 
> Hope it is clearer now.
> 
So the core reason is ocfs2_zero_range_for_truncate() grabs pages and
then zero, right?
I think an alternative way is using zeroout instead of zero pages, which
won't grab pages again.
Anyway, I'm also fine with your way since it is simple.

Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>



_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 2/2] ocfs2: Do not zero pages beyond i_size

[Joseph Qi]

On 10/25/21 11:13 PM, Jan Kara wrote:
> ocfs2_zero_range_for_truncate() can try to zero pages beyond current
> inode size despite the fact that underlying blocks should be already
> zeroed out and writeback will skip writing such pages anyway. Avoid the
> pointless work.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

> ---
>  fs/ocfs2/alloc.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 5d9ae17bd443..ff5ff0e7de44 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -6921,13 +6921,12 @@ static int ocfs2_grab_eof_pages(struct inode *inode, loff_t start, loff_t end,
>  }
>  
>  /*
> - * Zero the area past i_size but still within an allocated
> - * cluster. This avoids exposing nonzero data on subsequent file
> - * extends.
> + * Zero partial cluster for a hole punch or truncate. This avoids exposing
> + * nonzero data on subsequent file extends.
>   *
>   * We need to call this before i_size is updated on the inode because
>   * otherwise block_write_full_page() will skip writeout of pages past
> - * i_size. The new_i_size parameter is passed for this reason.
> + * i_size.
>   */
>  int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
>  				  u64 range_start, u64 range_end)
> @@ -6945,6 +6944,15 @@ int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
>  	if (!ocfs2_sparse_alloc(OCFS2_SB(sb)))
>  		return 0;
>  
> +	/*
> +	 * Avoid zeroing pages fully beyond current i_size. It is pointless as
> +	 * underlying blocks of those pages should be already zeroed out and
> +	 * page writeback will skip them anyway.
> +	 */
> +	range_end = min_t(u64, range_end, i_size_read(inode));
> +	if (range_start >= range_end)
> +		return 0;
> +
>  	pages = kcalloc(ocfs2_pages_per_cluster(sb),
>  			sizeof(struct page *), GFP_NOFS);
>  	if (pages == NULL) {
> @@ -6953,9 +6961,6 @@ int ocfs2_zero_range_for_truncate(struct inode *inode, handle_t *handle,
>  		goto out;
>  	}
>  
> -	if (range_start == range_end)
> -		goto out;
> -
>  	ret = ocfs2_extent_map_get_blocks(inode,
>  					  range_start >> sb->s_blocksize_bits,
>  					  &phys, NULL, &ext_flags);
> 

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Re: [Ocfs2-devel] [PATCH 1/2] ocfs2: Fix data corruption on truncate

[Jan Kara]

On Tue 02-11-21 10:36:42, Joseph Qi wrote:
> On 11/1/21 7:31 PM, Jan Kara wrote:
> > On Thu 28-10-21 15:09:08, Joseph Qi wrote:
> >> Hi Jan,
> >>
> >> On 10/25/21 11:13 PM, Jan Kara wrote:
> >>> ocfs2_truncate_file() did unmap invalidate page cache pages before
> >>> zeroing partial tail cluster and setting i_size. Thus some pages could
> >>> be left (and likely have left if the cluster zeroing happened) in the
> >>> page cache beyond i_size after truncate finished letting user possibly
> >>> see stale data once the file was extended again. Also the tail cluster
> >>
> >> I don't quite understand the case. 
> >> truncate_inode_pages() will truncate pages from new_i_size to i_size,
> >> and the following ocfs2_orphan_for_truncate() will zero range and then
> >> update i_size for inode as well as dinode.
> >> So once truncate finished, how stale data exposing happens? Or do you
> >> mean a race case between the above two steps?
> > 
> > Sorry, I was not quite accurate in the above paragraph. There are several
> > ways how stale pages in the pagecache can cause problems.
> > 
> > 1) Because i_size is reduced after truncating page cache, page fault can
> > happen after truncating page cache and zeroing pages but before reducing i_size.
> > This will in allow user to arbitrarily modify pages that are used for
> > writing zeroes into the cluster tail and after file extension these data
> > will become visible.
> > 
> > 2) The tail cluster zeroing in ocfs2_orphan_for_truncate() can actually try
> > to write zeroed pages above i_size (e.g. if we have 4k blocksize, 64k
> > clustersize, and do truncate(f, 4k) on a 4k file). This will cause exactly
> > same problems as already described in commit 5314454ea3f "ocfs2: fix data
> > corruption after conversion from inline format".
> > 
> > Hope it is clearer now.
> > 
> So the core reason is ocfs2_zero_range_for_truncate() grabs pages and
> then zero, right?

Well, that is the part that makes things easy to reproduce.

> I think an alternative way is using zeroout instead of zero pages, which
> won't grab pages again.

That would certainly reduce the likelyhood of problems but it is always
problematic to first truncate page cache and only then update i_size.
For OCFS2 racing page faults can recreate pages in the page cache before
i_size is reduced and thus cause "interesting" problems.

> Anyway, I'm also fine with your way since it is simple.
> 
> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Thanks!

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

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel