Hi Ronnie,
Thanks for the great work on compounding! My comments bellow.
Ronnie Sahlberg <lsahlber@xxxxxxxxxx> writes:
> This is used for SMB3 encryption and compounded requests.
> The first rqst begins with a smb3 transform header as the first iov.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> ---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/smb2ops.c | 155 ++++++++++++++++++++++++++--------------------------
> fs/cifs/transport.c | 24 ++++++--
> 3 files changed, 100 insertions(+), 81 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index ff71fbd619bf..0f486dac8e69 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -452,7 +452,7 @@ struct smb_version_operations {
> int (*init_transform_rq)(struct TCP_Server_Info *, struct smb_rqst *,
> struct smb_rqst *);
> /* free transform request */
> - void (*free_transform_rq)(struct smb_rqst *);
> + void (*free_transform_rq)(int num_rqst, struct smb_rqst *);
> int (*is_transform_hdr)(void *buf);
> int (*receive_transform)(struct TCP_Server_Info *,
> struct mid_q_entry **);
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index c3648e9b5ec7..240267378baf 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -2191,30 +2191,51 @@ static inline void smb2_sg_set_buf(struct scatterlist *sg, const void *buf,
> * rqst->rq_iov[1+] data to be encrypted/decrypted
> */
> static struct scatterlist *
> -init_sg(struct smb_rqst *rqst, u8 *sign)
> +init_sg(int num_rqst, struct smb_rqst *rqst, u8 *sign)
> {
> - unsigned int sg_len = rqst->rq_nvec + rqst->rq_npages + 1;
> - unsigned int assoc_data_len = sizeof(struct smb2_transform_hdr) - 20;
> + unsigned int sg_len;
> struct scatterlist *sg;
> unsigned int i;
> unsigned int j;
> + unsigned int idx = 0;
> +
> + sg_len = 1;
> + for (i = 0; i < num_rqst; i++)
> + sg_len += rqst[i].rq_nvec + rqst[i].rq_npages;
>
> sg = kmalloc_array(sg_len, sizeof(struct scatterlist), GFP_KERNEL);
> if (!sg)
> return NULL;
>
> sg_init_table(sg, sg_len);
> - smb2_sg_set_buf(&sg[0], rqst->rq_iov[0].iov_base + 20, assoc_data_len);
> - for (i = 1; i < rqst->rq_nvec; i++)
> - smb2_sg_set_buf(&sg[i], rqst->rq_iov[i].iov_base,
> - rqst->rq_iov[i].iov_len);
> - for (j = 0; i < sg_len - 1; i++, j++) {
> - unsigned int len, offset;
> + for (i = 0; i < num_rqst; i++) {
> + /* the first rqst has a transform header where the first 20
> + * bytes are not part of the encrypted blob
> + */
> + // XXX tmp
> + if (rqst[i].rq_iov == NULL)
> + continue;
Is this a temporary check as per the comment above? If so, please remove
it. Otherwise, could you use an unlikely() there?
> + if (i == 0)
> + smb2_sg_set_buf(&sg[idx++],
> + rqst[i].rq_iov[0].iov_base + 20,
> + rqst[i].rq_iov[0].iov_len - 20);
I know we only support a few requests (MAX_COMPOUND) and this check would
wouldn't hurt that much. But what about removing it entirely and just do
smb2_sg_set_buf(rqst[0].rq_iov[0]...) before the loop and 'i' starting
at 1?
> + else
> + smb2_sg_set_buf(&sg[idx++],
> + rqst[i].rq_iov[0].iov_base,
> + rqst[i].rq_iov[0].iov_len);
> +
> + for (j = 1; j < rqst[i].rq_nvec; j++)
> + smb2_sg_set_buf(&sg[idx++], rqst[i].rq_iov[j].iov_base,
> + rqst[i].rq_iov[j].iov_len);
> +
> + for (j = 0; j < rqst[i].rq_npages; j++) {
> + unsigned int len, offset;
>
> - rqst_page_get_length(rqst, j, &len, &offset);
> - sg_set_page(&sg[i], rqst->rq_pages[j], len, offset);
> + rqst_page_get_length(&rqst[i], j, &len, &offset);
> + sg_set_page(&sg[idx++], rqst[i].rq_pages[j], len, offset);
> + }
> }
> - smb2_sg_set_buf(&sg[sg_len - 1], sign, SMB2_SIGNATURE_SIZE);
> + smb2_sg_set_buf(&sg[idx], sign, SMB2_SIGNATURE_SIZE);
> return sg;
> }
>
> @@ -2246,10 +2267,11 @@ smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
> * untouched.
> */
> static int
> -crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
> +crypt_message(struct TCP_Server_Info *server, int num_rqst,
> + struct smb_rqst *rqst, int enc)
> {
> struct smb2_transform_hdr *tr_hdr =
> - (struct smb2_transform_hdr *)rqst->rq_iov[0].iov_base;
> + (struct smb2_transform_hdr *)rqst[0].rq_iov[0].iov_base;
> unsigned int assoc_data_len = sizeof(struct smb2_transform_hdr) - 20;
> int rc = 0;
> struct scatterlist *sg;
> @@ -2300,7 +2322,7 @@ crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
> crypt_len += SMB2_SIGNATURE_SIZE;
> }
>
> - sg = init_sg(rqst, sign);
> + sg = init_sg(num_rqst, rqst, sign);
> if (!sg) {
> cifs_dbg(VFS, "%s: Failed to init sg", __func__);
> rc = -ENOMEM;
> @@ -2341,98 +2363,79 @@ static int
> smb3_init_transform_rq(struct TCP_Server_Info *server, struct smb_rqst *new_rq,
> struct smb_rqst *old_rq)
> {
> - struct kvec *iov;
> struct page **pages;
> - struct smb2_transform_hdr *tr_hdr;
> + struct smb2_transform_hdr *tr_hdr = new_rq[0].rq_iov[0].iov_base;
> unsigned int npages = old_rq->rq_npages;
> unsigned int orig_len = 0;
> - int i;
> + int j;
> int rc = -ENOMEM;
>
> pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> if (!pages)
> - return rc;
> + goto err_free;
>
> - new_rq->rq_pages = pages;
> - new_rq->rq_offset = old_rq->rq_offset;
> - new_rq->rq_npages = old_rq->rq_npages;
> - new_rq->rq_pagesz = old_rq->rq_pagesz;
> - new_rq->rq_tailsz = old_rq->rq_tailsz;
> + new_rq[1].rq_pages = pages;
> + new_rq[1].rq_npages = npages;
> + new_rq[1].rq_offset = old_rq->rq_offset;
> + new_rq[1].rq_pagesz = old_rq->rq_pagesz;
> + new_rq[1].rq_tailsz = old_rq->rq_tailsz;
> + new_rq[1].rq_iov = old_rq->rq_iov;
> + new_rq[1].rq_nvec = old_rq->rq_nvec;
>
> - for (i = 0; i < old_rq->rq_nvec; i++)
> - orig_len += old_rq->rq_iov[i].iov_len;
> + for (j = 0; j < old_rq->rq_nvec; j++)
> + orig_len += old_rq->rq_iov[j].iov_len;
BTW, the above code is buggy (e.g. it's not counting any data pages in
the request) and has been fixed by 35e2cc1ba755cf. Could you please
rebase your series with steve's for-next?
>
> - for (i = 0; i < npages; i++) {
> - pages[i] = alloc_page(GFP_KERNEL|__GFP_HIGHMEM);
> - if (!pages[i])
> - goto err_free_pages;
> + for (j = 0; j < npages; j++) {
> + pages[j] = alloc_page(GFP_KERNEL|__GFP_HIGHMEM);
> + if (!pages[j])
> + goto err_free;
> }
>
> - iov = kmalloc_array(old_rq->rq_nvec + 1, sizeof(struct kvec),
> - GFP_KERNEL);
> - if (!iov)
> - goto err_free_pages;
> -
> - /* copy all iovs from the old */
> - memcpy(&iov[1], &old_rq->rq_iov[0],
> - sizeof(struct kvec) * old_rq->rq_nvec);
> -
> - new_rq->rq_iov = iov;
> - new_rq->rq_nvec = old_rq->rq_nvec + 1;
> -
> - tr_hdr = kmalloc(sizeof(struct smb2_transform_hdr), GFP_KERNEL);
> - if (!tr_hdr)
> - goto err_free_iov;
> -
> - /* fill the 2nd iov with a transform header */
> - fill_transform_hdr(tr_hdr, orig_len, old_rq);
> - new_rq->rq_iov[0].iov_base = tr_hdr;
> - new_rq->rq_iov[0].iov_len = sizeof(struct smb2_transform_hdr);
> -
> /* copy pages form the old */
> - for (i = 0; i < npages; i++) {
> + for (j = 0; j < npages; j++) {
> char *dst, *src;
> unsigned int offset, len;
>
> - rqst_page_get_length(new_rq, i, &len, &offset);
> + rqst_page_get_length(&new_rq[1], j, &len, &offset);
>
> - dst = (char *) kmap(new_rq->rq_pages[i]) + offset;
> - src = (char *) kmap(old_rq->rq_pages[i]) + offset;
> + dst = (char *) kmap(new_rq[1].rq_pages[j]) + offset;
> + src = (char *) kmap(old_rq->rq_pages[j]) + offset;
>
> memcpy(dst, src, len);
> - kunmap(new_rq->rq_pages[i]);
> - kunmap(old_rq->rq_pages[i]);
> + kunmap(new_rq[1].rq_pages[j]);
> + kunmap(old_rq->rq_pages[j]);
> }
>
> - rc = crypt_message(server, new_rq, 1);
> +
> + /* fill the 1nd iov with a transform header */
s/1nd/1st/
> + fill_transform_hdr(tr_hdr, orig_len, old_rq);
> +
> + rc = crypt_message(server, 2, new_rq, 1);
> cifs_dbg(FYI, "encrypt message returned %d", rc);
> if (rc)
> - goto err_free_tr_hdr;
> + goto err_free;
>
> return rc;
>
> -err_free_tr_hdr:
> - kfree(tr_hdr);
> -err_free_iov:
> - kfree(iov);
> -err_free_pages:
> - for (i = i - 1; i >= 0; i--)
> - put_page(pages[i]);
> - kfree(pages);
> +err_free:
> + if (new_rq[1].rq_pages) {
> + for (j = new_rq[1].rq_npages - 1; j >= 0; j--)
> + put_page(new_rq[1].rq_pages[j]);
> + kfree(new_rq[1].rq_pages);
> + }
> return rc;
> }
>
> static void
> -smb3_free_transform_rq(struct smb_rqst *rqst)
> +smb3_free_transform_rq(int num_rqst, struct smb_rqst *rqst)
> {
> - int i = rqst->rq_npages - 1;
> + int i, j;
>
> - for (; i >= 0; i--)
> - put_page(rqst->rq_pages[i]);
> - kfree(rqst->rq_pages);
> - /* free transform header */
> - kfree(rqst->rq_iov[0].iov_base);
> - kfree(rqst->rq_iov);
> + for (i = 1; i < num_rqst; i++) {
> + for (j = rqst[i].rq_npages - 1; j >= 0; j--)
> + put_page(rqst[i].rq_pages[j]);
> + kfree(rqst[i].rq_pages);
> + }
> }
>
> static int
> @@ -2464,7 +2467,7 @@ decrypt_raw_data(struct TCP_Server_Info *server, char *buf,
> rqst.rq_pagesz = PAGE_SIZE;
> rqst.rq_tailsz = (page_data_size % PAGE_SIZE) ? : PAGE_SIZE;
>
> - rc = crypt_message(server, &rqst, 0);
> + rc = crypt_message(server, 1, &rqst, 0);
> cifs_dbg(FYI, "decrypt message returned %d\n", rc);
>
> if (rc)
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 13c244dfb3c1..7a8b601b144e 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -284,6 +284,9 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
> for (j = 0; j < num_rqst; j++) {
> iov = rqst[j].rq_iov;
> n_vec = rqst[j].rq_nvec;
> + // XXX temp
> + if (iov == NULL)
> + continue;
Temp check?
>
> cifs_dbg(FYI, "Sending smb: smb_len=%u\n", send_length);
> dump_smb(iov[0].iov_base, iov[0].iov_len);
> @@ -346,27 +349,40 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
> return rc;
> }
>
> +#define MAX_COMPOUND 2
> +
> static int
> smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst, int flags)
> {
> - struct smb_rqst cur_rqst;
> + struct kvec iov;
> + struct smb2_transform_hdr tr_hdr;
> + struct smb_rqst cur_rqst[MAX_COMPOUND];
> int rc;
>
> if (!(flags & CIFS_TRANSFORM_REQ))
> return __smb_send_rqst(server, 1, rqst);
>
> + memset(&cur_rqst[0], 0, sizeof(cur_rqst));
> + memset(&iov, 0, sizeof(iov));
> + memset(&tr_hdr, 0, sizeof(tr_hdr));
> +
> + iov.iov_base = &tr_hdr;
> + iov.iov_len = sizeof(tr_hdr);
> + cur_rqst[0].rq_iov = &iov;
> + cur_rqst[0].rq_nvec = 1;
> +
> if (!server->ops->init_transform_rq ||
> !server->ops->free_transform_rq) {
> cifs_dbg(VFS, "Encryption requested but transform callbacks are missed\n");
> return -EIO;
> }
>
> - rc = server->ops->init_transform_rq(server, &cur_rqst, rqst);
> + rc = server->ops->init_transform_rq(server, &cur_rqst[0], rqst);
> if (rc)
> return rc;
>
> - rc = __smb_send_rqst(server, 1, &cur_rqst);
> - server->ops->free_transform_rq(&cur_rqst);
> + rc = __smb_send_rqst(server, 2, &cur_rqst[0]);
> + server->ops->free_transform_rq(2, &cur_rqst[0]);
What about using MAX_COMPOUND instead of 2?
Please pardon the brevity and lack of information. I'm not very familiar
with the code yet.
Thanks
Paulo
> return rc;
> }
>
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
