| Message ID | 20250317083915.20004-1-chunjie.zhu@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | open hardlink on deferred close file return EINVAL | expand |
I tried out the patch and it does fix getting STATUS_INVALID_PARAMETER
error from the server, but doesn't fix the issue of getting EINVAL
Traceback (most recent call last):
File "/root/hl-test.py", line 15, in <module>
newfd = os.open('new', os.O_RDONLY|os.O_DIRECT)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 22] Invalid argument: 'new'
It is fixed by running with leases disable (via mount parm), but
wouldn't it be better to fix the error so apps don't break. Ideas?
On Mon, Mar 17, 2025 at 3:41 AM Chunjie Zhu <chunjie.zhu@cloud.com> 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"
>
> The client side can return EINVAL directly without invoking server
> operations. This reduces client server network communication overhead.
>
> Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com>
> ---
> 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..0a66cce6e0ff 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) {
> + goto out;
> + }
> }
>
> 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
>
>
If we run 2 applications on a CIFS client machine, one application opens file A, the other application opens file B which is hard link of file A, this issue would happen, as well. The purpose of this patch is to reduce the CIFS protocol network communication as we can decide how to responsd to application at client side. > It is fixed by running with leases disable (via mount parm), but > wouldn't it be better to fix the error so apps don't break. Ideas? > Ideas, Extend SMB SMB_COM_OPEN_ANDX and SMB_COM_NT_CREATE_ANDX messages, add file alias into open or create request messages, an alias means a hard link of the original file. > On Mon, Mar 17, 2025 at 3:41=E2=80=AFAM Chunjie Zhu <chunjie.zhu@cloud.com>= > wrote: > > > > The following Python script results in unexpected behaviour when run on > > a CIFS filesystem against a Windows Server: > > > > # Create file > > fd =3D 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 =3D 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 =3D 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" > > > > The client side can return EINVAL directly without invoking server > > operations. This reduces client server network communication overhead. > > > > Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com> > > --- > > 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 *t= > con, const char *name, > > extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, b= > ool); > > extern int cifs_get_readable_path(struct cifs_tcon *tcon, const char *na= > me, > > 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..0a66cce6e0ff 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 =3D cifs_get_hardlink_path(tcon, inode, file); > > + if (rc) { > > + goto out; > > + } > > } > > > > if (server->oplocks) > > @@ -2413,6 +2419,29 @@ cifs_get_readable_path(struct cifs_tcon *tcon, con= > st char *name, > > return -ENOENT; > > } > > > > +int > > +cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode, > > + struct file *file) > > +{ > > + struct cifsFileInfo *open_file =3D NULL; > > + struct cifsInodeInfo *cinode =3D CIFS_I(inode); > > + int rc =3D 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 =3D=3D open_file->f_flags) { > > + rc =3D -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 > > > > > > > --=20 > Thanks, > > Steve >
Wouldn't a simpler fix be to either not include the lease key when opening the hardlink of the file which already has a lease on the other filename? or alternatively don't request a lease at all on the second open? or if we get the error invalid parameter then check if the problem was the other open of the hardlink and either immediately close it and reopen (if deferred close) or retry the open without requesting leases? Failing intentionally seems wrong On Mon, Mar 17, 2025 at 11:19 PM Chunjie Zhu <chunjie.zhu@cloud.com> wrote: > > If we run 2 applications on a CIFS client machine, one application opens file A, > the other application opens file B which is hard link of file A, this issue would > happen, as well. > > The purpose of this patch is to reduce the CIFS protocol network communication as > we can decide how to responsd to application at client side. > > > It is fixed by running with leases disable (via mount parm), but > > wouldn't it be better to fix the error so apps don't break. Ideas? > > > > Ideas, > > Extend SMB SMB_COM_OPEN_ANDX and SMB_COM_NT_CREATE_ANDX messages, add file alias > into open or create request messages, an alias means a hard link of the original > file. > > > On Mon, Mar 17, 2025 at 3:41=E2=80=AFAM Chunjie Zhu <chunjie.zhu@cloud.com>= > > wrote: > > > > > > The following Python script results in unexpected behaviour when run on > > > a CIFS filesystem against a Windows Server: > > > > > > # Create file > > > fd =3D 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 =3D 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 =3D 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" > > > > > > The client side can return EINVAL directly without invoking server > > > operations. This reduces client server network communication overhead. > > > > > > Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com> > > > --- > > > 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 *t= > > con, const char *name, > > > extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, b= > > ool); > > > extern int cifs_get_readable_path(struct cifs_tcon *tcon, const char *na= > > me, > > > 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..0a66cce6e0ff 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 =3D cifs_get_hardlink_path(tcon, inode, file); > > > + if (rc) { > > > + goto out; > > > + } > > > } > > > > > > if (server->oplocks) > > > @@ -2413,6 +2419,29 @@ cifs_get_readable_path(struct cifs_tcon *tcon, con= > > st char *name, > > > return -ENOENT; > > > } > > > > > > +int > > > +cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode, > > > + struct file *file) > > > +{ > > > + struct cifsFileInfo *open_file =3D NULL; > > > + struct cifsInodeInfo *cinode =3D CIFS_I(inode); > > > + int rc =3D 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 =3D=3D open_file->f_flags) { > > > + rc =3D -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 > > > > > > > > > > > > --=20 > > Thanks, > > > > Steve > >
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..0a66cce6e0ff 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) { + goto out; + } } 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) {
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" The client side can return EINVAL directly without invoking server operations. This reduces client server network communication overhead. Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com> --- fs/smb/client/cifsproto.h | 2 ++ fs/smb/client/file.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+)