* [PATCH 0/1] server stat data out of cache for the root handle
@ 2019-03-11 6:00 Ronnie Sahlberg
2019-03-11 6:00 ` [PATCH] cifs: cache FILE_ALL_INFO for the shared " Ronnie Sahlberg
0 siblings, 1 reply; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-03-11 6:00 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky
Steve,
Largely finished WIP patch for starting to cache metadata for the
shared root. This initial patch caches the FILE_ALL_INFO infolevel.
I should break this up into a series of smaller patches so that we can separate
out what is just standard boilerplate code and what is the important changes.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 6:00 [PATCH 0/1] server stat data out of cache for the root handle Ronnie Sahlberg
@ 2019-03-11 6:00 ` Ronnie Sahlberg
2019-03-11 12:02 ` ronnie sahlberg
2019-03-11 23:19 ` Pavel Shilovsky
0 siblings, 2 replies; 7+ messages in thread
From: Ronnie Sahlberg @ 2019-03-11 6:00 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Pavel Shilovsky, Ronnie Sahlberg
When we open the shared root handle also ask for FILE_ALL_INFORMATION since
we can do this at zero cost as part of a compound.
Cache this information as long as the lease is return and serve any
future requests from cache.
This allows us to serve "stat /<mountpoint>" directly from cache and avoid
a network roundtrip. Since clients ofthen need to do this quite a lot
this improve performance slightly.
As an example: xfstest generic/533 performs 43 stat operations on the root
of the share while it is run. Which are eliminated with this patch.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 3 ++
fs/cifs/smb2inode.c | 15 ++++---
fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
fs/cifs/smb2pdu.c | 12 +++---
fs/cifs/smb2proto.h | 3 ++
5 files changed, 116 insertions(+), 28 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f293e052e351..b8360ca221eb 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
struct cached_fid {
bool is_valid:1; /* Do we have a useable root fid */
+ bool file_all_info_is_valid:1;
+
struct kref refcount;
struct cifs_fid *fid;
struct mutex fid_mutex;
struct cifs_tcon *tcon;
struct work_struct lease_break;
+ struct smb2_file_all_info file_all_info;
};
/*
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index 01a76bccdb8d..b6e07e2eed10 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
rc = open_shroot(xid, tcon, &fid);
if (rc)
goto out;
- rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
- fid.volatile_fid, smb2_data);
+
+ if (tcon->crfid.file_all_info_is_valid) {
+ move_smb2_info_to_cifs(data,
+ &tcon->crfid.file_all_info);
+ } else {
+ rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
+ fid.volatile_fid, smb2_data);
+ if (!rc)
+ move_smb2_info_to_cifs(data, smb2_data);
+ }
close_shroot(&tcon->crfid);
- if (rc)
- goto out;
- move_smb2_info_to_cifs(data, smb2_data);
goto out;
}
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 085e91436da7..0d8bf87592ff 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
cfid->fid->volatile_fid);
cfid->is_valid = false;
+ cfid->file_all_info_is_valid = false;
}
}
@@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
*/
int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
{
- struct cifs_open_parms oparams;
- int rc;
- __le16 srch_path = 0; /* Null - since an open of top of share */
+ struct cifs_ses *ses = tcon->ses;
+ struct TCP_Server_Info *server = ses->server;
+ struct cifs_open_parms oparms;
+ struct smb2_create_rsp *o_rsp = NULL;
+ struct smb2_query_info_rsp *qi_rsp = NULL;
+ int resp_buftype[2];
+ struct smb_rqst rqst[2];
+ struct kvec rsp_iov[2];
+ struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
+ struct kvec qi_iov[1];
+ int rc, flags = 0;
+ __le16 utf16_path = 0; /* Null - since an open of top of share */
u8 oplock = SMB2_OPLOCK_LEVEL_II;
mutex_lock(&tcon->crfid.fid_mutex);
@@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
return 0;
}
- oparams.tcon = tcon;
- oparams.create_options = 0;
- oparams.desired_access = FILE_READ_ATTRIBUTES;
- oparams.disposition = FILE_OPEN;
- oparams.fid = pfid;
- oparams.reconnect = false;
-
- rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
- if (rc == 0) {
- memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
- tcon->crfid.tcon = tcon;
- tcon->crfid.is_valid = true;
- kref_init(&tcon->crfid.refcount);
- kref_get(&tcon->crfid.refcount);
- }
+ if (smb3_encryption_required(tcon))
+ flags |= CIFS_TRANSFORM_REQ;
+
+ memset(rqst, 0, sizeof(rqst));
+ resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
+ memset(rsp_iov, 0, sizeof(rsp_iov));
+
+ /* Open */
+ memset(&open_iov, 0, sizeof(open_iov));
+ rqst[0].rq_iov = open_iov;
+ rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
+
+ oparms.tcon = tcon;
+ oparms.create_options = 0;
+ oparms.desired_access = FILE_READ_ATTRIBUTES;
+ oparms.disposition = FILE_OPEN;
+ oparms.fid = pfid;
+ oparms.reconnect = false;
+
+ rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
+ if (rc)
+ goto oshr_exit;
+ smb2_set_next_command(tcon, &rqst[0]);
+
+ memset(&qi_iov, 0, sizeof(qi_iov));
+ rqst[1].rq_iov = qi_iov;
+ rqst[1].rq_nvec = 1;
+
+ rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
+ COMPOUND_FID, FILE_ALL_INFORMATION,
+ SMB2_O_INFO_FILE, 0,
+ sizeof(struct smb2_file_all_info) +
+ PATH_MAX * 2, 0, NULL);
+ if (rc)
+ goto oshr_exit;
+
+ smb2_set_related(&rqst[1]);
+
+ rc = compound_send_recv(xid, ses, flags, 2, rqst,
+ resp_buftype, rsp_iov);
+ if (rc)
+ goto oshr_exit;
+
+ o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
+ oparms.fid->persistent_fid = o_rsp->PersistentFileId;
+ oparms.fid->volatile_fid = o_rsp->VolatileFileId;
+#ifdef CONFIG_CIFS_DEBUG2
+ oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
+#endif /* CIFS_DEBUG2 */
+
+ if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
+ oplock = smb2_parse_lease_state(server, o_rsp,
+ &oparms.fid->epoch,
+ oparms.fid->lease_key);
+ else
+ goto oshr_exit;
+
+
+ memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
+ tcon->crfid.tcon = tcon;
+ tcon->crfid.is_valid = true;
+ kref_init(&tcon->crfid.refcount);
+ kref_get(&tcon->crfid.refcount);
+
+
+ qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
+ rc = smb2_validate_and_copy_iov(
+ le16_to_cpu(qi_rsp->OutputBufferOffset),
+ le32_to_cpu(qi_rsp->OutputBufferLength),
+ &rsp_iov[1], sizeof(struct smb2_file_all_info),
+ (char *)&tcon->crfid.file_all_info);
+ if (rc)
+ goto oshr_exit;
+ tcon->crfid.file_all_info_is_valid = 1;
+
+ oshr_exit:
mutex_unlock(&tcon->crfid.fid_mutex);
+ SMB2_open_free(&rqst[0]);
+ SMB2_query_info_free(&rqst[1]);
+ free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
+ free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
return rc;
}
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 60fbe306f604..cfe9fe41ccf5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
return buf;
}
-static __u8
-parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
- unsigned int *epoch, char *lease_key)
+__u8
+smb2_parse_lease_state(struct TCP_Server_Info *server,
+ struct smb2_create_rsp *rsp,
+ unsigned int *epoch, char *lease_key)
{
char *data_offset;
struct create_context *cc;
@@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
}
if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
- *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
- oparms->fid->lease_key);
+ *oplock = smb2_parse_lease_state(server, rsp,
+ &oparms->fid->epoch,
+ oparms->fid->lease_key);
else
*oplock = rsp->OplockLevel;
creat_exit:
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 87733b27a65f..72cc563c32fe 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
enum securityEnum);
+extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
+ struct smb2_create_rsp *rsp,
+ unsigned int *epoch, char *lease_key);
extern int smb3_encryption_required(const struct cifs_tcon *tcon);
extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
struct kvec *iov, unsigned int min_buf_size);
--
2.13.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 6:00 ` [PATCH] cifs: cache FILE_ALL_INFO for the shared " Ronnie Sahlberg
@ 2019-03-11 12:02 ` ronnie sahlberg
2019-03-11 15:38 ` Steve French
2019-03-11 23:19 ` Pavel Shilovsky
1 sibling, 1 reply; 7+ messages in thread
From: ronnie sahlberg @ 2019-03-11 12:02 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
As a followup discussion.
We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we
should change this to do the actual tracing in SMB2_open_init()
instead and catch ALL places where we try to open a file.
Though, we don't actually have the result of the operation in
_open_init() which means we may need to reconsider how we trace these
things.
As we will likely switch more and more things to be compounds, maybe
we should not have tracing in SMB2_open() and have it trace
trace_smb3_open_err()
Maybe we should trace "smb2_init : constructing compound"
and have trace_smb3_compound_error()
We restructure the tracing to be focused around compounds instead of
individual PDUs.
I.e. tracing in all the _init() functions, then tracing in
compound_send_recv() for success/error.
Tracing success/error on pdu/compound level instead of individual
open/close/read/... operations.
For individual non-compounded commands we can still treat these the
same. They are just trivial compounds of one single SMB2 PDU.
On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> When we open the shared root handle also ask for FILE_ALL_INFORMATION since
> we can do this at zero cost as part of a compound.
> Cache this information as long as the lease is return and serve any
> future requests from cache.
>
> This allows us to serve "stat /<mountpoint>" directly from cache and avoid
> a network roundtrip. Since clients ofthen need to do this quite a lot
> this improve performance slightly.
>
> As an example: xfstest generic/533 performs 43 stat operations on the root
> of the share while it is run. Which are eliminated with this patch.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsglob.h | 3 ++
> fs/cifs/smb2inode.c | 15 ++++---
> fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
> fs/cifs/smb2pdu.c | 12 +++---
> fs/cifs/smb2proto.h | 3 ++
> 5 files changed, 116 insertions(+), 28 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f293e052e351..b8360ca221eb 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
>
> struct cached_fid {
> bool is_valid:1; /* Do we have a useable root fid */
> + bool file_all_info_is_valid:1;
> +
> struct kref refcount;
> struct cifs_fid *fid;
> struct mutex fid_mutex;
> struct cifs_tcon *tcon;
> struct work_struct lease_break;
> + struct smb2_file_all_info file_all_info;
> };
>
> /*
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 01a76bccdb8d..b6e07e2eed10 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> rc = open_shroot(xid, tcon, &fid);
> if (rc)
> goto out;
> - rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> - fid.volatile_fid, smb2_data);
> +
> + if (tcon->crfid.file_all_info_is_valid) {
> + move_smb2_info_to_cifs(data,
> + &tcon->crfid.file_all_info);
> + } else {
> + rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> + fid.volatile_fid, smb2_data);
> + if (!rc)
> + move_smb2_info_to_cifs(data, smb2_data);
> + }
> close_shroot(&tcon->crfid);
> - if (rc)
> - goto out;
> - move_smb2_info_to_cifs(data, smb2_data);
> goto out;
> }
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 085e91436da7..0d8bf87592ff 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
> SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> cfid->fid->volatile_fid);
> cfid->is_valid = false;
> + cfid->file_all_info_is_valid = false;
> }
> }
>
> @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
> */
> int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> {
> - struct cifs_open_parms oparams;
> - int rc;
> - __le16 srch_path = 0; /* Null - since an open of top of share */
> + struct cifs_ses *ses = tcon->ses;
> + struct TCP_Server_Info *server = ses->server;
> + struct cifs_open_parms oparms;
> + struct smb2_create_rsp *o_rsp = NULL;
> + struct smb2_query_info_rsp *qi_rsp = NULL;
> + int resp_buftype[2];
> + struct smb_rqst rqst[2];
> + struct kvec rsp_iov[2];
> + struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> + struct kvec qi_iov[1];
> + int rc, flags = 0;
> + __le16 utf16_path = 0; /* Null - since an open of top of share */
> u8 oplock = SMB2_OPLOCK_LEVEL_II;
>
> mutex_lock(&tcon->crfid.fid_mutex);
> @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> return 0;
> }
>
> - oparams.tcon = tcon;
> - oparams.create_options = 0;
> - oparams.desired_access = FILE_READ_ATTRIBUTES;
> - oparams.disposition = FILE_OPEN;
> - oparams.fid = pfid;
> - oparams.reconnect = false;
> -
> - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> - if (rc == 0) {
> - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> - tcon->crfid.tcon = tcon;
> - tcon->crfid.is_valid = true;
> - kref_init(&tcon->crfid.refcount);
> - kref_get(&tcon->crfid.refcount);
> - }
> + if (smb3_encryption_required(tcon))
> + flags |= CIFS_TRANSFORM_REQ;
> +
> + memset(rqst, 0, sizeof(rqst));
> + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
> + memset(rsp_iov, 0, sizeof(rsp_iov));
> +
> + /* Open */
> + memset(&open_iov, 0, sizeof(open_iov));
> + rqst[0].rq_iov = open_iov;
> + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> +
> + oparms.tcon = tcon;
> + oparms.create_options = 0;
> + oparms.desired_access = FILE_READ_ATTRIBUTES;
> + oparms.disposition = FILE_OPEN;
> + oparms.fid = pfid;
> + oparms.reconnect = false;
> +
> + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
> + if (rc)
> + goto oshr_exit;
> + smb2_set_next_command(tcon, &rqst[0]);
> +
> + memset(&qi_iov, 0, sizeof(qi_iov));
> + rqst[1].rq_iov = qi_iov;
> + rqst[1].rq_nvec = 1;
> +
> + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
> + COMPOUND_FID, FILE_ALL_INFORMATION,
> + SMB2_O_INFO_FILE, 0,
> + sizeof(struct smb2_file_all_info) +
> + PATH_MAX * 2, 0, NULL);
> + if (rc)
> + goto oshr_exit;
> +
> + smb2_set_related(&rqst[1]);
> +
> + rc = compound_send_recv(xid, ses, flags, 2, rqst,
> + resp_buftype, rsp_iov);
> + if (rc)
> + goto oshr_exit;
> +
> + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> + oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> + oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> +#ifdef CONFIG_CIFS_DEBUG2
> + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> +#endif /* CIFS_DEBUG2 */
> +
> + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> + oplock = smb2_parse_lease_state(server, o_rsp,
> + &oparms.fid->epoch,
> + oparms.fid->lease_key);
> + else
> + goto oshr_exit;
> +
> +
> + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> + tcon->crfid.tcon = tcon;
> + tcon->crfid.is_valid = true;
> + kref_init(&tcon->crfid.refcount);
> + kref_get(&tcon->crfid.refcount);
> +
> +
> + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> + rc = smb2_validate_and_copy_iov(
> + le16_to_cpu(qi_rsp->OutputBufferOffset),
> + le32_to_cpu(qi_rsp->OutputBufferLength),
> + &rsp_iov[1], sizeof(struct smb2_file_all_info),
> + (char *)&tcon->crfid.file_all_info);
> + if (rc)
> + goto oshr_exit;
> + tcon->crfid.file_all_info_is_valid = 1;
> +
> + oshr_exit:
> mutex_unlock(&tcon->crfid.fid_mutex);
> + SMB2_open_free(&rqst[0]);
> + SMB2_query_info_free(&rqst[1]);
> + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> return rc;
> }
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 60fbe306f604..cfe9fe41ccf5 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
> return buf;
> }
>
> -static __u8
> -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
> - unsigned int *epoch, char *lease_key)
> +__u8
> +smb2_parse_lease_state(struct TCP_Server_Info *server,
> + struct smb2_create_rsp *rsp,
> + unsigned int *epoch, char *lease_key)
> {
> char *data_offset;
> struct create_context *cc;
> @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> }
>
> if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
> - oparms->fid->lease_key);
> + *oplock = smb2_parse_lease_state(server, rsp,
> + &oparms->fid->epoch,
> + oparms->fid->lease_key);
> else
> *oplock = rsp->OplockLevel;
> creat_exit:
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 87733b27a65f..72cc563c32fe 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
>
> extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
> enum securityEnum);
> +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
> + struct smb2_create_rsp *rsp,
> + unsigned int *epoch, char *lease_key);
> extern int smb3_encryption_required(const struct cifs_tcon *tcon);
> extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
> struct kvec *iov, unsigned int min_buf_size);
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 12:02 ` ronnie sahlberg
@ 2019-03-11 15:38 ` Steve French
2019-03-11 22:36 ` Pavel Shilovsky
0 siblings, 1 reply; 7+ messages in thread
From: Steve French @ 2019-03-11 15:38 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs, Pavel Shilovsky
Pavel and I had talked about adding tracing in smb2_compound_op for
enter/exit since it allows more granularity (and with dynamic tracing,
overhead is small). Thoughts?
On Mon, Mar 11, 2019 at 7:02 AM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> As a followup discussion.
>
> We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we
> should change this to do the actual tracing in SMB2_open_init()
> instead and catch ALL places where we try to open a file.
> Though, we don't actually have the result of the operation in
> _open_init() which means we may need to reconsider how we trace these
> things.
>
> As we will likely switch more and more things to be compounds, maybe
> we should not have tracing in SMB2_open() and have it trace
> trace_smb3_open_err()
>
> Maybe we should trace "smb2_init : constructing compound"
> and have trace_smb3_compound_error()
>
>
> We restructure the tracing to be focused around compounds instead of
> individual PDUs.
> I.e. tracing in all the _init() functions, then tracing in
> compound_send_recv() for success/error.
>
> Tracing success/error on pdu/compound level instead of individual
> open/close/read/... operations.
> For individual non-compounded commands we can still treat these the
> same. They are just trivial compounds of one single SMB2 PDU.
>
>
> On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > When we open the shared root handle also ask for FILE_ALL_INFORMATION since
> > we can do this at zero cost as part of a compound.
> > Cache this information as long as the lease is return and serve any
> > future requests from cache.
> >
> > This allows us to serve "stat /<mountpoint>" directly from cache and avoid
> > a network roundtrip. Since clients ofthen need to do this quite a lot
> > this improve performance slightly.
> >
> > As an example: xfstest generic/533 performs 43 stat operations on the root
> > of the share while it is run. Which are eliminated with this patch.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> > fs/cifs/cifsglob.h | 3 ++
> > fs/cifs/smb2inode.c | 15 ++++---
> > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
> > fs/cifs/smb2pdu.c | 12 +++---
> > fs/cifs/smb2proto.h | 3 ++
> > 5 files changed, 116 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index f293e052e351..b8360ca221eb 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
> >
> > struct cached_fid {
> > bool is_valid:1; /* Do we have a useable root fid */
> > + bool file_all_info_is_valid:1;
> > +
> > struct kref refcount;
> > struct cifs_fid *fid;
> > struct mutex fid_mutex;
> > struct cifs_tcon *tcon;
> > struct work_struct lease_break;
> > + struct smb2_file_all_info file_all_info;
> > };
> >
> > /*
> > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> > index 01a76bccdb8d..b6e07e2eed10 100644
> > --- a/fs/cifs/smb2inode.c
> > +++ b/fs/cifs/smb2inode.c
> > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> > rc = open_shroot(xid, tcon, &fid);
> > if (rc)
> > goto out;
> > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > - fid.volatile_fid, smb2_data);
> > +
> > + if (tcon->crfid.file_all_info_is_valid) {
> > + move_smb2_info_to_cifs(data,
> > + &tcon->crfid.file_all_info);
> > + } else {
> > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > + fid.volatile_fid, smb2_data);
> > + if (!rc)
> > + move_smb2_info_to_cifs(data, smb2_data);
> > + }
> > close_shroot(&tcon->crfid);
> > - if (rc)
> > - goto out;
> > - move_smb2_info_to_cifs(data, smb2_data);
> > goto out;
> > }
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 085e91436da7..0d8bf87592ff 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
> > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > cfid->fid->volatile_fid);
> > cfid->is_valid = false;
> > + cfid->file_all_info_is_valid = false;
> > }
> > }
> >
> > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
> > */
> > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > {
> > - struct cifs_open_parms oparams;
> > - int rc;
> > - __le16 srch_path = 0; /* Null - since an open of top of share */
> > + struct cifs_ses *ses = tcon->ses;
> > + struct TCP_Server_Info *server = ses->server;
> > + struct cifs_open_parms oparms;
> > + struct smb2_create_rsp *o_rsp = NULL;
> > + struct smb2_query_info_rsp *qi_rsp = NULL;
> > + int resp_buftype[2];
> > + struct smb_rqst rqst[2];
> > + struct kvec rsp_iov[2];
> > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> > + struct kvec qi_iov[1];
> > + int rc, flags = 0;
> > + __le16 utf16_path = 0; /* Null - since an open of top of share */
> > u8 oplock = SMB2_OPLOCK_LEVEL_II;
> >
> > mutex_lock(&tcon->crfid.fid_mutex);
> > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > return 0;
> > }
> >
> > - oparams.tcon = tcon;
> > - oparams.create_options = 0;
> > - oparams.desired_access = FILE_READ_ATTRIBUTES;
> > - oparams.disposition = FILE_OPEN;
> > - oparams.fid = pfid;
> > - oparams.reconnect = false;
> > -
> > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> > - if (rc == 0) {
> > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > - tcon->crfid.tcon = tcon;
> > - tcon->crfid.is_valid = true;
> > - kref_init(&tcon->crfid.refcount);
> > - kref_get(&tcon->crfid.refcount);
> > - }
> > + if (smb3_encryption_required(tcon))
> > + flags |= CIFS_TRANSFORM_REQ;
> > +
> > + memset(rqst, 0, sizeof(rqst));
> > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
> > + memset(rsp_iov, 0, sizeof(rsp_iov));
> > +
> > + /* Open */
> > + memset(&open_iov, 0, sizeof(open_iov));
> > + rqst[0].rq_iov = open_iov;
> > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> > +
> > + oparms.tcon = tcon;
> > + oparms.create_options = 0;
> > + oparms.desired_access = FILE_READ_ATTRIBUTES;
> > + oparms.disposition = FILE_OPEN;
> > + oparms.fid = pfid;
> > + oparms.reconnect = false;
> > +
> > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
> > + if (rc)
> > + goto oshr_exit;
> > + smb2_set_next_command(tcon, &rqst[0]);
> > +
> > + memset(&qi_iov, 0, sizeof(qi_iov));
> > + rqst[1].rq_iov = qi_iov;
> > + rqst[1].rq_nvec = 1;
> > +
> > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
> > + COMPOUND_FID, FILE_ALL_INFORMATION,
> > + SMB2_O_INFO_FILE, 0,
> > + sizeof(struct smb2_file_all_info) +
> > + PATH_MAX * 2, 0, NULL);
> > + if (rc)
> > + goto oshr_exit;
> > +
> > + smb2_set_related(&rqst[1]);
> > +
> > + rc = compound_send_recv(xid, ses, flags, 2, rqst,
> > + resp_buftype, rsp_iov);
> > + if (rc)
> > + goto oshr_exit;
> > +
> > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> > + oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> > + oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> > +#ifdef CONFIG_CIFS_DEBUG2
> > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> > +#endif /* CIFS_DEBUG2 */
> > +
> > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > + oplock = smb2_parse_lease_state(server, o_rsp,
> > + &oparms.fid->epoch,
> > + oparms.fid->lease_key);
> > + else
> > + goto oshr_exit;
> > +
> > +
> > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > + tcon->crfid.tcon = tcon;
> > + tcon->crfid.is_valid = true;
> > + kref_init(&tcon->crfid.refcount);
> > + kref_get(&tcon->crfid.refcount);
> > +
> > +
> > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > + rc = smb2_validate_and_copy_iov(
> > + le16_to_cpu(qi_rsp->OutputBufferOffset),
> > + le32_to_cpu(qi_rsp->OutputBufferLength),
> > + &rsp_iov[1], sizeof(struct smb2_file_all_info),
> > + (char *)&tcon->crfid.file_all_info);
> > + if (rc)
> > + goto oshr_exit;
> > + tcon->crfid.file_all_info_is_valid = 1;
> > +
> > + oshr_exit:
> > mutex_unlock(&tcon->crfid.fid_mutex);
> > + SMB2_open_free(&rqst[0]);
> > + SMB2_query_info_free(&rqst[1]);
> > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > return rc;
> > }
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 60fbe306f604..cfe9fe41ccf5 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
> > return buf;
> > }
> >
> > -static __u8
> > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
> > - unsigned int *epoch, char *lease_key)
> > +__u8
> > +smb2_parse_lease_state(struct TCP_Server_Info *server,
> > + struct smb2_create_rsp *rsp,
> > + unsigned int *epoch, char *lease_key)
> > {
> > char *data_offset;
> > struct create_context *cc;
> > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> > }
> >
> > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
> > - oparms->fid->lease_key);
> > + *oplock = smb2_parse_lease_state(server, rsp,
> > + &oparms->fid->epoch,
> > + oparms->fid->lease_key);
> > else
> > *oplock = rsp->OplockLevel;
> > creat_exit:
> > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> > index 87733b27a65f..72cc563c32fe 100644
> > --- a/fs/cifs/smb2proto.h
> > +++ b/fs/cifs/smb2proto.h
> > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
> >
> > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
> > enum securityEnum);
> > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
> > + struct smb2_create_rsp *rsp,
> > + unsigned int *epoch, char *lease_key);
> > extern int smb3_encryption_required(const struct cifs_tcon *tcon);
> > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
> > struct kvec *iov, unsigned int min_buf_size);
> > --
> > 2.13.6
> >
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 15:38 ` Steve French
@ 2019-03-11 22:36 ` Pavel Shilovsky
0 siblings, 0 replies; 7+ messages in thread
From: Pavel Shilovsky @ 2019-03-11 22:36 UTC (permalink / raw)
To: Steve French
Cc: ronnie sahlberg, Ronnie Sahlberg, linux-cifs, Pavel Shilovsky
I like the idea of tracing compound chain as one logical operation
because it correlates with a particular action in the user space.
Individual operations like SMB2_open() should also be traced as one
logical operation for the same reason.
With tracing we need to be able to match a particular system call with
logical operations (open, remove, setinfo) and PDU(s) (sid, tid,
mid(s)).
--
Best regards,
Pavel Shilovsky
пн, 11 мар. 2019 г. в 08:39, Steve French <smfrench@gmail.com>:
>
> Pavel and I had talked about adding tracing in smb2_compound_op for
> enter/exit since it allows more granularity (and with dynamic tracing,
> overhead is small). Thoughts?
>
> On Mon, Mar 11, 2019 at 7:02 AM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > As a followup discussion.
> >
> > We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we
> > should change this to do the actual tracing in SMB2_open_init()
> > instead and catch ALL places where we try to open a file.
> > Though, we don't actually have the result of the operation in
> > _open_init() which means we may need to reconsider how we trace these
> > things.
> >
> > As we will likely switch more and more things to be compounds, maybe
> > we should not have tracing in SMB2_open() and have it trace
> > trace_smb3_open_err()
> >
> > Maybe we should trace "smb2_init : constructing compound"
> > and have trace_smb3_compound_error()
> >
> >
> > We restructure the tracing to be focused around compounds instead of
> > individual PDUs.
> > I.e. tracing in all the _init() functions, then tracing in
> > compound_send_recv() for success/error.
> >
> > Tracing success/error on pdu/compound level instead of individual
> > open/close/read/... operations.
> > For individual non-compounded commands we can still treat these the
> > same. They are just trivial compounds of one single SMB2 PDU.
> >
> >
> > On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since
> > > we can do this at zero cost as part of a compound.
> > > Cache this information as long as the lease is return and serve any
> > > future requests from cache.
> > >
> > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid
> > > a network roundtrip. Since clients ofthen need to do this quite a lot
> > > this improve performance slightly.
> > >
> > > As an example: xfstest generic/533 performs 43 stat operations on the root
> > > of the share while it is run. Which are eliminated with this patch.
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > > fs/cifs/cifsglob.h | 3 ++
> > > fs/cifs/smb2inode.c | 15 ++++---
> > > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
> > > fs/cifs/smb2pdu.c | 12 +++---
> > > fs/cifs/smb2proto.h | 3 ++
> > > 5 files changed, 116 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index f293e052e351..b8360ca221eb 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
> > >
> > > struct cached_fid {
> > > bool is_valid:1; /* Do we have a useable root fid */
> > > + bool file_all_info_is_valid:1;
> > > +
> > > struct kref refcount;
> > > struct cifs_fid *fid;
> > > struct mutex fid_mutex;
> > > struct cifs_tcon *tcon;
> > > struct work_struct lease_break;
> > > + struct smb2_file_all_info file_all_info;
> > > };
> > >
> > > /*
> > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> > > index 01a76bccdb8d..b6e07e2eed10 100644
> > > --- a/fs/cifs/smb2inode.c
> > > +++ b/fs/cifs/smb2inode.c
> > > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> > > rc = open_shroot(xid, tcon, &fid);
> > > if (rc)
> > > goto out;
> > > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > > - fid.volatile_fid, smb2_data);
> > > +
> > > + if (tcon->crfid.file_all_info_is_valid) {
> > > + move_smb2_info_to_cifs(data,
> > > + &tcon->crfid.file_all_info);
> > > + } else {
> > > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > > + fid.volatile_fid, smb2_data);
> > > + if (!rc)
> > > + move_smb2_info_to_cifs(data, smb2_data);
> > > + }
> > > close_shroot(&tcon->crfid);
> > > - if (rc)
> > > - goto out;
> > > - move_smb2_info_to_cifs(data, smb2_data);
> > > goto out;
> > > }
> > >
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index 085e91436da7..0d8bf87592ff 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
> > > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > > cfid->fid->volatile_fid);
> > > cfid->is_valid = false;
> > > + cfid->file_all_info_is_valid = false;
> > > }
> > > }
> > >
> > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
> > > */
> > > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > > {
> > > - struct cifs_open_parms oparams;
> > > - int rc;
> > > - __le16 srch_path = 0; /* Null - since an open of top of share */
> > > + struct cifs_ses *ses = tcon->ses;
> > > + struct TCP_Server_Info *server = ses->server;
> > > + struct cifs_open_parms oparms;
> > > + struct smb2_create_rsp *o_rsp = NULL;
> > > + struct smb2_query_info_rsp *qi_rsp = NULL;
> > > + int resp_buftype[2];
> > > + struct smb_rqst rqst[2];
> > > + struct kvec rsp_iov[2];
> > > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> > > + struct kvec qi_iov[1];
> > > + int rc, flags = 0;
> > > + __le16 utf16_path = 0; /* Null - since an open of top of share */
> > > u8 oplock = SMB2_OPLOCK_LEVEL_II;
> > >
> > > mutex_lock(&tcon->crfid.fid_mutex);
> > > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > > return 0;
> > > }
> > >
> > > - oparams.tcon = tcon;
> > > - oparams.create_options = 0;
> > > - oparams.desired_access = FILE_READ_ATTRIBUTES;
> > > - oparams.disposition = FILE_OPEN;
> > > - oparams.fid = pfid;
> > > - oparams.reconnect = false;
> > > -
> > > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> > > - if (rc == 0) {
> > > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > > - tcon->crfid.tcon = tcon;
> > > - tcon->crfid.is_valid = true;
> > > - kref_init(&tcon->crfid.refcount);
> > > - kref_get(&tcon->crfid.refcount);
> > > - }
> > > + if (smb3_encryption_required(tcon))
> > > + flags |= CIFS_TRANSFORM_REQ;
> > > +
> > > + memset(rqst, 0, sizeof(rqst));
> > > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
> > > + memset(rsp_iov, 0, sizeof(rsp_iov));
> > > +
> > > + /* Open */
> > > + memset(&open_iov, 0, sizeof(open_iov));
> > > + rqst[0].rq_iov = open_iov;
> > > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> > > +
> > > + oparms.tcon = tcon;
> > > + oparms.create_options = 0;
> > > + oparms.desired_access = FILE_READ_ATTRIBUTES;
> > > + oparms.disposition = FILE_OPEN;
> > > + oparms.fid = pfid;
> > > + oparms.reconnect = false;
> > > +
> > > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
> > > + if (rc)
> > > + goto oshr_exit;
> > > + smb2_set_next_command(tcon, &rqst[0]);
> > > +
> > > + memset(&qi_iov, 0, sizeof(qi_iov));
> > > + rqst[1].rq_iov = qi_iov;
> > > + rqst[1].rq_nvec = 1;
> > > +
> > > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
> > > + COMPOUND_FID, FILE_ALL_INFORMATION,
> > > + SMB2_O_INFO_FILE, 0,
> > > + sizeof(struct smb2_file_all_info) +
> > > + PATH_MAX * 2, 0, NULL);
> > > + if (rc)
> > > + goto oshr_exit;
> > > +
> > > + smb2_set_related(&rqst[1]);
> > > +
> > > + rc = compound_send_recv(xid, ses, flags, 2, rqst,
> > > + resp_buftype, rsp_iov);
> > > + if (rc)
> > > + goto oshr_exit;
> > > +
> > > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> > > + oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> > > + oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> > > +#ifdef CONFIG_CIFS_DEBUG2
> > > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> > > +#endif /* CIFS_DEBUG2 */
> > > +
> > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > > + oplock = smb2_parse_lease_state(server, o_rsp,
> > > + &oparms.fid->epoch,
> > > + oparms.fid->lease_key);
> > > + else
> > > + goto oshr_exit;
> > > +
> > > +
> > > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > > + tcon->crfid.tcon = tcon;
> > > + tcon->crfid.is_valid = true;
> > > + kref_init(&tcon->crfid.refcount);
> > > + kref_get(&tcon->crfid.refcount);
> > > +
> > > +
> > > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > > + rc = smb2_validate_and_copy_iov(
> > > + le16_to_cpu(qi_rsp->OutputBufferOffset),
> > > + le32_to_cpu(qi_rsp->OutputBufferLength),
> > > + &rsp_iov[1], sizeof(struct smb2_file_all_info),
> > > + (char *)&tcon->crfid.file_all_info);
> > > + if (rc)
> > > + goto oshr_exit;
> > > + tcon->crfid.file_all_info_is_valid = 1;
> > > +
> > > + oshr_exit:
> > > mutex_unlock(&tcon->crfid.fid_mutex);
> > > + SMB2_open_free(&rqst[0]);
> > > + SMB2_query_info_free(&rqst[1]);
> > > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> > > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > > return rc;
> > > }
> > >
> > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > index 60fbe306f604..cfe9fe41ccf5 100644
> > > --- a/fs/cifs/smb2pdu.c
> > > +++ b/fs/cifs/smb2pdu.c
> > > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
> > > return buf;
> > > }
> > >
> > > -static __u8
> > > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
> > > - unsigned int *epoch, char *lease_key)
> > > +__u8
> > > +smb2_parse_lease_state(struct TCP_Server_Info *server,
> > > + struct smb2_create_rsp *rsp,
> > > + unsigned int *epoch, char *lease_key)
> > > {
> > > char *data_offset;
> > > struct create_context *cc;
> > > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> > > }
> > >
> > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
> > > - oparms->fid->lease_key);
> > > + *oplock = smb2_parse_lease_state(server, rsp,
> > > + &oparms->fid->epoch,
> > > + oparms->fid->lease_key);
> > > else
> > > *oplock = rsp->OplockLevel;
> > > creat_exit:
> > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> > > index 87733b27a65f..72cc563c32fe 100644
> > > --- a/fs/cifs/smb2proto.h
> > > +++ b/fs/cifs/smb2proto.h
> > > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
> > >
> > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
> > > enum securityEnum);
> > > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
> > > + struct smb2_create_rsp *rsp,
> > > + unsigned int *epoch, char *lease_key);
> > > extern int smb3_encryption_required(const struct cifs_tcon *tcon);
> > > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
> > > struct kvec *iov, unsigned int min_buf_size);
> > > --
> > > 2.13.6
> > >
>
>
>
> --
> Thanks,
>
> Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 6:00 ` [PATCH] cifs: cache FILE_ALL_INFO for the shared " Ronnie Sahlberg
2019-03-11 12:02 ` ronnie sahlberg
@ 2019-03-11 23:19 ` Pavel Shilovsky
2019-03-12 21:57 ` ronnie sahlberg
1 sibling, 1 reply; 7+ messages in thread
From: Pavel Shilovsky @ 2019-03-11 23:19 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French, Pavel Shilovsky
вс, 10 мар. 2019 г. в 23:01, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> When we open the shared root handle also ask for FILE_ALL_INFORMATION since
> we can do this at zero cost as part of a compound.
> Cache this information as long as the lease is return and serve any
> future requests from cache.
>
> This allows us to serve "stat /<mountpoint>" directly from cache and avoid
> a network roundtrip. Since clients ofthen need to do this quite a lot
> this improve performance slightly.
>
> As an example: xfstest generic/533 performs 43 stat operations on the root
> of the share while it is run. Which are eliminated with this patch.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsglob.h | 3 ++
> fs/cifs/smb2inode.c | 15 ++++---
> fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
> fs/cifs/smb2pdu.c | 12 +++---
> fs/cifs/smb2proto.h | 3 ++
> 5 files changed, 116 insertions(+), 28 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f293e052e351..b8360ca221eb 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
>
> struct cached_fid {
> bool is_valid:1; /* Do we have a useable root fid */
> + bool file_all_info_is_valid:1;
> +
> struct kref refcount;
> struct cifs_fid *fid;
> struct mutex fid_mutex;
> struct cifs_tcon *tcon;
> struct work_struct lease_break;
> + struct smb2_file_all_info file_all_info;
The structure contains Name[1] - length of 1 byte...
> };
>
> /*
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 01a76bccdb8d..b6e07e2eed10 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> rc = open_shroot(xid, tcon, &fid);
> if (rc)
> goto out;
> - rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> - fid.volatile_fid, smb2_data);
> +
> + if (tcon->crfid.file_all_info_is_valid) {
> + move_smb2_info_to_cifs(data,
> + &tcon->crfid.file_all_info);
> + } else {
> + rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> + fid.volatile_fid, smb2_data);
> + if (!rc)
> + move_smb2_info_to_cifs(data, smb2_data);
> + }
> close_shroot(&tcon->crfid);
> - if (rc)
> - goto out;
> - move_smb2_info_to_cifs(data, smb2_data);
> goto out;
> }
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 085e91436da7..0d8bf87592ff 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
> SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> cfid->fid->volatile_fid);
> cfid->is_valid = false;
> + cfid->file_all_info_is_valid = false;
> }
> }
>
> @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
> */
> int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> {
> - struct cifs_open_parms oparams;
> - int rc;
> - __le16 srch_path = 0; /* Null - since an open of top of share */
> + struct cifs_ses *ses = tcon->ses;
> + struct TCP_Server_Info *server = ses->server;
> + struct cifs_open_parms oparms;
> + struct smb2_create_rsp *o_rsp = NULL;
> + struct smb2_query_info_rsp *qi_rsp = NULL;
> + int resp_buftype[2];
> + struct smb_rqst rqst[2];
> + struct kvec rsp_iov[2];
> + struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> + struct kvec qi_iov[1];
> + int rc, flags = 0;
> + __le16 utf16_path = 0; /* Null - since an open of top of share */
> u8 oplock = SMB2_OPLOCK_LEVEL_II;
>
> mutex_lock(&tcon->crfid.fid_mutex);
> @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> return 0;
> }
>
> - oparams.tcon = tcon;
> - oparams.create_options = 0;
> - oparams.desired_access = FILE_READ_ATTRIBUTES;
> - oparams.disposition = FILE_OPEN;
> - oparams.fid = pfid;
> - oparams.reconnect = false;
> -
> - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> - if (rc == 0) {
> - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> - tcon->crfid.tcon = tcon;
> - tcon->crfid.is_valid = true;
> - kref_init(&tcon->crfid.refcount);
> - kref_get(&tcon->crfid.refcount);
> - }
> + if (smb3_encryption_required(tcon))
> + flags |= CIFS_TRANSFORM_REQ;
> +
> + memset(rqst, 0, sizeof(rqst));
> + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
> + memset(rsp_iov, 0, sizeof(rsp_iov));
> +
> + /* Open */
> + memset(&open_iov, 0, sizeof(open_iov));
> + rqst[0].rq_iov = open_iov;
> + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> +
> + oparms.tcon = tcon;
> + oparms.create_options = 0;
> + oparms.desired_access = FILE_READ_ATTRIBUTES;
> + oparms.disposition = FILE_OPEN;
> + oparms.fid = pfid;
> + oparms.reconnect = false;
> +
> + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
> + if (rc)
> + goto oshr_exit;
> + smb2_set_next_command(tcon, &rqst[0]);
> +
> + memset(&qi_iov, 0, sizeof(qi_iov));
> + rqst[1].rq_iov = qi_iov;
> + rqst[1].rq_nvec = 1;
> +
> + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
> + COMPOUND_FID, FILE_ALL_INFORMATION,
> + SMB2_O_INFO_FILE, 0,
> + sizeof(struct smb2_file_all_info) +
> + PATH_MAX * 2, 0, NULL);
...but OutputLenght is sizeof(struct smb2_file_all_info) + PATH_MAX * 2.
> + if (rc)
> + goto oshr_exit;
> +
> + smb2_set_related(&rqst[1]);
> +
> + rc = compound_send_recv(xid, ses, flags, 2, rqst,
> + resp_buftype, rsp_iov);
> + if (rc)
> + goto oshr_exit;
> +
> + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> + oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> + oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> +#ifdef CONFIG_CIFS_DEBUG2
> + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> +#endif /* CIFS_DEBUG2 */
> +
> + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> + oplock = smb2_parse_lease_state(server, o_rsp,
> + &oparms.fid->epoch,
> + oparms.fid->lease_key);
> + else
> + goto oshr_exit;
> +
> +
> + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> + tcon->crfid.tcon = tcon;
> + tcon->crfid.is_valid = true;
> + kref_init(&tcon->crfid.refcount);
> + kref_get(&tcon->crfid.refcount);
> +
> +
> + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> + rc = smb2_validate_and_copy_iov(
> + le16_to_cpu(qi_rsp->OutputBufferOffset),
> + le32_to_cpu(qi_rsp->OutputBufferLength),
> + &rsp_iov[1], sizeof(struct smb2_file_all_info),
> + (char *)&tcon->crfid.file_all_info);
so, the above call will cause buffer overflow. Please include PATH_MAX
* 2 bytes into the structure to hold the name or even make a pointer
for the info.
> + if (rc)
> + goto oshr_exit;
> + tcon->crfid.file_all_info_is_valid = 1;
> +
> + oshr_exit:
> mutex_unlock(&tcon->crfid.fid_mutex);
> + SMB2_open_free(&rqst[0]);
> + SMB2_query_info_free(&rqst[1]);
> + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> return rc;
> }
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 60fbe306f604..cfe9fe41ccf5 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
> return buf;
> }
>
> -static __u8
> -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
> - unsigned int *epoch, char *lease_key)
> +__u8
> +smb2_parse_lease_state(struct TCP_Server_Info *server,
> + struct smb2_create_rsp *rsp,
> + unsigned int *epoch, char *lease_key)
> {
> char *data_offset;
> struct create_context *cc;
> @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> }
>
> if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
> - oparms->fid->lease_key);
> + *oplock = smb2_parse_lease_state(server, rsp,
> + &oparms->fid->epoch,
> + oparms->fid->lease_key);
> else
> *oplock = rsp->OplockLevel;
> creat_exit:
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 87733b27a65f..72cc563c32fe 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
>
> extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
> enum securityEnum);
> +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
> + struct smb2_create_rsp *rsp,
> + unsigned int *epoch, char *lease_key);
> extern int smb3_encryption_required(const struct cifs_tcon *tcon);
> extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
> struct kvec *iov, unsigned int min_buf_size);
> --
> 2.13.6
>
In general we should probably need to set oplock/lease level on the
inode of the root, so the existing caching mechanism (see
cifs_inode_need_reval in inode.c) can handle stat calls, so they don't
reach to query_path_info(). But it seems to be out of scope of this
patch and might be done separately.
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: cache FILE_ALL_INFO for the shared root handle
2019-03-11 23:19 ` Pavel Shilovsky
@ 2019-03-12 21:57 ` ronnie sahlberg
0 siblings, 0 replies; 7+ messages in thread
From: ronnie sahlberg @ 2019-03-12 21:57 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Ronnie Sahlberg, linux-cifs, Steve French, Pavel Shilovsky
On Tue, Mar 12, 2019 at 9:19 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вс, 10 мар. 2019 г. в 23:01, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > When we open the shared root handle also ask for FILE_ALL_INFORMATION since
> > we can do this at zero cost as part of a compound.
> > Cache this information as long as the lease is return and serve any
> > future requests from cache.
> >
> > This allows us to serve "stat /<mountpoint>" directly from cache and avoid
> > a network roundtrip. Since clients ofthen need to do this quite a lot
> > this improve performance slightly.
> >
> > As an example: xfstest generic/533 performs 43 stat operations on the root
> > of the share while it is run. Which are eliminated with this patch.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> > fs/cifs/cifsglob.h | 3 ++
> > fs/cifs/smb2inode.c | 15 ++++---
> > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++---------
> > fs/cifs/smb2pdu.c | 12 +++---
> > fs/cifs/smb2proto.h | 3 ++
> > 5 files changed, 116 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index f293e052e351..b8360ca221eb 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses)
> >
> > struct cached_fid {
> > bool is_valid:1; /* Do we have a useable root fid */
> > + bool file_all_info_is_valid:1;
> > +
> > struct kref refcount;
> > struct cifs_fid *fid;
> > struct mutex fid_mutex;
> > struct cifs_tcon *tcon;
> > struct work_struct lease_break;
> > + struct smb2_file_all_info file_all_info;
>
> The structure contains Name[1] - length of 1 byte...
>
> > };
> >
> > /*
> > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> > index 01a76bccdb8d..b6e07e2eed10 100644
> > --- a/fs/cifs/smb2inode.c
> > +++ b/fs/cifs/smb2inode.c
> > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> > rc = open_shroot(xid, tcon, &fid);
> > if (rc)
> > goto out;
> > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > - fid.volatile_fid, smb2_data);
> > +
> > + if (tcon->crfid.file_all_info_is_valid) {
> > + move_smb2_info_to_cifs(data,
> > + &tcon->crfid.file_all_info);
> > + } else {
> > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
> > + fid.volatile_fid, smb2_data);
> > + if (!rc)
> > + move_smb2_info_to_cifs(data, smb2_data);
> > + }
> > close_shroot(&tcon->crfid);
> > - if (rc)
> > - goto out;
> > - move_smb2_info_to_cifs(data, smb2_data);
> > goto out;
> > }
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 085e91436da7..0d8bf87592ff 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref)
> > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > cfid->fid->volatile_fid);
> > cfid->is_valid = false;
> > + cfid->file_all_info_is_valid = false;
> > }
> > }
> >
> > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work)
> > */
> > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > {
> > - struct cifs_open_parms oparams;
> > - int rc;
> > - __le16 srch_path = 0; /* Null - since an open of top of share */
> > + struct cifs_ses *ses = tcon->ses;
> > + struct TCP_Server_Info *server = ses->server;
> > + struct cifs_open_parms oparms;
> > + struct smb2_create_rsp *o_rsp = NULL;
> > + struct smb2_query_info_rsp *qi_rsp = NULL;
> > + int resp_buftype[2];
> > + struct smb_rqst rqst[2];
> > + struct kvec rsp_iov[2];
> > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> > + struct kvec qi_iov[1];
> > + int rc, flags = 0;
> > + __le16 utf16_path = 0; /* Null - since an open of top of share */
> > u8 oplock = SMB2_OPLOCK_LEVEL_II;
> >
> > mutex_lock(&tcon->crfid.fid_mutex);
> > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> > return 0;
> > }
> >
> > - oparams.tcon = tcon;
> > - oparams.create_options = 0;
> > - oparams.desired_access = FILE_READ_ATTRIBUTES;
> > - oparams.disposition = FILE_OPEN;
> > - oparams.fid = pfid;
> > - oparams.reconnect = false;
> > -
> > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> > - if (rc == 0) {
> > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > - tcon->crfid.tcon = tcon;
> > - tcon->crfid.is_valid = true;
> > - kref_init(&tcon->crfid.refcount);
> > - kref_get(&tcon->crfid.refcount);
> > - }
> > + if (smb3_encryption_required(tcon))
> > + flags |= CIFS_TRANSFORM_REQ;
> > +
> > + memset(rqst, 0, sizeof(rqst));
> > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER;
> > + memset(rsp_iov, 0, sizeof(rsp_iov));
> > +
> > + /* Open */
> > + memset(&open_iov, 0, sizeof(open_iov));
> > + rqst[0].rq_iov = open_iov;
> > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
> > +
> > + oparms.tcon = tcon;
> > + oparms.create_options = 0;
> > + oparms.desired_access = FILE_READ_ATTRIBUTES;
> > + oparms.disposition = FILE_OPEN;
> > + oparms.fid = pfid;
> > + oparms.reconnect = false;
> > +
> > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
> > + if (rc)
> > + goto oshr_exit;
> > + smb2_set_next_command(tcon, &rqst[0]);
> > +
> > + memset(&qi_iov, 0, sizeof(qi_iov));
> > + rqst[1].rq_iov = qi_iov;
> > + rqst[1].rq_nvec = 1;
> > +
> > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID,
> > + COMPOUND_FID, FILE_ALL_INFORMATION,
> > + SMB2_O_INFO_FILE, 0,
> > + sizeof(struct smb2_file_all_info) +
> > + PATH_MAX * 2, 0, NULL);
>
> ...but OutputLenght is sizeof(struct smb2_file_all_info) + PATH_MAX * 2.
>
> > + if (rc)
> > + goto oshr_exit;
> > +
> > + smb2_set_related(&rqst[1]);
> > +
> > + rc = compound_send_recv(xid, ses, flags, 2, rqst,
> > + resp_buftype, rsp_iov);
> > + if (rc)
> > + goto oshr_exit;
> > +
> > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> > + oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> > + oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> > +#ifdef CONFIG_CIFS_DEBUG2
> > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> > +#endif /* CIFS_DEBUG2 */
> > +
> > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > + oplock = smb2_parse_lease_state(server, o_rsp,
> > + &oparms.fid->epoch,
> > + oparms.fid->lease_key);
> > + else
> > + goto oshr_exit;
> > +
> > +
> > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> > + tcon->crfid.tcon = tcon;
> > + tcon->crfid.is_valid = true;
> > + kref_init(&tcon->crfid.refcount);
> > + kref_get(&tcon->crfid.refcount);
> > +
> > +
> > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > + rc = smb2_validate_and_copy_iov(
> > + le16_to_cpu(qi_rsp->OutputBufferOffset),
> > + le32_to_cpu(qi_rsp->OutputBufferLength),
> > + &rsp_iov[1], sizeof(struct smb2_file_all_info),
> > + (char *)&tcon->crfid.file_all_info);
>
> so, the above call will cause buffer overflow. Please include PATH_MAX
> * 2 bytes into the structure to hold the name or even make a pointer
> for the info.
Thanks.
I changed it to clamp the length to sizeof(struct smb2_file_all_info)
since we never actually need or use the name.
>
> > + if (rc)
> > + goto oshr_exit;
> > + tcon->crfid.file_all_info_is_valid = 1;
> > +
> > + oshr_exit:
> > mutex_unlock(&tcon->crfid.fid_mutex);
> > + SMB2_open_free(&rqst[0]);
> > + SMB2_query_info_free(&rqst[1]);
> > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > return rc;
> > }
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 60fbe306f604..cfe9fe41ccf5 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid)
> > return buf;
> > }
> >
> > -static __u8
> > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
> > - unsigned int *epoch, char *lease_key)
> > +__u8
> > +smb2_parse_lease_state(struct TCP_Server_Info *server,
> > + struct smb2_create_rsp *rsp,
> > + unsigned int *epoch, char *lease_key)
> > {
> > char *data_offset;
> > struct create_context *cc;
> > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> > }
> >
> > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
> > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch,
> > - oparms->fid->lease_key);
> > + *oplock = smb2_parse_lease_state(server, rsp,
> > + &oparms->fid->epoch,
> > + oparms->fid->lease_key);
> > else
> > *oplock = rsp->OplockLevel;
> > creat_exit:
> > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> > index 87733b27a65f..72cc563c32fe 100644
> > --- a/fs/cifs/smb2proto.h
> > +++ b/fs/cifs/smb2proto.h
> > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
> >
> > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
> > enum securityEnum);
> > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server,
> > + struct smb2_create_rsp *rsp,
> > + unsigned int *epoch, char *lease_key);
> > extern int smb3_encryption_required(const struct cifs_tcon *tcon);
> > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length,
> > struct kvec *iov, unsigned int min_buf_size);
> > --
> > 2.13.6
> >
>
> In general we should probably need to set oplock/lease level on the
> inode of the root, so the existing caching mechanism (see
> cifs_inode_need_reval in inode.c) can handle stat calls, so they don't
> reach to query_path_info(). But it seems to be out of scope of this
> patch and might be done separately.
>
> --
> Best regards,
> Pavel Shilovsky
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-12 21:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-11 6:00 [PATCH 0/1] server stat data out of cache for the root handle Ronnie Sahlberg
2019-03-11 6:00 ` [PATCH] cifs: cache FILE_ALL_INFO for the shared " Ronnie Sahlberg
2019-03-11 12:02 ` ronnie sahlberg
2019-03-11 15:38 ` Steve French
2019-03-11 22:36 ` Pavel Shilovsky
2019-03-11 23:19 ` Pavel Shilovsky
2019-03-12 21:57 ` ronnie sahlberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).