[PATCH] crypto: algif - Mark sgl end at the end of data.

[PATCH] crypto: algif - Mark sgl end at the end of data.

[Tadeusz Struk]

Hi,
algif_skcipher sends 127 sgl buffers for encryption regardless of how many
buffers acctually have data to process, where the few first with valid len
and the rest with zero len. This is not very eficient and may cause problems
when algs do something like this without checking the buff lenght:
for_each_sg(sgl, sg, sg_nents, i)
	sg_virt(sg)

This patch marks the last one with data as the last one to process.
Also removed some unneeded code.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/algif_skcipher.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 85e3bdb..e393c71 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -359,8 +359,6 @@ static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
 	err = 0;
 
 	ctx->more = msg->msg_flags & MSG_MORE;
-	if (!ctx->more && !list_empty(&ctx->tsgl))
-		sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
 
 unlock:
 	skcipher_data_wakeup(sk);
@@ -408,9 +406,6 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
 
 done:
 	ctx->more = flags & MSG_MORE;
-	if (!ctx->more && !list_empty(&ctx->tsgl))
-		sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
-
 unlock:
 	skcipher_data_wakeup(sk);
 	release_sock(sk);
@@ -469,6 +464,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
 			if (!used)
 				goto free;
 
+			sg_mark_end(&sg[sgl->cur - 1]);
 			ablkcipher_request_set_crypt(&ctx->req, sg,
 						     ctx->rsgl.sg, used,
 						     ctx->iv);

Re: [PATCH] crypto: algif - Mark sgl end at the end of data.

[Herbert Xu]

On Fri, Nov 21, 2014 at 09:14:06AM -0800, Tadeusz Struk wrote:
>
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 85e3bdb..e393c71 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -359,8 +359,6 @@ static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
>  	err = 0;
>  
>  	ctx->more = msg->msg_flags & MSG_MORE;
> -	if (!ctx->more && !list_empty(&ctx->tsgl))
> -		sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
>  
>  unlock:
>  	skcipher_data_wakeup(sk);
> @@ -408,9 +406,6 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
>  
>  done:
>  	ctx->more = flags & MSG_MORE;
> -	if (!ctx->more && !list_empty(&ctx->tsgl))
> -		sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
> -
>  unlock:
>  	skcipher_data_wakeup(sk);
>  	release_sock(sk);

Please put these clean-ups in a separate patch.

> @@ -469,6 +464,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
>  			if (!used)
>  				goto free;
>  
> +			sg_mark_end(&sg[sgl->cur - 1]);

I don't think this will work as if we only partially use up the
SGs and MSG_MORE is set then bad things will happen to the next
send call on the socket.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: algif - Mark sgl end at the end of data.

[Tadeusz Struk]

Hi Herbert,
On 11/25/2014 06:42 AM, Herbert Xu wrote:
> Please put these clean-ups in a separate patch.
Ok, will do.

> 
>> > @@ -469,6 +464,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
>> >  			if (!used)
>> >  				goto free;
>> >  
>> > +			sg_mark_end(&sg[sgl->cur - 1]);
> I don't think this will work as if we only partially use up the
> SGs and MSG_MORE is set then bad things will happen to the next
> send call on the socket.

Yes, I see now. I assumed that the user would want to read the same len
that was first sent and thus the skcipher_pull_sgl() would clean the
whole ctx->tsgl and then skcipher_alloc_sgl() would create a new one.
I'll see if something else can be done to mark the end of data.
Thanks,
Tadeusz

[PATCH] crypto: algif - Mark sgl end at the end of data

[Tadeusz Struk]

Hi,
algif_skcipher sends 127 sgl buffers for encryption regardless of how
many buffers acctually have data to process, where the few first with
valid len and the rest with zero len. This is not very eficient and may cause
problems when algs do something like this without checking the buff
lenght:
for_each_sg(sgl, sg, sg_nents, i)
	sg_virt(sg)
