| 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: <CAFTVevUYbPCTQs7V5EcqaRy1B3kpFwj+NKCemUamDa0P10QVUg@mail.gmail.com>
Date: Wed, 6 Mar 2024 09:49:46 +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 3/3] smb: client: retry compound request without reusing lease
Patch 1 of this patch series
https://lore.kernel.org/all/20240306034353.190039-1-meetakshisetiyaoss@gmail.com/
aims to fix a customer reported bug by reusing lease key in unlink,
rename and set_path_size compound operations on the smb client. The
bug, its implications and reproduction has been described in the
commit message of the patch. In short, unlink, rename and
set_path_size operations can cause the server to send lease breaks to
the same client, on the same connection which hurts performance.
The aim is to have a fix in place for this problem without regressing
existing behaviour. Also, the proposed changes should go in smaller
batches so that they can be backported with relative ease. Patch 1
regressed a few operations on hardlinks (eg., xfstests generic 002,
013). As per MS-SMB2, lease keys are associated with the file name.
Linux cifs client maintains lease keys with the inode. If the file
has hardlinks, it is possible that the lease for a file be wrongly
reused for an operation on the hardlink or vice versa. In these
cases, the mentioned compound operations fail with
STATUS_INVALID_PARAMETER.
A simple fix for the regressions would be to have a two-phased
approach and resend the compound op request again without the lease
key if STATUS_INVALID_PARAMETER is received. This would help patch 1
fix the original issue. Fix(es) for the hardlink-leasekey problem can
come in the next batch.
On Wed, Mar 6, 2024 at 9:14 AM <meetakshisetiyaoss@...il.com> wrote:
>
> From: Meetakshi Setiya <msetiya@...rosoft.com>
>
> There is a shortcoming in the current implementation of the file
> lease mechanism exposed when the lease keys were attempted to be
> reused for unlink, rename and set_path_size operations for a client. As
> per MS-SMB2, lease keys are associated with the file name. Linux smb
> client maintains lease keys with the inode. If the file has any hardlinks,
> it is possible that the lease for a file be wrongly reused for an
> operation on the hardlink or vice versa. In these cases, the mentioned
> compound operations fail with STATUS_INVALID_PARAMETER.
> This patch adds a fallback to the old mechanism of not sending any
> lease with these compound operations if the request with lease key fails
> with STATUS_INVALID_PARAMETER.
> Resending the same request without lease key should not hurt any
> functionality, but might impact performance especially in cases where
> the error is not because of the usage of wrong lease key and we might
> end up doing an extra roundtrip.
>
> Signed-off-by: Meetakshi Setiya <msetiya@...rosoft.com>
> ---
> fs/smb/client/smb2inode.c | 41 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 429d83d31280..f697c14cd8c6 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -154,6 +154,17 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> }
>
> /* if there is an existing lease, reuse it */
> +
> + /*
> + * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2,
> + * lease keys are associated with the filepath. We are maintaining lease keys
> + * with the inode on the client. If the file has hardlinks, it is possible
> + * that the lease for a file be reused for an operation on its hardlink or
> + * vice versa.
> + * As a workaround, send request using an existing lease key and if the server
> + * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request
> + * again without the lease.
> + */
> if (dentry) {
> inode = d_inode(dentry);
> if (CIFS_I(inode)->lease_granted && server->ops->get_lease_key) {
> @@ -874,11 +885,20 @@ int
> smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
> struct cifs_sb_info *cifs_sb, struct dentry *dentry)
> {
> - return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN,
> + int rc = 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, dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = 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);
> + }
> + return rc;
> }
>
> static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon,
> @@ -919,8 +939,14 @@ int smb2_rename_path(const unsigned int xid,
> drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb);
> 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,
> + int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> co, DELETE, SMB2_OP_RENAME, cfile, source_dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb,
> + co, DELETE, SMB2_OP_RENAME, cfile, NULL);
> + }
> + return rc;
> }
>
> int smb2_create_hardlink(const unsigned int xid,
> @@ -949,11 +975,20 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> in_iov.iov_base = &eof;
> in_iov.iov_len = sizeof(eof);
> cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> - return smb2_compound_op(xid, tcon, cifs_sb, full_path,
> + int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> FILE_WRITE_DATA, FILE_OPEN,
> 0, ACL_NO_MODE, &in_iov,
> &(int){SMB2_OP_SET_EOF}, 1,
> cfile, NULL, NULL, dentry);
> + if (rc == -EINVAL) {
> + cifs_dbg(FYI, "invalid lease key, resending request without lease");
> + rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> + FILE_WRITE_DATA, FILE_OPEN,
> + 0, ACL_NO_MODE, &in_iov,
> + &(int){SMB2_OP_SET_EOF}, 1,
> + cfile, NULL, NULL, NULL);
> + }
> + return rc;
> }
>
> int
> --
> 2.39.2
>
Powered by blists - more mailing lists