[PATCH] ksmbd: truncate files before allocating space below EOF

[PATCH] ksmbd: truncate files before allocating space below EOF

[DaeMyung Kang]

set_file_allocation_info() compares the requested allocation with
stat.blocks to decide whether to fallocate or truncate. A file with
holes can have fewer allocated blocks than requested even when its EOF
is beyond the requested allocation size. In that case, ksmbd fallocates
with FALLOC_FL_KEEP_SIZE and returns success without shrinking the file.

For example, a file with one byte at offset 1 MiB has an EOF of
1 MiB + 1 but only one allocated page. Setting AllocationSize to 512 KiB
leaves the EOF and the byte unchanged, including after reopening the
file. [MS-FSA] section 2.1.5.15.1 requires EOF to be reduced when the
new allocation size is smaller than the file size.

Check EOF before comparing allocated blocks. Truncate first when the
requested allocation falls below EOF, and skip fallocate in that case.
This avoids allocating holes immediately before discarding them and
allows a shrink request to succeed when space is exhausted. Preserve
the existing behavior for allocation changes that do not shorten EOF.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
 fs/smb/server/smb2pdu.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 12a43efdb53f..b155a27eaeac 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -8533,23 +8533,15 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 	alloc_blks = (alloc_size + 511) >> 9;
 	inode = file_inode(fp->filp);
 
-	if (alloc_blks > stat.blocks) {
-		smb_break_all_levII_oplock(work, fp, 1);
-		rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0,
-				   alloc_blks * 512);
-		if (rc && rc != -EOPNOTSUPP) {
-			pr_err("vfs_fallocate is failed : %d\n", rc);
-			return rc;
-		}
-	} else if (alloc_blks < stat.blocks) {
+	if (alloc_blks * 512 < i_size_read(inode) ||
+	    alloc_blks < stat.blocks) {
 		loff_t size;
 
 		/*
-		 * Allocation size could be smaller than original one
-		 * which means allocated blocks in file should be
-		 * deallocated. use truncate to cut out it, but inode
-		 * size is also updated with truncate offset.
-		 * inode size is retained by backup inode size.
+		 * Truncate before considering allocation when the new size is below
+		 * EOF. This avoids allocating holes in a sparse file just before
+		 * shrinking it. Also release excess blocks if EOF is already
+		 * smaller than the requested allocation size.
 		 */
 		size = i_size_read(inode);
 		rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
@@ -8559,6 +8551,14 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 		}
 		if (size < alloc_blks * 512)
 			i_size_write(inode, size);
+	} else if (alloc_blks > stat.blocks) {
+		smb_break_all_levII_oplock(work, fp, 1);
+		rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0,
+				   alloc_blks * 512);
+		if (rc && rc != -EOPNOTSUPP) {
+			pr_err("vfs_fallocate is failed : %d\n", rc);
+			return rc;
+		}
 	}
 
 	fp->allocation_size = le64_to_cpu(file_alloc_info->AllocationSize);
-- 
2.43.0

Re: [PATCH] ksmbd: truncate files before allocating space below EOF

[Namjae Jeon]

> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index 12a43efdb53f..b155a27eaeac 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -8533,23 +8533,15 @@ static int set_file_allocation_info(struct ksmbd_work *work,
>         alloc_blks = (alloc_size + 511) >> 9;
>         inode = file_inode(fp->filp);
>
> -       if (alloc_blks > stat.blocks) {
> -               smb_break_all_levII_oplock(work, fp, 1);
> -               rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0,
> -                                  alloc_blks * 512);
> -               if (rc && rc != -EOPNOTSUPP) {
> -                       pr_err("vfs_fallocate is failed : %d\n", rc);
> -                       return rc;
> -               }
> -       } else if (alloc_blks < stat.blocks) {
> +       if (alloc_blks * 512 < i_size_read(inode) ||
> +           alloc_blks < stat.blocks) {
>                 loff_t size;
>
>                 /*
> -                * Allocation size could be smaller than original one
> -                * which means allocated blocks in file should be
> -                * deallocated. use truncate to cut out it, but inode
> -                * size is also updated with truncate offset.
> -                * inode size is retained by backup inode size.
> +                * Truncate before considering allocation when the new size is below
> +                * EOF. This avoids allocating holes in a sparse file just before
> +                * shrinking it. Also release excess blocks if EOF is already
> +                * smaller than the requested allocation size.
>                  */
>                 size = i_size_read(inode);
>                 rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
Using alloc_blks * 512 for fallocate() and truncate() doesn't look right.
MS-FSCC section 2.4.4 describe:

"The file system rounds the requested allocation size up to an integer
multiple of the cluster size for nonresident files, or an
implementation-defined multiple for resident files."

And the truncate branch skips fallocate(), so it can return success
without reserving the requested space. For example, if a file has data
only at offset 1MB, setting AllocationSize to 512 KiB removes that
data. The file size will be 512KB, but the file will have no allocated
blocks.


> @@ -8559,6 +8551,14 @@ static int set_file_allocation_info(struct ksmbd_work *work,
>                 }
>                 if (size < alloc_blks * 512)
>                         i_size_write(inode, size);
> +       } else if (alloc_blks > stat.blocks) {
> +               smb_break_all_levII_oplock(work, fp, 1);
> +               rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0,
> +                                  alloc_blks * 512);
> +               if (rc && rc != -EOPNOTSUPP) {
> +                       pr_err("vfs_fallocate is failed : %d\n", rc);
> +                       return rc;
> +               }
>         }
>
>         fp->allocation_size = le64_to_cpu(file_alloc_info->AllocationSize);
> --
> 2.43.0
>

