Re: [PATCH v2] fix open hardlink on deferred close file error
From: Steve French
Date: Sat Apr 12 2025 - 22:35:16 EST
Have rebased the patch so it would merge onto current for-next
(6.15-rc1+), and cleanup a checkpatch warning and added Cc:stable
Have tentatively merged the updated patch (see attached) into cifs-2.6.git
Although I tested that it does fix the problem described in the commit
description, additional review/testing would be welcome.
On Fri, Apr 11, 2025 at 1:13 AM Chunjie Zhu <chunjie.zhu@xxxxxxxxx> wrote:
>
> The following Python script results in unexpected behaviour when run on
> a CIFS filesystem against a Windows Server:
>
> # Create file
> fd = os.open('test', os.O_WRONLY|os.O_CREAT)
> os.write(fd, b'foo')
> os.close(fd)
>
> # Open and close the file to leave a pending deferred close
> fd = os.open('test', os.O_RDONLY|os.O_DIRECT)
> os.close(fd)
>
> # Try to open the file via a hard link
> os.link('test', 'new')
> newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)
>
> The final open returns EINVAL due to the server returning
> STATUS_INVALID_PARAMETER. The root cause of this is that the client
> caches lease keys per inode, but the spec requires them to be related to
> the filename which causes problems when hard links are involved:
>
> From MS-SMB2 section 3.3.5.9.11:
>
> "The server MUST attempt to locate a Lease by performing a lookup in the
> LeaseTable.LeaseList using the LeaseKey in the
> SMB2_CREATE_REQUEST_LEASE_V2 as the lookup key. If a lease is found,
> Lease.FileDeleteOnClose is FALSE, and Lease.Filename does not match the
> file name for the incoming request, the request MUST be failed with
> STATUS_INVALID_PARAMETER"
>
> On client side, we first check the context of file open, if it hits above
> conditions, we first close all opening files which are belong to the same
> inode, then we do open the hard link file.
>
> Signed-off-by: Chunjie Zhu <chunjie.zhu@xxxxxxxxx>
> ---
>
> v2: if error, first close inode opening files and then open hard link
>
> fs/smb/client/cifsproto.h | 2 ++
> fs/smb/client/file.c | 29 +++++++++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 260a6299bddb..b563c227792e 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -157,6 +157,8 @@ extern int cifs_get_writable_path(struct cifs_tcon *tcon, const char *name,
> extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, bool);
> extern int cifs_get_readable_path(struct cifs_tcon *tcon, const char *name,
> struct cifsFileInfo **ret_file);
> +extern int cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode,
> + struct file *file);
> extern unsigned int smbCalcSize(void *buf);
> extern int decode_negTokenInit(unsigned char *security_blob, int length,
> struct TCP_Server_Info *server);
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index 4cbb5487bd8d..8e9582ff70f3 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -751,6 +751,12 @@ int cifs_open(struct inode *inode, struct file *file)
> } else {
> _cifsFileInfo_put(cfile, true, false);
> }
> + } else {
> + /* hard link on the defeered close file */
> + rc = cifs_get_hardlink_path(tcon, inode, file);
> + if (rc) {
> + cifs_close_deferred_file(CIFS_I(inode));
> + }
> }
>
> if (server->oplocks)
> @@ -2413,6 +2419,29 @@ cifs_get_readable_path(struct cifs_tcon *tcon, const char *name,
> return -ENOENT;
> }
>
> +int
> +cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode,
> + struct file *file)
> +{
> + struct cifsFileInfo *open_file = NULL;
> + struct cifsInodeInfo *cinode = CIFS_I(inode);
> + int rc = 0;
> +
> + spin_lock(&tcon->open_file_lock);
> + spin_lock(&cinode->open_file_lock);
> +
> + list_for_each_entry(open_file, &cinode->openFileList, flist) {
> + if (file->f_flags == open_file->f_flags) {
> + rc = -EINVAL;
> + break;
> + }
> + }
> +
> + spin_unlock(&cinode->open_file_lock);
> + spin_unlock(&tcon->open_file_lock);
> + return rc;
> +}
> +
> void
> cifs_writedata_release(struct kref *refcount)
> {
> --
> 2.34.1
>
--
Thanks,
Steve
From b5e909ed835c96b36d536b66e8767f20dfb13559 Mon Sep 17 00:00:00 2001
From: Chunjie Zhu <chunjie.zhu@xxxxxxxxx>
Date: Sat, 12 Apr 2025 21:15:55 -0500
Subject: [PATCH 2/2] smb3 client: fix open hardlink on deferred close file
error
The following Python script results in unexpected behaviour when run on
a CIFS filesystem against a Windows Server:
# Create file
fd = os.open('test', os.O_WRONLY|os.O_CREAT)
os.write(fd, b'foo')
os.close(fd)
# Open and close the file to leave a pending deferred close
fd = os.open('test', os.O_RDONLY|os.O_DIRECT)
os.close(fd)
# Try to open the file via a hard link
os.link('test', 'new')
newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)
The final open returns EINVAL due to the server returning
STATUS_INVALID_PARAMETER. The root cause of this is that the client
caches lease keys per inode, but the spec requires them to be related to
the filename which causes problems when hard links are involved:
From MS-SMB2 section 3.3.5.9.11:
"The server MUST attempt to locate a Lease by performing a lookup in the
LeaseTable.LeaseList using the LeaseKey in the
SMB2_CREATE_REQUEST_LEASE_V2 as the lookup key. If a lease is found,
Lease.FileDeleteOnClose is FALSE, and Lease.Filename does not match the
file name for the incoming request, the request MUST be failed with
STATUS_INVALID_PARAMETER"
On client side, we first check the context of file open, if it hits above
conditions, we first close all opening files which are belong to the same
inode, then we do open the hard link file.
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Chunjie Zhu <chunjie.zhu@xxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
v2: if error, first close inode opening files and then open hard link
---
fs/smb/client/cifsproto.h | 2 ++
fs/smb/client/file.c | 28 ++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index cfcc07905bdf..59f6fdfe560e 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -163,6 +163,8 @@ extern int cifs_get_writable_path(struct cifs_tcon *tcon, const char *name,
extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, bool);
extern int cifs_get_readable_path(struct cifs_tcon *tcon, const char *name,
struct cifsFileInfo **ret_file);
+extern int cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode,
+ struct file *file);
extern unsigned int smbCalcSize(void *buf);
extern int decode_negTokenInit(unsigned char *security_blob, int length,
struct TCP_Server_Info *server);
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 8407fb108664..9e8f404b9e56 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -1007,6 +1007,11 @@ int cifs_open(struct inode *inode, struct file *file)
} else {
_cifsFileInfo_put(cfile, true, false);
}
+ } else {
+ /* hard link on the defeered close file */
+ rc = cifs_get_hardlink_path(tcon, inode, file);
+ if (rc)
+ cifs_close_deferred_file(CIFS_I(inode));
}
if (server->oplocks)
@@ -2071,6 +2076,29 @@ cifs_move_llist(struct list_head *source, struct list_head *dest)
list_move(li, dest);
}
+int
+cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode,
+ struct file *file)
+{
+ struct cifsFileInfo *open_file = NULL;
+ struct cifsInodeInfo *cinode = CIFS_I(inode);
+ int rc = 0;
+
+ spin_lock(&tcon->open_file_lock);
+ spin_lock(&cinode->open_file_lock);
+
+ list_for_each_entry(open_file, &cinode->openFileList, flist) {
+ if (file->f_flags == open_file->f_flags) {
+ rc = -EINVAL;
+ break;
+ }
+ }
+
+ spin_unlock(&cinode->open_file_lock);
+ spin_unlock(&tcon->open_file_lock);
+ return rc;
+}
+
void
cifs_free_llist(struct list_head *llist)
{
--
2.43.0