| Message ID | 20180620011350.12328-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ronnie, Thanks for the great work on compounding! My comments bellow. Ronnie Sahlberg <lsahlber@redhat.com> 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@redhat.com> > --- > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review. ----- Original Message ----- > From: "Paulo Alcantara" <paulo@paulo.ac> > To: "Ronnie Sahlberg" <lsahlber@redhat.com>, "linux-cifs" <linux-cifs@vger.kernel.org> > Cc: "Steve French" <smfrench@gmail.com> > 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@redhat.com> 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@redhat.com> > > --- > > 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@vger.kernel.org > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + 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); + 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; - 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 */ + 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; 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]); return rc; }
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@redhat.com> --- fs/cifs/cifsglob.h | 2 +- fs/cifs/smb2ops.c | 155 ++++++++++++++++++++++++++-------------------------- fs/cifs/transport.c | 24 ++++++-- 3 files changed, 100 insertions(+), 81 deletions(-)