[PATCH v3] ntfs: trim the sizes after a short write

[PATCH v3] ntfs: trim the sizes after a short write

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

ntfs_file_write_iter() moves the data size and, for a non-resident
uncompressed file, the initialized size to pos + count before the copy.
__ntfs_extend_initialized_size() only zeroes [old initialized size, pos),
so the range [pos, pos + count) is initialized only by the copy itself.
When iomap_file_buffered_write() copies less than count, it returns a
positive short count and the error rollback at out: does not run.  The
file keeps the size pos + count, and [pos + ret, pos + count) reads back
whatever the freed clusters held before, also after a remount.  A local
user can read the old contents of freed clusters this way.

Run the existing rollback for a short write as well, and roll the two
sizes back to the end of what was written instead of to the old sizes.
A short copy that ends inside a block can also leave the rest of that
block, and of the block holding pos + count, cached as read from disk,
so clear the page cache from the new initialized size to EOF before
shrinking it.

Fixes: b041ca562526 ("ntfs: update iomap and address space operations")
Cc: stable@vger.kernel.org
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
v3: clear the cache up to EOF, not up to the extended initialized
size, which left the rest of the block holding pos + count (Baolin).
v2: also clear the cached tail of the last block, which v1 left in
place when the write stayed inside the old data size (Baolin).

v2: https://lore.kernel.org/all/20261112030815.982026-1-zhanxusheng1024@gmail.com/
v1: https://lore.kernel.org/all/20261111093459.660484-1-zhanxusheng1024@gmail.com/

Tested in a VM on ntfs-next b025f526c5d0, with all free space filled
with a known pattern first.  Short buffered writes of 4M, old data
readable after drop_caches and after remount, 3 runs each:

                                    unfixed    v1     v2     v3
  fallocated, ends block-aligned    3137536    0      0      0
  fallocated, ends inside a block   3141332    212    0      0
  fallocated, unaligned start       3137536    412    0      0
  same, pos + count in that page    -          -      436    0
  append                            3137536    0      0      0

A short write 1M past EOF, O_DIRECT, resident and compressed files and
full writes give the same results with v3 as with v1.
---
 fs/ntfs/file.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index 3213a0c9f96b..aa58afd729d0 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -928,10 +928,21 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		ret = iomap_file_buffered_write(iocb, from, &ntfs_write_iomap_ops,
 				&ntfs_iomap_folio_ops, NULL);
 out:
