Re: [PATCH] open hardlink on deferred close file return EINVAL

[Steve French]

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@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"
>
> 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@xxxxxxxxx>
> ---
>  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
>
>


-- 
Thanks,

Steve