[PATCH] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[PATCH] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[Mandeep Singh Baines]

Plus some other minor cleanup.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
---
 crypto/sha1_generic.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c
index 0416091..4bdd228 100644
--- a/crypto/sha1_generic.c
+++ b/crypto/sha1_generic.c
@@ -40,33 +40,32 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 			unsigned int len)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
-	unsigned int partial, done;
-	const u8 *src;
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
+	u32 temp[SHA_WORKSPACE_WORDS];
+	const u8 *src = data;
 
-	partial = sctx->count & 0x3f;
 	sctx->count += len;
-	done = 0;
-	src = data;
 
-	if ((partial + len) > 63) {
-		u32 temp[SHA_WORKSPACE_WORDS];
+	if (partial && ((partial + len) >= SHA1_BLOCK_SIZE)) {
+		unsigned int done = SHA1_BLOCK_SIZE - partial;
 
-		if (partial) {
-			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
-			src = sctx->buffer;
-		}
+		memcpy(sctx->buffer + partial, src, done);
+		sha_transform(sctx->state, sctx->buffer, temp);
+		len -= done;
+		src += done;
+		partial = 0;
+	}
 
-		do {
-			sha_transform(sctx->state, src, temp);
-			done += 64;
-			src = data + done;
-		} while (done + 63 < len);
+	while (len >= SHA1_BLOCK_SIZE) {
+		sha_transform(sctx->state, src, temp);
+		len -= SHA1_BLOCK_SIZE;
+		src += SHA1_BLOCK_SIZE;
+	}
 
+	if (src != data)
 		memset(temp, 0, sizeof(temp));
-		partial = 0;
-	}
-	memcpy(sctx->buffer + partial, src, len - done);
+
+	memcpy(sctx->buffer + partial, src, len);
 
 	return 0;
 }
-- 
1.7.3.1

Re: [PATCH] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[David Miller]

From: Mandeep Singh Baines <msb@chromium.org>
Date: Wed, 25 May 2011 20:11:17 -0700

> Plus some other minor cleanup.
> 
> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>

The temp[] buffer is explicitly places inside the inner most
basic block so that the compiler doesn't allocate the stack
space unless that code path is taken.

Besides the use of the SHA_* macros, I think your changes
actually make the code worse.

Re: [PATCH] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[Joe Perches]

On Wed, 2011-05-25 at 23:34 -0400, David Miller wrote:
> From: Mandeep Singh Baines <msb@chromium.org>
> Date: Wed, 25 May 2011 20:11:17 -0700
> > Plus some other minor cleanup.
> > Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
> The temp[] buffer is explicitly places inside the inner most
> basic block so that the compiler doesn't allocate the stack
> space unless that code path is taken.

Does any version of gcc manage to do that?

Regardless, it's still a good idea to keep
declarations in the use scope.

[PATCH v2] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[Mandeep Singh Baines]

David Miller (davem@davemloft.net) wrote:
> 
> The temp[] buffer is explicitly places inside the inner most
> basic block so that the compiler doesn't allocate the stack
> space unless that code path is taken.
> 

Fixed in V2 (this patch). Thanks for the review.

-- >8 -- (snip)

Plus some other minor cleanup.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joe Perches <joe@perches.com>
Cc: linux-crypto@vger.kernel.org
---
 crypto/sha1_generic.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c
index 0416091..0b56719 100644
--- a/crypto/sha1_generic.c
+++ b/crypto/sha1_generic.c
@@ -40,33 +40,30 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 			unsigned int len)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
-	unsigned int partial, done;
-	const u8 *src;
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
 
-	partial = sctx->count & 0x3f;
 	sctx->count += len;
-	done = 0;
-	src = data;
 
-	if ((partial + len) > 63) {
+	if ((partial + len) >= SHA1_BLOCK_SIZE) {
 		u32 temp[SHA_WORKSPACE_WORDS];
 
 		if (partial) {
-			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
-			src = sctx->buffer;
-		}
-
-		do {
-			sha_transform(sctx->state, src, temp);
-			done += 64;
-			src = data + done;
-		} while (done + 63 < len);
+			unsigned int done = SHA1_BLOCK_SIZE - partial;
 
+			memcpy(sctx->buffer + partial, data, done);
+			sha_transform(sctx->state, sctx->buffer, temp);
+			len -= done;
+			data += done;
+			partial = 0;
+		}
+		while (len >= SHA1_BLOCK_SIZE) {
+			sha_transform(sctx->state, data, temp);
+			len -= SHA1_BLOCK_SIZE;
+			data += SHA1_BLOCK_SIZE;
+		}
 		memset(temp, 0, sizeof(temp));
-		partial = 0;
 	}
-	memcpy(sctx->buffer + partial, src, len - done);
+	memcpy(sctx->buffer + partial, data, len);
 
 	return 0;
 }
-- 
1.7.3.1

Re: [PATCH v2] crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

[Herbert Xu]

On Thu, May 26, 2011 at 04:20:58PM -0700, Mandeep Singh Baines wrote:
> David Miller (davem@davemloft.net) wrote:
> > 
> > The temp[] buffer is explicitly places inside the inner most
> > basic block so that the compiler doesn't allocate the stack
> > space unless that code path is taken.
> > 
> 
> Fixed in V2 (this patch). Thanks for the review.
> 
> -- >8 -- (snip)
> 
> Plus some other minor cleanup.

I don't really like the cleanups.  In any case, mixing up the
use of SHA1_BLOCK_SIZE with cleanups increases the chance of
introducing a bug.

So please redo the patch with only the addition of SHA1_BLOCK_SIZE
and nothing else.

Thanks,
-- 
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

[PATCH v3] crypto: sha1: use SHA1_BLOCK_SIZE

[Mandeep Singh Baines]

Modify sha1_update to use SHA1_BLOCK_SIZE.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
---
 crypto/sha1_generic.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c
index 0416091..00ae60e 100644
--- a/crypto/sha1_generic.c
+++ b/crypto/sha1_generic.c
@@ -43,25 +43,26 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 	unsigned int partial, done;
 	const u8 *src;
 
-	partial = sctx->count & 0x3f;
+	partial = sctx->count % SHA1_BLOCK_SIZE;
 	sctx->count += len;
 	done = 0;
 	src = data;
 
-	if ((partial + len) > 63) {
+	if ((partial + len) >= SHA1_BLOCK_SIZE) {
 		u32 temp[SHA_WORKSPACE_WORDS];
 
 		if (partial) {
 			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
+			memcpy(sctx->buffer + partial, data,
+			       done + SHA1_BLOCK_SIZE);
 			src = sctx->buffer;
 		}
 
 		do {
 			sha_transform(sctx->state, src, temp);
-			done += 64;
+			done += SHA1_BLOCK_SIZE;
 			src = data + done;
-		} while (done + 63 < len);
+		} while (done + SHA1_BLOCK_SIZE <= len);
 
 		memset(temp, 0, sizeof(temp));
 		partial = 0;
-- 
1.7.3.1

Re: [PATCH v3] crypto: sha1: use SHA1_BLOCK_SIZE

[Herbert Xu]

On Thu, Jun 23, 2011 at 12:02:27PM -0700, Mandeep Singh Baines wrote:
> Modify sha1_update to use SHA1_BLOCK_SIZE.
> 
> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>

Patch applied.  Thanks a lot!
-- 
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