-	if (ret < 0 && ret != -EIOCBQUEUED) {
+	/*
+	 * Both sizes were moved to end before the copy, so a short write,
+	 * like a failed one, must take back what it did not write.
+	 */
+	if (ret < count && ret != -EIOCBQUEUED) {
+		loff_t new_init_size = old_init_size;
+		loff_t new_data_size = old_data_size;
 		int init_err = 0;
 		int data_err = 0;
 
+		if (ret > 0) {
+			new_init_size = max(old_init_size, pos + ret);
+			new_data_size = max(old_data_size, pos + ret);
+		}
+
 		/*
 		 * Like ntfs_setattr_size(): __ntfs_attr_truncate_vfs()
 		 * expects the invalidate lock held across the size
@@ -940,13 +951,23 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		filemap_invalidate_lock(vi->i_mapping);
 
 		/* ntfs_attr_set_initialized_size() rejects a resident one. */
-		if (ni->initialized_size != old_init_size &&
+		if (ni->initialized_size != new_init_size &&
 		    NInoNonResident(ni)) {
 			struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
 
+			/*
+			 * A short copy can leave the rest of a block read
+			 * from disk in the cache, also past the temporarily
+			 * extended initialized size.  Clear everything that
+			 * is no longer initialized, as
+			 * __ntfs_extend_initialized_size() does.
+			 */
+			truncate_pagecache_range(vi, new_init_size,
+					max_t(loff_t, i_size_read(vi),
+					      ni->initialized_size) - 1);
 			mutex_lock(&mrec_ni->mrec_lock);
 			init_err = ntfs_attr_set_initialized_size(ni,
-					old_init_size);
+					new_init_size);
 			mutex_unlock(&mrec_ni->mrec_lock);
 		}
 		/*
@@ -955,13 +976,13 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		 * the uninitialized tail, so skip it unless the rollback
 		 * above restored that size.
 		 */
-		if (ni->data_size != old_data_size && !init_err) {
+		if (ni->data_size != new_data_size && !init_err) {
 			struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
 
-			truncate_setsize(vi, old_data_size);
+			truncate_setsize(vi, new_data_size);
 			mutex_lock(&mrec_ni->mrec_lock);
-			data_err = __ntfs_attr_truncate_vfs(ni, old_data_size,
-					old_data_size);
+			data_err = __ntfs_attr_truncate_vfs(ni, new_data_size,
+					new_data_size);
 			mutex_unlock(&mrec_ni->mrec_lock);
 			/* Like ntfs_setattr_size(): stay on the durable size. */
 			if (data_err)

base-commit: b025f526c5d04929212c9d5fb7ffc96608f3eacc
-- 
2.43.0

Re: [PATCH v3] ntfs: trim the sizes after a short write

[liubaolin]

在 2026/11/12 17:32, Zhan Xusheng 写道:
> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> ntfs_file_write_iter() moves the data size and, for a non-resident
> uncompressed file, the initialized size to pos + count before the copy.
> __ntfs_extend_initialized_size() only zeroes [old initialized size, pos),
> so the range [pos, pos + count) is initialized only by the copy itself.
> When iomap_file_buffered_write() copies less than count, it returns a
> positive short count and the error rollback at out: does not run.  The
> file keeps the size pos + count, and [pos + ret, pos + count) reads back
> whatever the freed clusters held before, also after a remount.  A local
> user can read the old contents of freed clusters this way.
> 
> Run the existing rollback for a short write as well, and roll the two
> sizes back to the end of what was written instead of to the old sizes.
> A short copy that ends inside a block can also leave the rest of that
> block, and of the block holding pos + count, cached as read from disk,
> so clear the page cache from the new initialized size to EOF before
> shrinking it.
> 
> Fixes: b041ca562526 ("ntfs: update iomap and address space operations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
> v3: clear the cache up to EOF, not up to the extended initialized
> size, which left the rest of the block holding pos + count (Baolin).
> v2: also clear the cached tail of the last block, which v1 left in
> place when the write stayed inside the old data size (Baolin).
> 
> v2: https://lore.kernel.org/all/20261112030815.982026-1-zhanxusheng1024@gmail.com/
> v1: https://lore.kernel.org/all/20261111093459.660484-1-zhanxusheng1024@gmail.com/
> 
> Tested in a VM on ntfs-next b025f526c5d0, with all free space filled
> with a known pattern first.  Short buffered writes of 4M, old data
> readable after drop_caches and after remount, 3 runs each:
> 
>                                      unfixed    v1     v2     v3
>    fallocated, ends block-aligned    3137536    0      0      0
>    fallocated, ends inside a block   3141332    212    0      0
>    fallocated, unaligned start       3137536    412    0      0
>    same, pos + count in that page    -          -      436    0
>    append                            3137536    0      0      0
> 
> A short write 1M past EOF, O_DIRECT, resident and compressed files and
> full writes give the same results with v3 as with v1.
> ---
>   fs/ntfs/file.c | 35 ++++++++++++++++++++++++++++-------
>   1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 3213a0c9f96b..aa58afd729d0 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -928,10 +928,21 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		ret = iomap_file_buffered_write(iocb, from, &ntfs_write_iomap_ops,
>   				&ntfs_iomap_folio_ops, NULL);
>   out:
> -	if (ret < 0 && ret != -EIOCBQUEUED) {
> +	/*
> +	 * Both sizes were moved to end before the copy, so a short write,
> +	 * like a failed one, must take back what it did not write.
> +	 */
> +	if (ret < count && ret != -EIOCBQUEUED) {
> +		loff_t new_init_size = old_init_size;
> +		loff_t new_data_size = old_data_size;
>   		int init_err = 0;
>   		int data_err = 0;
>   
> +		if (ret > 0) {
> +			new_init_size = max(old_init_size, pos + ret);
> +			new_data_size = max(old_data_size, pos + ret);
> +		}
> +
>   		/*
>   		 * Like ntfs_setattr_size(): __ntfs_attr_truncate_vfs()
>   		 * expects the invalidate lock held across the size
> @@ -940,13 +951,23 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		filemap_invalidate_lock(vi->i_mapping);
>   
>   		/* ntfs_attr_set_initialized_size() rejects a resident one. */
> -		if (ni->initialized_size != old_init_size &&
> +		if (ni->initialized_size != new_init_size &&
>   		    NInoNonResident(ni)) {
>   			struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
>   
> +			/*
> +			 * A short copy can leave the rest of a block read
> +			 * from disk in the cache, also past the temporarily
> +			 * extended initialized size.  Clear everything that
> +			 * is no longer initialized, as
> +			 * __ntfs_extend_initialized_size() does.
> +			 */
> +			truncate_pagecache_range(vi, new_init_size,
> +					max_t(loff_t, i_size_read(vi),
> +					      ni->initialized_size) - 1);
>   			mutex_lock(&mrec_ni->mrec_lock);

Hi Xusheng,
    I missed the concurrent mmap case in my earlier suggestion to 
truncate the page cache.
    Sorry for overlooking this. This truncation can discard valid data 
written through a shared mapping while the buffered write is copying 
user data.

    I reproduced this with the following sequence on an x86_64 VM using 
4 KiB pages and 4 KiB NTFS clusters:

1. Create an empty file and extend it to 1 MiB with ftruncate().
2. Start a buffered pwritev() at offset 0 with three 4 KiB source 
buffers: a valid page, a missing page registered with userfaultfd, and a 
PROT_NONE page.
3. Wait for the userfaultfd event on the second buffer, leaving the 
writer paused during the copy.
4. Create a writable MAP_SHARED mapping at file offset 64 KiB and store 
a known marker there.
5. Resolve the missing source page with UFFDIO_COPY. The inaccessible 
third buffer then causes pwritev() to return a short count of 8192.
6. Read the marker back with pread().

    Before applying v3, the marker is preserved. After applying v3, it 
is lost and its first byte reads back as zero. The file size remains 1 
MiB in both cases.

    The mmap operation has already extended initialized_size beyond the 
marker, but new_init_size is calculated from the pre-write snapshot and 
this write's return value, so it becomes 8 KiB. The cache truncation 
discards the marker, and the subsequent initialized-size rollback also 
fails to preserve the initialization established by mmap.

    Taking invalidate_lock here is too late to prevent this sequence: 
the mmap initialization and store have already completed. The inode lock 
held by the write iterator does not serialize against mmap 
initialization either.

    We need to revise the rollback approach to preserve initialization 
and data established by concurrent mmap activity. Simply narrowing the 
cache truncation range would not address the initialized-size rollback.
    One possible direction is to initialize newly exposed bytes before 
publishing initialized_size and retain that initialization after a short 
write. This would also need careful handling of direct I/O and NOWAIT, 
since populating pagecache before NOWAIT direct I/O can itself cause EAGAIN.

Thanks,
Baolin.


>   			init_err = ntfs_attr_set_initialized_size(ni,
> -					old_init_size);
> +					new_init_size);
>   			mutex_unlock(&mrec_ni->mrec_lock);
>   		}
>   		/*
> @@ -955,13 +976,13 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		 * the uninitialized tail, so skip it unless the rollback
>   		 * above restored that size.
>   		 */
> -		if (ni->data_size != old_data_size && !init_err) {
> +		if (ni->data_size != new_data_size && !init_err) {
>   			struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
>   
> -			truncate_setsize(vi, old_data_size);
> +			truncate_setsize(vi, new_data_size);
>   			mutex_lock(&mrec_ni->mrec_lock);
> -			data_err = __ntfs_attr_truncate_vfs(ni, old_data_size,
> -					old_data_size);
> +			data_err = __ntfs_attr_truncate_vfs(ni, new_data_size,
> +					new_data_size);
>   			mutex_unlock(&mrec_ni->mrec_lock);
>   			/* Like ntfs_setattr_size(): stay on the durable size. */
>   			if (data_err)
> 
> base-commit: b025f526c5d04929212c9d5fb7ffc96608f3eacc