[PATCH 0/2] ksmbd: fix SetAllocation EOF and rounding

[DaeMyung Kang]

In response to Namjae's review of my earlier SetAllocation patch, this
series separates two fixes. Patch 1/2 prevents the existing cleanup
branch from extending EOF on disk and then restoring only the in-memory
size. It carries Fixes and Cc: stable. Patch 2/2 rounds the request to
f_bsize, the allocation unit ksmbd advertises in FS_SIZE_INFORMATION,
instead of 512 bytes.

The other review point, reserving the requested range when shrinking,
remains open. I tried fallocate before truncate, but the decision to
shrink uses an EOF value read before vfs_truncate() takes inode_lock.
The existing block-count branch already has this race; extending
truncate to additional sparse-file cases would expose it more widely.
If another client truncates and writes in that interval, our truncate
can extend the file again with zeros. I reproduced this with an
injected delay in QEMU. Rechecking EOF after fallocate narrows the
window but does not close it.

A VFS operation that checks EOF and truncates under the same inode_lock
without extending the file may be needed. I plan to discuss the
reservation fix separately with linux-fsdevel and would welcome your
thoughts on that approach.

DaeMyung Kang (2):
  ksmbd: avoid restoring only in-memory EOF after allocation change
  ksmbd: round SetAllocation to the advertised allocation unit

 fs/smb/server/smb2pdu.c | 48 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 25 deletions(-)


base-commit: bf3465919b0337dfef8798da01469145887c4ef5
-- 
2.43.0

[PATCH 1/2] ksmbd: avoid restoring only in-memory EOF after allocation change

[DaeMyung Kang]

When SetAllocation requests a size below the allocated block count but
above EOF, ksmbd truncates the file to that size and restores only the
in-memory i_size. The larger EOF remains on disk and reappears after a
reboot. A 3-byte file preallocated to 64 KiB can acquire a 4 KiB EOF
after reducing its allocation to 4 KiB. Extending EOF this way is not
a reliable way to reclaim the preallocation beyond the old EOF either.

Only use this truncate path when the requested allocation is at or below
the observed EOF. A truncate to the existing EOF still lets filesystems
release preallocated blocks beyond EOF. Remove the in-memory-only i_size
restoration. The size check still precedes vfs_truncate() taking the
inode lock, so a concurrent shrink can make the truncate extend EOF;
that race also existed in the original path.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
 fs/smb/server/smb2pdu.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 2da6163ee776..d06bdc8b23db 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -8636,24 +8636,17 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 			pr_err("vfs_fallocate is failed : %d\n", rc);
 			return rc;
 		}
-	} else if (alloc_blks < stat.blocks) {
-		loff_t size;
-
+	} else if (alloc_blks < stat.blocks &&
+		   alloc_blks * 512 <= i_size_read(inode)) {
 		/*
-		 * Allocation size could be smaller than original one
-		 * which means allocated blocks in file should be
-		 * deallocated. use truncate to cut out it, but inode
-		 * size is also updated with truncate offset.
-		 * inode size is retained by backup inode size.
+		 * Truncate at or below EOF to discard excess allocation.
+		 * Truncating above EOF would extend the file on disk.
 		 */
-		size = i_size_read(inode);
 		rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
 		if (rc) {
 			pr_err("truncate failed!, err %d\n", rc);
 			return rc;
 		}
-		if (size < alloc_blks * 512)
-			i_size_write(inode, size);
 	}
 
 	fp->allocation_size = le64_to_cpu(file_alloc_info->AllocationSize);
