[PATCH v4] ksmbd: remove unnecessary generic_fillattr in smb2_open

[PATCH v4] ksmbd: remove unnecessary generic_fillattr in smb2_open

[Hyunchul Lee]

Remove unnecessary generic_fillattr to fix wrong
AllocationSize of SMB2_CREATE response, And
Move the call of ksmbd_vfs_getattr above the place
where stat is needed because of truncate.

This patch fixes wrong AllocationSize of SMB2_CREATE
response. Because ext4 updates inode->i_blocks only
when disk space is allocated, generic_fillattr does
not set stat.blocks properly for delayed allocation.
But ext4 returns the blocks that include the delayed
allocation blocks when getattr is called.

The issue can be reproduced with commands below:

touch ${FILENAME}
xfs_io -c "pwrite -S 0xAB 0 40k" ${FILENAME}
xfs_io -c "stat" ${FILENAME}

40KB are written, but the count of blocks is 8.

Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
Changes from v1:
 - Update the commit description.
Changes from v2:
 - Fix the commit description and add the way
   to reproduce the issue.
Changes from v3:
 - Delete generic_fillattr.

 fs/ksmbd/smb2pdu.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 9751cc92c111..c41ec3d2abe8 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -3042,12 +3042,6 @@ int smb2_open(struct ksmbd_work *work)
 	list_add(&fp->node, &fp->f_ci->m_fp_list);
 	write_unlock(&fp->f_ci->m_lock);
 
-	rc = ksmbd_vfs_getattr(&path, &stat);
-	if (rc) {
-		generic_fillattr(user_ns, d_inode(path.dentry), &stat);
-		rc = 0;
-	}
-
 	/* Check delete pending among previous fp before oplock break */
 	if (ksmbd_inode_pending_delete(fp)) {
 		rc = -EBUSY;
@@ -3134,6 +3128,10 @@ int smb2_open(struct ksmbd_work *work)
 		}
 	}
 
+	rc = ksmbd_vfs_getattr(&path, &stat);
+	if (rc)
+		goto err_out;
+
 	if (stat.result_mask & STATX_BTIME)
 		fp->create_time = ksmbd_UnixTimeToNT(stat.btime);
 	else
@@ -3149,9 +3147,6 @@ int smb2_open(struct ksmbd_work *work)
 
 	memcpy(fp->client_guid, conn->ClientGUID, SMB2_CLIENT_GUID_SIZE);
 
-	generic_fillattr(user_ns, file_inode(fp->filp),
-			 &stat);
-
 	rsp->StructureSize = cpu_to_le16(89);
 	rcu_read_lock();
 	opinfo = rcu_dereference(fp->f_opinfo);
-- 
2.25.1

Re: [PATCH v4] ksmbd: remove unnecessary generic_fillattr in smb2_open

[Namjae Jeon]

2022-08-12 11:11 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> Remove unnecessary generic_fillattr to fix wrong
> AllocationSize of SMB2_CREATE response, And
> Move the call of ksmbd_vfs_getattr above the place
> where stat is needed because of truncate.
>
> This patch fixes wrong AllocationSize of SMB2_CREATE
> response. Because ext4 updates inode->i_blocks only
> when disk space is allocated, generic_fillattr does
> not set stat.blocks properly for delayed allocation.
> But ext4 returns the blocks that include the delayed
> allocation blocks when getattr is called.
>
> The issue can be reproduced with commands below:
>
> touch ${FILENAME}
> xfs_io -c "pwrite -S 0xAB 0 40k" ${FILENAME}
> xfs_io -c "stat" ${FILENAME}
>
> 40KB are written, but the count of blocks is 8.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!

Re: [PATCH v4] ksmbd: remove unnecessary generic_fillattr in smb2_open

[Namjae Jeon]

2022-08-12 11:11 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> Remove unnecessary generic_fillattr to fix wrong
> AllocationSize of SMB2_CREATE response, And
> Move the call of ksmbd_vfs_getattr above the place
> where stat is needed because of truncate.
>
> This patch fixes wrong AllocationSize of SMB2_CREATE
> response. Because ext4 updates inode->i_blocks only
> when disk space is allocated, generic_fillattr does
> not set stat.blocks properly for delayed allocation.
> But ext4 returns the blocks that include the delayed
> allocation blocks when getattr is called.
>
> The issue can be reproduced with commands below:
>
> touch ${FILENAME}
> xfs_io -c "pwrite -S 0xAB 0 40k" ${FILENAME}
> xfs_io -c "stat" ${FILENAME}
>
> 40KB are written, but the count of blocks is 8.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
This patch seems to cause the smb2.create.open test in smbtorture to
fail. Can you reproduce it too?

Re: [PATCH v4] ksmbd: remove unnecessary generic_fillattr in smb2_open

[Hyunchul Lee]

2022년 8월 24일 (수) 오후 11:09, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2022-08-12 11:11 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > Remove unnecessary generic_fillattr to fix wrong
> > AllocationSize of SMB2_CREATE response, And
> > Move the call of ksmbd_vfs_getattr above the place
> > where stat is needed because of truncate.
> >
> > This patch fixes wrong AllocationSize of SMB2_CREATE
> > response. Because ext4 updates inode->i_blocks only
> > when disk space is allocated, generic_fillattr does
> > not set stat.blocks properly for delayed allocation.
> > But ext4 returns the blocks that include the delayed
> > allocation blocks when getattr is called.
> >
> > The issue can be reproduced with commands below:
> >
> > touch ${FILENAME}
> > xfs_io -c "pwrite -S 0xAB 0 40k" ${FILENAME}
> > xfs_io -c "stat" ${FILENAME}
> >
> > 40KB are written, but the count of blocks is 8.
> >
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> This patch seems to cause the smb2.create.open test in smbtorture to
> fail. Can you reproduce it too?

Yes, this issue can be reproduced. I will look into this.

-- 
Thanks,
Hyunchul