[1/9] cifs: update init_sg and crypt_message to take an array of rqst

Message ID 20180620011350.12328-2-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: compounding
Related show

Commit Message

Ronnie Sahlberg June 20, 2018, 1:13 a.m.
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(-)

Comments

Paulo Alcantara June 20, 2018, 6:44 p.m. | #1
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
Ronnie Sahlberg June 20, 2018, 10:28 p.m. | #2
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

Patch

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;
 }