| 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
| ||
|
Message-ID: <CAFTVevXHstK=Vh4kGu-m1FnLL9FmVy4wJFjU6mdx2zMDAnBfMw@mail.gmail.com>
Date: Fri, 9 Feb 2024 18:59:08 +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 2/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 1 of this patch series
https://lore.kernel.org/linux-cifs/20240209131552.471765-1-meetakshisetiyaoss@gmail.com/
The fix for 002 and 013 is in part 3 of this patch series
https://lore.kernel.org/linux-cifs/20240209131552.471765-3-meetakshisetiyaoss@gmail.com/
Thanks
Meetakshi
On Fri, Feb 9, 2024 at 6:46 PM <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 f6b4acdcdeb3..baeed01d356b 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 f995b766b177..e108964950b5 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -406,7 +406,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);
> @@ -442,7 +443,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 8121b5b1aa22..c3e86876a2a8 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) {
> @@ -2799,7 +2799,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;
> @@ -2850,7 +2850,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);
> @@ -2940,7 +2940,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;
> }
> @@ -3107,7 +3107,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