This patch marks the last one with data as the last one to process.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/algif_skcipher.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f80e652..46a0758 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -441,6 +441,8 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
 		char __user *from = iov->iov_base;
 
 		while (seglen) {
+			int nents;
+
 			sgl = list_first_entry(&ctx->tsgl,
 					       struct skcipher_sg_list, list);
 			sg = sgl->sg;
@@ -468,6 +470,9 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
 			if (!used)
 				goto free;
 
+			nents = sg_nents(ctx->rsgl.sg);
+			sg_mark_end(&sg[nents - 1]);
+
 			ablkcipher_request_set_crypt(&ctx->req, sg,
 						     ctx->rsgl.sg, used,
 						     ctx->iv);
@@ -477,6 +482,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
 					crypto_ablkcipher_encrypt(&ctx->req) :
 					crypto_ablkcipher_decrypt(&ctx->req),
 				&ctx->completion);
+			sg_unmark_end(&sg[nents - 1]);
 
 free:
 			af_alg_free_sg(&ctx->rsgl);

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Herbert Xu]

On Fri, Nov 28, 2014 at 10:40:36AM -0800, Tadeusz Struk wrote:
>
> @@ -468,6 +470,9 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
>  			if (!used)
>  				goto free;
>  
> +			nents = sg_nents(ctx->rsgl.sg);
> +			sg_mark_end(&sg[nents - 1]);

Huh? You're getting nents from the RX side and using it to mark
the TX side? This makes no sense because RX may have no relationship
whatsoever with TX.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Tadeusz Struk]

Hi Herbert,
On 12/01/2014 06:40 AM, Herbert Xu wrote:
>> +			nents = sg_nents(ctx->rsgl.sg);
>> > +			sg_mark_end(&sg[nents - 1]);
> Huh? You're getting nents from the RX side and using it to mark
> the TX side? This makes no sense because RX may have no relationship
> whatsoever with TX.

Yes, but there shouldn't be more nents with data to be processed in TX
than nents in RX and in most cases they should be equal. Or am I missing
something?
Thanks,
Tadeusz

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Herbert Xu]

On Mon, Dec 01, 2014 at 06:53:41AM -0800, Tadeusz Struk wrote:
> Hi Herbert,
> On 12/01/2014 06:40 AM, Herbert Xu wrote:
> >> +			nents = sg_nents(ctx->rsgl.sg);
> >> > +			sg_mark_end(&sg[nents - 1]);
> > Huh? You're getting nents from the RX side and using it to mark
> > the TX side? This makes no sense because RX may have no relationship
> > whatsoever with TX.
> 
> Yes, but there shouldn't be more nents with data to be processed in TX
> than nents in RX and in most cases they should be equal. Or am I missing
> something?

As I said the two are arbitrary and we don't place any restrictions
on them at all (apart from the fact that the TX length in bytes must
obviously be longer than the RX bytes).

So you can have a 1-element TX list with a multi-element RX list.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Tadeusz Struk]

On 12/01/2014 07:00 AM, Herbert Xu wrote:
> As I said the two are arbitrary and we don't place any restrictions
> on them at all (apart from the fact that the TX length in bytes must
> obviously be longer than the RX bytes).
> 
> So you can have a 1-element TX list with a multi-element RX list.

Ok, I can look at the data, but do you think the idea with marking the
end of data in TX sgl is worthwhile or should I just forget about it.
Another question is - is an sgl with lots of empty buffers a valid input
from an algorithm implementation point of view?
Thanks,
Tadeusz

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Herbert Xu]

On Mon, Dec 01, 2014 at 07:03:00AM -0800, Tadeusz Struk wrote:
> 
> Ok, I can look at the data, but do you think the idea with marking the
> end of data in TX sgl is worthwhile or should I just forget about it.
> Another question is - is an sgl with lots of empty buffers a valid input
> from an algorithm implementation point of view?

I think marking the end is useful.  How about doing the marking
and unmarking whenever sgl->cur is updated?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: algif - Mark sgl end at the end of data

[Tadeusz Struk]

Hi,
On 12/02/2014 06:33 AM, Herbert Xu wrote:
> I think marking the end is useful.  How about doing the marking
> and unmarking whenever sgl->cur is updated?

I have a v2 ready where I mark it based on the actual data.
Thanks