lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFTVevUPYBO8vgFY3+yYuJYf4TnwMFk9JHPfAa+ZJQLDEEVHeA@mail.gmail.com>
Date: Wed, 6 Mar 2024 09:44:58 +0530
From: Meetakshi Setiya <meetakshisetiyaoss@...il.com>
To: sfrench@...ba.org, pc@...guebit.com, ronniesahlberg@...il.com, 
	sprasad@...rosoft.com, nspmangalore@...il.com, tom@...pey.com, 
	linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	samba-technical@...ts.samba.org, bharathsm.hsk@...il.com
Cc: Meetakshi Setiya <msetiya@...rosoft.com>
Subject: Re: [PATCH 1/3] smb: client: reuse file lease key in compound operations

On its own, the patch is expected to regress xfstests generic 013,
002 and 591.

The fix for 591 is in part 2 of this patch series
https://lore.kernel.org/all/20240306034353.190039-2-meetakshisetiyaoss@gmail.com/

The fix for 002 and 013 is in part 3 of this patch series
https://lore.kernel.org/all/20240306034353.190039-3-meetakshisetiyaoss@gmail.com/

On Wed, Mar 6, 2024 at 9:13 AM <meetakshisetiyaoss@...il.com> wrote:
>
> From: Meetakshi Setiya <msetiya@...rosoft.com>
>
> Currently, when a rename, unlink or set path size compound operation
> is requested on a file that has a lot of dirty pages to be written
> to the server, we do not send the lease key for these requests. As a
> result, the server can assume that this request is from a new client, and
> send a lease break notification to the same client, on the same
> connection. As a response to the lease break, the client can consume
> several credits to write the dirty pages to the server. Depending on the
> server's credit grant implementation, the server can stop granting more
> credits to this connection, and this can cause a deadlock (which can only
> be resolved when the lease timer on the server expires).
> One of the problems here is that the client is sending no lease key,
> even if it has a lease for the file. This patch fixes the problem by
> reusing the existing lease key on the file for rename, unlink and set path
> size compound operations so that the client does not break its own lease.
>
> A very trivial example could be a set of commands by a client that
> maintains open handle (for write) to a file and then tries to copy the
> contents of that file to another one, eg.,
>
> tail -f /dev/null > myfile &
> mv myfile myfile2
>
> Presently, the network capture on the client shows that the move (or
> rename) would trigger a lease break on the same client, for the same file.
> With the lease key reused, the lease break request-response overhead is
> eliminated, thereby reducing the roundtrips performed for this set of
> operations.
>
> The patch fixes the bug described above and also provides perf benefit.
>
> Signed-off-by: Meetakshi Setiya <msetiya@...rosoft.com>
> ---
>  fs/smb/client/cifsglob.h  |  5 ++--
>  fs/smb/client/cifsproto.h |  6 +++--
>  fs/smb/client/cifssmb.c   |  4 ++--
>  fs/smb/client/inode.c     | 10 ++++----
>  fs/smb/client/smb2inode.c | 48 ++++++++++++++++++++++++---------------
>  fs/smb/client/smb2proto.h |  6 +++--
>  6 files changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 16befff4cbb4..50f7e47c2229 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -371,7 +371,8 @@ struct smb_version_operations {
>                             struct cifs_open_info_data *data);
>         /* set size by path */
>         int (*set_path_size)(const unsigned int, struct cifs_tcon *,
> -                            const char *, __u64, struct cifs_sb_info *, bool);
> +                            const char *, __u64, struct cifs_sb_info *, bool,
> +                                struct dentry *);
>         /* set size by file handle */
>         int (*set_file_size)(const unsigned int, struct cifs_tcon *,
>                              struct cifsFileInfo *, __u64, bool);
> @@ -401,7 +402,7 @@ struct smb_version_operations {
>                      struct cifs_sb_info *);
>         /* unlink file */
>         int (*unlink)(const unsigned int, struct cifs_tcon *, const char *,
> -                     struct cifs_sb_info *);
> +                     struct cifs_sb_info *, struct dentry *);
>         /* open, rename and delete file */
>         int (*rename_pending_delete)(const char *, struct dentry *,
>                                      const unsigned int);
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index a841bf4967fa..ef98c840791f 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -402,7 +402,8 @@ extern int CIFSSMBSetFileDisposition(const unsigned int xid,
>                                      __u32 pid_of_opener);
>  extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
>                          const char *file_name, __u64 size,
> -                        struct cifs_sb_info *cifs_sb, bool set_allocation);
> +                        struct cifs_sb_info *cifs_sb, bool set_allocation,
> +                        struct dentry *dentry);
>  extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>                               struct cifsFileInfo *cfile, __u64 size,
>                               bool set_allocation);
> @@ -438,7 +439,8 @@ extern int CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
>                         const struct nls_table *nls_codepage,
>                         int remap_special_chars);
>  extern int CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon,
> -                         const char *name, struct cifs_sb_info *cifs_sb);
> +                         const char *name, struct cifs_sb_info *cifs_sb,
> +                         struct dentry *dentry);
>  int CIFSSMBRename(const unsigned int xid, struct cifs_tcon *tcon,
>                   struct dentry *source_dentry,
>                   const char *from_name, const char *to_name,
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 01e89070df5a..301189ee1335 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -738,7 +738,7 @@ CIFSPOSIXDelFile(const unsigned int xid, struct cifs_tcon *tcon,
>
>  int
>  CIFSSMBDelFile(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> -              struct cifs_sb_info *cifs_sb)
> +              struct cifs_sb_info *cifs_sb, struct dentry *dentry)
>  {
>         DELETE_FILE_REQ *pSMB = NULL;
>         DELETE_FILE_RSP *pSMBr = NULL;
> @@ -4993,7 +4993,7 @@ CIFSSMBQFSPosixInfo(const unsigned int xid, struct cifs_tcon *tcon,
>  int
>  CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
>               const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> -             bool set_allocation)
> +             bool set_allocation, struct dentry *dentry)
>  {
>         struct smb_com_transaction2_spi_req *pSMB = NULL;
>         struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index d02f8ba29cb5..3073eac989ea 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -1846,7 +1846,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
>                 goto psx_del_no_retry;
>         }
>
> -       rc = server->ops->unlink(xid, tcon, full_path, cifs_sb);
> +       rc = server->ops->unlink(xid, tcon, full_path, cifs_sb, dentry);
>
>  psx_del_no_retry:
>         if (!rc) {
> @@ -2797,7 +2797,7 @@ void cifs_setsize(struct inode *inode, loff_t offset)
>
>  static int
>  cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> -                  unsigned int xid, const char *full_path)
> +                  unsigned int xid, const char *full_path, struct dentry *dentry)
>  {
>         int rc;
>         struct cifsFileInfo *open_file;
> @@ -2848,7 +2848,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>          */
>         if (server->ops->set_path_size)
>                 rc = server->ops->set_path_size(xid, tcon, full_path,
> -                                               attrs->ia_size, cifs_sb, false);
> +                                               attrs->ia_size, cifs_sb, false, dentry);
>         else
>                 rc = -ENOSYS;
>         cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
> @@ -2938,7 +2938,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
>         rc = 0;
>
>         if (attrs->ia_valid & ATTR_SIZE) {
> -               rc = cifs_set_file_size(inode, attrs, xid, full_path);
> +               rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
>                 if (rc != 0)
>                         goto out;
>         }
> @@ -3105,7 +3105,7 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>         }
>
>         if (attrs->ia_valid & ATTR_SIZE) {
> -               rc = cifs_set_file_size(inode, attrs, xid, full_path);
> +               rc = cifs_set_file_size(inode, attrs, xid, full_path, direntry);
>                 if (rc != 0)
>                         goto cifs_setattr_exit;
>         }
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 05818cd6d932..69f3442c5b96 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -98,7 +98,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
>                             __u32 desired_access, __u32 create_disposition,
>                             __u32 create_options, umode_t mode, struct kvec *in_iov,
>                             int *cmds, int num_cmds, struct cifsFileInfo *cfile,
> -                           struct kvec *out_iov, int *out_buftype)
> +                           struct kvec *out_iov, int *out_buftype, struct dentry *dentry)
>  {
>
>         struct reparse_data_buffer *rbuf;
> @@ -115,6 +115,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
>         int resp_buftype[MAX_COMPOUND];
>         struct smb2_query_info_rsp *qi_rsp = NULL;
>         struct cifs_open_info_data *idata;
> +       struct inode *inode = NULL;
>         int flags = 0;
>         __u8 delete_pending[8] = {1, 0, 0, 0, 0, 0, 0, 0};
>         unsigned int size[2];
> @@ -152,6 +153,15 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
>                 goto finished;
>         }
>
> +       /* if there is an existing lease, reuse it */
> +       if (dentry) {
> +               inode = d_inode(dentry);
> +               if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> +                       oplock = SMB2_OPLOCK_LEVEL_LEASE;
> +                       server->ops->get_lease_key(inode, &fid);
> +               }
> +       }
> +
>         vars->oparms = (struct cifs_open_parms) {
>                 .tcon = tcon,
>                 .path = full_path,
> @@ -747,7 +757,7 @@ int smb2_query_path_info(const unsigned int xid,
>         rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
>                               FILE_READ_ATTRIBUTES, FILE_OPEN,
>                               create_options, ACL_NO_MODE, in_iov,
> -                             cmds, 1, cfile, out_iov, out_buftype);
> +                             cmds, 1, cfile, out_iov, out_buftype, NULL);
>         hdr = out_iov[0].iov_base;
>         /*
>          * If first iov is unset, then SMB session was dropped or we've got a
> @@ -779,7 +789,7 @@ int smb2_query_path_info(const unsigned int xid,
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
>                                       FILE_READ_ATTRIBUTES, FILE_OPEN,
>                                       create_options, ACL_NO_MODE, in_iov,
> -                                     cmds, num_cmds, cfile, NULL, NULL);
> +                                     cmds, num_cmds, cfile, NULL, NULL, NULL);
>                 break;
>         case -EREMOTE:
>                 break;
> @@ -811,7 +821,7 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
>                                 FILE_WRITE_ATTRIBUTES, FILE_CREATE,
>                                 CREATE_NOT_FILE, mode,
>                                 NULL, &(int){SMB2_OP_MKDIR}, 1,
> -                               NULL, NULL, NULL);
> +                               NULL, NULL, NULL, NULL);
>  }
>
>  void
> @@ -836,7 +846,7 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
>                                  FILE_WRITE_ATTRIBUTES, FILE_CREATE,
>                                  CREATE_NOT_FILE, ACL_NO_MODE, &in_iov,
>                                  &(int){SMB2_OP_SET_INFO}, 1,
> -                                cfile, NULL, NULL);
> +                                cfile, NULL, NULL, NULL);
>         if (tmprc == 0)
>                 cifs_i->cifsAttrs = dosattrs;
>  }
> @@ -850,25 +860,26 @@ smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
>                                 DELETE, FILE_OPEN, CREATE_NOT_FILE,
>                                 ACL_NO_MODE, NULL,
>                                 &(int){SMB2_OP_RMDIR}, 1,
> -                               NULL, NULL, NULL);
> +                               NULL, NULL, NULL, NULL);
>  }
>
>  int
>  smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> -           struct cifs_sb_info *cifs_sb)
> +           struct cifs_sb_info *cifs_sb, struct dentry *dentry)
>  {
>         return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
>                                 CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT,
>                                 ACL_NO_MODE, NULL,
>                                 &(int){SMB2_OP_DELETE}, 1,
> -                               NULL, NULL, NULL);
> +                               NULL, NULL, NULL, dentry);
>  }
>
>  static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
>                               const char *from_name, const char *to_name,
>                               struct cifs_sb_info *cifs_sb,
>                               __u32 create_options, __u32 access,
> -                             int command, struct cifsFileInfo *cfile)
> +                             int command, struct cifsFileInfo *cfile,
> +                                 struct dentry *dentry)
>  {
>         struct kvec in_iov;
>         __le16 *smb2_to_name = NULL;
> @@ -883,7 +894,7 @@ static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
>         in_iov.iov_len = 2 * UniStrnlen((wchar_t *)smb2_to_name, PATH_MAX);
>         rc = smb2_compound_op(xid, tcon, cifs_sb, from_name, access,
>                               FILE_OPEN, create_options, ACL_NO_MODE,
> -                             &in_iov, &command, 1, cfile, NULL, NULL);
> +                             &in_iov, &command, 1, cfile, NULL, NULL, dentry);
>  smb2_rename_path:
>         kfree(smb2_to_name);
>         return rc;
> @@ -902,7 +913,7 @@ int smb2_rename_path(const unsigned int xid,
>         cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile);
>
>         return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> -                                 co, DELETE, SMB2_OP_RENAME, cfile);
> +                                 co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
>  }
>
>  int smb2_create_hardlink(const unsigned int xid,
> @@ -915,13 +926,14 @@ int smb2_create_hardlink(const unsigned int xid,
>
>         return smb2_set_path_attr(xid, tcon, from_name, to_name,
>                                   cifs_sb, co, FILE_READ_ATTRIBUTES,
> -                                 SMB2_OP_HARDLINK, NULL);
> +                                 SMB2_OP_HARDLINK, NULL, NULL);
>  }
>
>  int
>  smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
>                    const char *full_path, __u64 size,
> -                  struct cifs_sb_info *cifs_sb, bool set_alloc)
> +                  struct cifs_sb_info *cifs_sb, bool set_alloc,
> +                  struct dentry *dentry)
>  {
>         struct cifsFileInfo *cfile;
>         struct kvec in_iov;
> @@ -934,7 +946,7 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
>                                 FILE_WRITE_DATA, FILE_OPEN,
>                                 0, ACL_NO_MODE, &in_iov,
>                                 &(int){SMB2_OP_SET_EOF}, 1,
> -                               cfile, NULL, NULL);
> +                               cfile, NULL, NULL, dentry);
>  }
>
>  int
> @@ -963,7 +975,7 @@ smb2_set_file_info(struct inode *inode, const char *full_path,
>                               FILE_WRITE_ATTRIBUTES, FILE_OPEN,
>                               0, ACL_NO_MODE, &in_iov,
>                               &(int){SMB2_OP_SET_INFO}, 1,
> -                             cfile, NULL, NULL);
> +                             cfile, NULL, NULL, NULL);
>         cifs_put_tlink(tlink);
>         return rc;
>  }
> @@ -998,7 +1010,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
>                                       da, cd, co, ACL_NO_MODE, in_iov,
> -                                     cmds, 2, cfile, NULL, NULL);
> +                                     cmds, 2, cfile, NULL, NULL, NULL);
>                 if (!rc) {
>                         rc = smb311_posix_get_inode_info(&new, full_path,
>                                                          data, sb, xid);
> @@ -1008,7 +1020,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
>                                       da, cd, co, ACL_NO_MODE, in_iov,
> -                                     cmds, 2, cfile, NULL, NULL);
> +                                     cmds, 2, cfile, NULL, NULL, NULL);
>                 if (!rc) {
>                         rc = cifs_get_inode_info(&new, full_path,
>                                                  data, sb, xid, NULL);
> @@ -1036,7 +1048,7 @@ int smb2_query_reparse_point(const unsigned int xid,
>                               FILE_READ_ATTRIBUTES, FILE_OPEN,
>                               OPEN_REPARSE_POINT, ACL_NO_MODE, &in_iov,
>                               &(int){SMB2_OP_GET_REPARSE}, 1,
> -                             cfile, NULL, NULL);
> +                             cfile, NULL, NULL, NULL);
>         if (rc)
>                 goto out;
>
> diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
> index b3069911e9dd..221143788a1c 100644
> --- a/fs/smb/client/smb2proto.h
> +++ b/fs/smb/client/smb2proto.h
> @@ -75,7 +75,8 @@ int smb2_query_path_info(const unsigned int xid,
>                          struct cifs_open_info_data *data);
>  extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
>                               const char *full_path, __u64 size,
> -                             struct cifs_sb_info *cifs_sb, bool set_alloc);
> +                             struct cifs_sb_info *cifs_sb, bool set_alloc,
> +                                 struct dentry *dentry);
>  extern int smb2_set_file_info(struct inode *inode, const char *full_path,
>                               FILE_BASIC_INFO *buf, const unsigned int xid);
>  extern int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
> @@ -91,7 +92,8 @@ extern void smb2_mkdir_setinfo(struct inode *inode, const char *full_path,
>  extern int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
>                       const char *name, struct cifs_sb_info *cifs_sb);
>  extern int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon,
> -                      const char *name, struct cifs_sb_info *cifs_sb);
> +                      const char *name, struct cifs_sb_info *cifs_sb,
> +                          struct dentry *dentry);
>  int smb2_rename_path(const unsigned int xid,
>                      struct cifs_tcon *tcon,
>                      struct dentry *source_dentry,
> --
> 2.39.2
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