Thanks for the review.
----- Original Message -----
> From: "Paulo Alcantara" <paulo@xxxxxxxx>
> To: "Ronnie Sahlberg" <lsahlber@xxxxxxxxxx>, "linux-cifs" <linux-cifs@xxxxxxxxxxxxxxx>
> Cc: "Steve French" <smfrench@xxxxxxxxx>
> Sent: Thursday, 21 June, 2018 4:44:30 AM
> Subject: Re: [PATCH 1/9] cifs: update init_sg and crypt_message to take an array of rqst
>
> 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?
Yepp, artefact from debugging.
Thanks.
>
> > + 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?
Yepp, that code can be made simpler. Will fix.
>
> > + 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?
Will do.
>
> >
> > - 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/
Fixed.
>
> > + 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?
removed.
> >
> > 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?
This is addressed in a later patch where we use the number of requests passed into this function instead of '1' and '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
