From: Pavel Shilovsky <piastryyy@gmail.com>
To: Ronnie Sahlberg <lsahlber@redhat.com>
Cc: linux-cifs <linux-cifs@vger.kernel.org>
Subject: Re: [PATCH 1/3] cifs: prepare SMB2_query_directory to be used with compounding
Date: Mon, 23 Dec 2019 15:48:50 -0800 [thread overview]
Message-ID: <CAKywueQ40hEMH_X9M+ip6ftwO-is9w-Yv6h4vxSzD_F=h-kG1w@mail.gmail.com> (raw)
In-Reply-To: <20191204225410.17514-2-lsahlber@redhat.com>
ср, 4 дек. 2019 г. в 14:54, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/smb2pdu.c | 108 +++++++++++++++++++++++++++++++++++-----------------
> fs/cifs/smb2pdu.h | 2 +
> fs/cifs/smb2proto.h | 5 +++
> 3 files changed, 80 insertions(+), 35 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index ed77f94dbf1d..df903931590e 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -4214,56 +4214,36 @@ num_entries(char *bufstart, char *end_of_buf, char **lastentry, size_t size)
> /*
> * Readdir/FindFirst
> */
> -int
> -SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> - u64 persistent_fid, u64 volatile_fid, int index,
> - struct cifs_search_info *srch_inf)
> +int SMB2_query_directory_init(const unsigned int xid,
> + struct cifs_tcon *tcon, struct smb_rqst *rqst,
> + u64 persistent_fid, u64 volatile_fid,
> + int index, int info_level)
> {
> - struct smb_rqst rqst;
> + struct TCP_Server_Info *server = tcon->ses->server;
> struct smb2_query_directory_req *req;
> - struct smb2_query_directory_rsp *rsp = NULL;
> - struct kvec iov[2];
> - struct kvec rsp_iov;
> - int rc = 0;
> - int len;
> - int resp_buftype = CIFS_NO_BUFFER;
> unsigned char *bufptr;
> - struct TCP_Server_Info *server;
> - struct cifs_ses *ses = tcon->ses;
> __le16 asteriks = cpu_to_le16('*');
> - char *end_of_smb;
> unsigned int output_size = CIFSMaxBufSize;
I think we need to account for compound responses here. The response
buffer size is MAX_SMB2_HDR_SIZE + CIFSMaxBufSize, where
MAX_SMB2_HDR_SIZE is 52 transform hdr + 64 hdr + 88 create rsp. While
output_size being CIFSMaxBufSize worked for non-compounded query
directory responses it may not fit into buffer for compounded ones
when encryption is used.
It seem like we need to set it to CIFSMaxBufSize -
MAX_SMB2_CREATE_RESPONSE_SIZE - MAX_SMB2_CLOSE_RESPONSE_SIZE like to
do in smb2_query_eas().
I found other occurrences of the similar issue in smb2_query_symlink()
and smb2_ioctl_query_info(): SMB2_ioctl_init() should use
CIFSMaxBufSize - MAX_SMB2_CREATE_RESPONSE_SIZE -
MAX_SMB2_CLOSE_RESPONSE_SIZE too instead of CIFSMaxBufSize. Could you
fix those together with this one?
> - size_t info_buf_size;
> - int flags = 0;
> unsigned int total_len;
> -
> - if (ses && (ses->server))
> - server = ses->server;
> - else
> - return -EIO;
> + struct kvec *iov = rqst->rq_iov;
> + int len, rc;
>
> rc = smb2_plain_req_init(SMB2_QUERY_DIRECTORY, tcon, (void **) &req,
> &total_len);
> if (rc)
> return rc;
>
> - if (smb3_encryption_required(tcon))
> - flags |= CIFS_TRANSFORM_REQ;
> -
> - switch (srch_inf->info_level) {
> + switch (info_level) {
> case SMB_FIND_FILE_DIRECTORY_INFO:
> req->FileInformationClass = FILE_DIRECTORY_INFORMATION;
> - info_buf_size = sizeof(FILE_DIRECTORY_INFO) - 1;
> break;
> case SMB_FIND_FILE_ID_FULL_DIR_INFO:
> req->FileInformationClass = FILEID_FULL_DIRECTORY_INFORMATION;
> - info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO) - 1;
> break;
> default:
> cifs_tcon_dbg(VFS, "info level %u isn't supported\n",
> - srch_inf->info_level);
> - rc = -EINVAL;
> - goto qdir_exit;
> + info_level);
> + return -EINVAL;
> }
>
> req->FileIndex = cpu_to_le32(index);
> @@ -4292,15 +4272,56 @@ SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> iov[1].iov_base = (char *)(req->Buffer);
> iov[1].iov_len = len;
>
> + trace_smb3_query_dir_enter(xid, persistent_fid, tcon->tid,
> + tcon->ses->Suid, index, output_size);
> +
> + return 0;
> +}
> +
> +void SMB2_query_directory_free(struct smb_rqst *rqst)
> +{
> + if (rqst && rqst->rq_iov) {
> + cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request */
> + }
> +}
> +
> +int
> +SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> + u64 persistent_fid, u64 volatile_fid, int index,
> + struct cifs_search_info *srch_inf)
> +{
> + struct smb_rqst rqst;
> + struct kvec iov[SMB2_QUERY_DIRECTORY_IOV_SIZE];
> + struct smb2_query_directory_rsp *rsp = NULL;
> + int resp_buftype = CIFS_NO_BUFFER;
> + struct kvec rsp_iov;
> + int rc = 0;
> + struct TCP_Server_Info *server;
> + struct cifs_ses *ses = tcon->ses;
> + char *end_of_smb;
> + size_t info_buf_size;
> + int flags = 0;
> +
> + if (ses && (ses->server))
> + server = ses->server;
> + else
> + return -EIO;
> +
> + if (smb3_encryption_required(tcon))
> + flags |= CIFS_TRANSFORM_REQ;
> +
> memset(&rqst, 0, sizeof(struct smb_rqst));
> + memset(&iov, 0, sizeof(iov));
> rqst.rq_iov = iov;
> - rqst.rq_nvec = 2;
> + rqst.rq_nvec = SMB2_QUERY_DIRECTORY_IOV_SIZE;
>
> - trace_smb3_query_dir_enter(xid, persistent_fid, tcon->tid,
> - tcon->ses->Suid, index, output_size);
> + rc = SMB2_query_directory_init(xid, tcon, &rqst, persistent_fid,
> + volatile_fid, index,
> + srch_inf->info_level);
> + if (rc)
> + goto qdir_exit;
>
> rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags, &rsp_iov);
> - cifs_small_buf_release(req);
> rsp = (struct smb2_query_directory_rsp *)rsp_iov.iov_base;
>
> if (rc) {
> @@ -4318,6 +4339,20 @@ SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> goto qdir_exit;
> }
>
> + switch (srch_inf->info_level) {
> + case SMB_FIND_FILE_DIRECTORY_INFO:
> + info_buf_size = sizeof(FILE_DIRECTORY_INFO) - 1;
> + break;
> + case SMB_FIND_FILE_ID_FULL_DIR_INFO:
> + info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO) - 1;
> + break;
> + default:
> + cifs_tcon_dbg(VFS, "info level %u isn't supported\n",
> + srch_inf->info_level);
> + rc = -EINVAL;
> + goto qdir_exit;
> + }
> +
> rc = smb2_validate_iov(le16_to_cpu(rsp->OutputBufferOffset),
> le32_to_cpu(rsp->OutputBufferLength), &rsp_iov,
> info_buf_size);
> @@ -4353,11 +4388,14 @@ SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> else
> cifs_tcon_dbg(VFS, "illegal search buffer type\n");
>
> + rsp = NULL;
minor: the subsequent patch removes this assignment, probably better
to not do it here.
> + resp_buftype = CIFS_NO_BUFFER;
> +
> trace_smb3_query_dir_done(xid, persistent_fid, tcon->tid,
> tcon->ses->Suid, index, srch_inf->entries_in_buffer);
> - return rc;
>
> qdir_exit:
> + SMB2_query_directory_free(&rqst);
> free_rsp_buf(resp_buftype, rsp);
> return rc;
> }
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index f264e1d36fe1..caf323be0d7f 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -1272,6 +1272,8 @@ struct smb2_echo_rsp {
> #define SMB2_INDEX_SPECIFIED 0x04
> #define SMB2_REOPEN 0x10
>
> +#define SMB2_QUERY_DIRECTORY_IOV_SIZE 2
> +
> struct smb2_query_directory_req {
> struct smb2_sync_hdr sync_hdr;
> __le16 StructureSize; /* Must be 33 */
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index d21a5fcc8d06..ba48ce9af620 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -194,6 +194,11 @@ extern int SMB2_echo(struct TCP_Server_Info *server);
> extern int SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> u64 persistent_fid, u64 volatile_fid, int index,
> struct cifs_search_info *srch_inf);
> +extern int SMB2_query_directory_init(unsigned int xid, struct cifs_tcon *tcon,
> + struct smb_rqst *rqst,
> + u64 persistent_fid, u64 volatile_fid,
> + int index, int info_level);
> +extern void SMB2_query_directory_free(struct smb_rqst *rqst);
> extern int SMB2_set_eof(const unsigned int xid, struct cifs_tcon *tcon,
> u64 persistent_fid, u64 volatile_fid, u32 pid,
> __le64 *eof);
> --
> 2.13.6
>
Other than the note about buffer sizes above the series looks good at
the first glance.
--
Best regards,
Pavel Shilovsky
next prev parent reply other threads:[~2019-12-23 23:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-04 22:54 [PATCH 0/3] use compounding to speed up readdir() Ronnie Sahlberg
2019-12-04 22:54 ` [PATCH 1/3] cifs: prepare SMB2_query_directory to be used with compounding Ronnie Sahlberg
2019-12-23 23:48 ` Pavel Shilovsky [this message]
2020-01-08 3:09 ` ronnie sahlberg
2019-12-04 22:54 ` [PATCH 2/3] cifs: create a helper function to parse the query-directory response buffer Ronnie Sahlberg
2019-12-04 22:54 ` [PATCH 3/3] cifs: use compounding for open and first query-dir for readdir() Ronnie Sahlberg
-- strict thread matches above, loose matches on Subject: below --
2019-12-02 7:28 [PATCH 0/3] WIP start using compounding " Ronnie Sahlberg
2019-12-02 7:28 ` [PATCH 1/3] cifs: prepare SMB2_query_directory to be used with compounding Ronnie Sahlberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKywueQ40hEMH_X9M+ip6ftwO-is9w-Yv6h4vxSzD_F=h-kG1w@mail.gmail.com' \
--to=piastryyy@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=lsahlber@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.