-- 
2.43.0

[PATCH 2/2] ksmbd: round SetAllocation to the advertised allocation unit

[DaeMyung Kang]

SetAllocation rounds requests to 512-byte blocks, although
FS_SIZE_INFORMATION advertises the filesystem's f_bsize as its
allocation unit. For example, on a filesystem with a 4 KiB unit, a
1000-byte request can truncate an 8 KiB file to 1024 bytes instead of
4 KiB.

Round the request to the advertised unit and use the same result for
the block-count decision, fallocate, truncate, and the size stored on
the open handle. Validate the unit and rounded size before either
operation. Use DIV_ROUND_UP_ULL() so the calculation works on 32-bit
kernels.

This does not change the existing block-count decision for whether to
call fallocate. In particular, success does not yet guarantee
reservation of holes within the requested range on a sparse file.

Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
 fs/smb/server/smb2pdu.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index d06bdc8b23db..6a91f939f3d6 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -8596,10 +8596,11 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 	 * properly with any smb.conf option
 	 */
 
-	loff_t alloc_blks;
-	u64 alloc_size;
+	u64 alloc_size, target, alloc_blks;
+	u32 alloc_unit;
 	struct inode *inode;
 	struct kstat stat;
+	struct kstatfs stfs;
 	int rc;
 
 	if (!(fp->daccess & FILE_WRITE_DATA_LE))
@@ -8612,44 +8613,48 @@ static int set_file_allocation_info(struct ksmbd_work *work,
 			 AT_STATX_SYNC_AS_STAT);
 	if (rc)
 		return rc;
+	rc = vfs_statfs(&fp->filp->f_path, &stfs);
+	if (rc)
+		return rc;
 
 	/*
-	 * AllocationSize is fully client-controlled (the caller only
-	 * validates the fixed 8-byte buffer length). Reject values that
-	 * would overflow the "round up to 512-byte blocks" conversion
-	 * below instead of silently wrapping it to a tiny block count,
-	 * which would truncate the file to a size the client never
-	 * asked for.
+	 * Use the allocation unit advertised in FS_SIZE_INFORMATION.
+	 * Reject requests that overflow when rounded to a whole unit.
 	 */
 	alloc_size = le64_to_cpu(file_alloc_info->AllocationSize);
-	if (alloc_size > MAX_LFS_FILESIZE - 511)
+	if (stfs.f_bsize <= 0 || (u64)stfs.f_bsize > U32_MAX)
+		return -EINVAL;
+	alloc_unit = stfs.f_bsize;
+	if (alloc_size > MAX_LFS_FILESIZE - (alloc_unit - 1))
 		return -EINVAL;
 
-	alloc_blks = (alloc_size + 511) >> 9;
+	target = DIV_ROUND_UP_ULL(alloc_size, alloc_unit) * alloc_unit;
+	alloc_blks = DIV_ROUND_UP_ULL(target, 512);
 	inode = file_inode(fp->filp);
+	if (target > inode->i_sb->s_maxbytes)
+		return -EFBIG;
 
 	if (alloc_blks > stat.blocks) {
 		smb_break_all_levII_oplock(work, fp, 1);
-		rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0,
-				   alloc_blks * 512);
+		rc = vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0, target);
 		if (rc && rc != -EOPNOTSUPP) {
 			pr_err("vfs_fallocate is failed : %d\n", rc);
 			return rc;
 		}
 	} else if (alloc_blks < stat.blocks &&
-		   alloc_blks * 512 <= i_size_read(inode)) {
+		   target <= i_size_read(inode)) {
 		/*
 		 * Truncate at or below EOF to discard excess allocation.
 		 * Truncating above EOF would extend the file on disk.
 		 */
-		rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
+		rc = ksmbd_vfs_truncate(work, fp, target);
 		if (rc) {
 			pr_err("truncate failed!, err %d\n", rc);
 			return rc;
 		}
 	}
 
-	fp->allocation_size = le64_to_cpu(file_alloc_info->AllocationSize);
+	fp->allocation_size = target;
 	fp->allocation_size_set = true;
 	return 0;
 }
-- 
2.43.0