[PATCH]: Fix ALIGN() macro

[PATCH]: Fix ALIGN() macro

[David Miller]

A regression or two were added by the crypto-2.6 tree merge.

I've tracked down one of them, and it's caused by the ALIGN()
macro truncating things down to "int".  Some of Herbert's new
code is aligning pointers using ALIGN() and this thus explodes
on 64-bit since the top 32-bits get chopped off.

It is arguable that perhaps we should make a special macro
for pointer aligning, but even in that case ALIGN() as a general
purpose macro should use the largest natural integer size in
order to not cause surprises for people.

I'm still trying to track down the other regression added by
the crypto merge.  I have it git bisected down to a single
changeset, but I haven't determined what's really wrong yet.
I should be able to kill that over the weekend.  I want to fix
this before merging my networking tree so I can be absolutely
sure that IPSEC doesn't break because of something in my tree :)

[KERNEL]: Do not truncate to 'int' in ALIGN() macro.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 851aa1b..2b2ae4f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -31,7 +31,7 @@ #define ULLONG_MAX	(~0ULL)
 #define STACK_MAGIC	0xdeadbeef
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#define ALIGN(x,a) (((x)+(a)-1)&~((a)-1))
+#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
 #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
 #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
 

Re: [PATCH]: Fix ALIGN() macro

[Herbert Xu]

On Fri, Sep 22, 2006 at 10:31:36PM -0700, David Miller wrote:
>
> I'm still trying to track down the other regression added by
> the crypto merge.  I have it git bisected down to a single
> changeset, but I haven't determined what's really wrong yet.
> I should be able to kill that over the weekend.  I want to fix
> this before merging my networking tree so I can be absolutely
> sure that IPSEC doesn't break because of something in my tree :)

Thanks for fixing this Dave.  I recall being bitten earlier
by the same thing as well.  I really need to start testing
on 64-bit.

BTW could you describe the other regression?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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]: Fix ALIGN() macro

[Herbert Xu]

On Sat, Sep 23, 2006 at 10:46:33PM +1000, herbert wrote:
>
> Thanks for fixing this Dave.  I recall being bitten earlier
> by the same thing as well.  I really need to start testing
> on 64-bit.
> 
> BTW could you describe the other regression?

Nevermind, I saw your note on IRC.  If you haven't found the
problem yet, could you pleas try modprobe tcrypt mode=100?

That should better pin-point the problem.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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]: Fix ALIGN() macro

[Herbert Xu]

Hi:

OK I think I've found the problem.

[CRYPTO] hmac: Fix hmac_init update call

The crypto_hash_update call in hmac_init gave the number 1
instead of the length of the sg list in bytes.  This is a
missed conversion from the digest => hash change.

As tcrypt only tests crypto_hash_digest it didn't catch this.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/crypto/hmac.c b/crypto/hmac.c
index f403b69..d52b234 100644
--- a/crypto/hmac.c
+++ b/crypto/hmac.c
@@ -98,7 +98,7 @@ static int hmac_init(struct hash_desc *p
 	sg_set_buf(&tmp, ipad, bs);
 
 	return unlikely(crypto_hash_init(&desc)) ?:
-	       crypto_hash_update(&desc, &tmp, 1);
+	       crypto_hash_update(&desc, &tmp, bs);
 }
 
 static int hmac_update(struct hash_desc *pdesc,

Re: [PATCH]: Fix ALIGN() macro

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 24 Sep 2006 00:40:41 +1000

> OK I think I've found the problem.
> 
> [CRYPTO] hmac: Fix hmac_init update call
> 
> The crypto_hash_update call in hmac_init gave the number 1
> instead of the length of the sg list in bytes.  This is a
> missed conversion from the digest => hash change.
> 
> As tcrypt only tests crypto_hash_digest it didn't catch this.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This, along with the ALIGN() patch, fixes all the regressions
for me.  Good spotting Herbert!

Linus, please apply.

Re: [PATCH]: Fix ALIGN() macro

[Kyle Moffett]

On Sep 23, 2006, at 10:40:41, Herbert Xu wrote:
> diff --git a/crypto/hmac.c b/crypto/hmac.c
> index f403b69..d52b234 100644
> --- a/crypto/hmac.c
> +++ b/crypto/hmac.c
> @@ -98,7 +98,7 @@ static int hmac_init(struct hash_desc *p
>  	sg_set_buf(&tmp, ipad, bs);
>
>  	return unlikely(crypto_hash_init(&desc)) ?:
> -	       crypto_hash_update(&desc, &tmp, 1);
> +	       crypto_hash_update(&desc, &tmp, bs);
>  }
>
>  static int hmac_update(struct hash_desc *pdesc,

Quick question:  does "crypto_hash_init()" ever return anything other  
than 0 or 1?  If so this is a subtle bug, as "unlikely()" is  
implemented like this:

# define unlikely(x) __builtin_expect(!!(x), 0)

IMO any usage of likely/unlikely other than if(unlikely()), if(likely 
()) is probably a bug.

Cheers,
Kyle Moffett

Re: [PATCH]: Fix ALIGN() macro

[Kyle Moffett]

On Sep 23, 2006, at 16:36:33, Kyle Moffett wrote:
> On Sep 23, 2006, at 10:40:41, Herbert Xu wrote:
>> diff --git a/crypto/hmac.c b/crypto/hmac.c
>> index f403b69..d52b234 100644
>> --- a/crypto/hmac.c
>> +++ b/crypto/hmac.c
>> @@ -98,7 +98,7 @@ static int hmac_init(struct hash_desc *p
>>  	sg_set_buf(&tmp, ipad, bs);
>>
>>  	return unlikely(crypto_hash_init(&desc)) ?:
>> -	       crypto_hash_update(&desc, &tmp, 1);
>> +	       crypto_hash_update(&desc, &tmp, bs);
>>  }
>>
>>  static int hmac_update(struct hash_desc *pdesc,
>
> Quick question:  does "crypto_hash_init()" ever return anything  
> other than 0 or 1?  If so this is a subtle bug, as "unlikely()" is  
> implemented like this:
>
> # define unlikely(x) __builtin_expect(!!(x), 0)
>
> IMO any usage of likely/unlikely other than if(unlikely()), if 
> (likely()) is probably a bug.

With a bit more contemplation, I think this is one place where (if  
the compiler or sparse are cooperative) we should really look at the  
_Bool type or similar.  If we could cast likely()/unlikely() to  
return (_Bool), then these sorts of problems would be caught at  
compile time, and likewise for other functions which return boolean  
values.

Cheers,
Kyle Moffett

Re: [PATCH]: Fix ALIGN() macro

[Herbert Xu]

On Sat, Sep 23, 2006 at 04:36:33PM -0400, Kyle Moffett wrote:
>
> Quick question:  does "crypto_hash_init()" ever return anything other  
> than 0 or 1?  If so this is a subtle bug, as "unlikely()" is  
> implemented like this:

Good point.  It's meant to be an errno value so this is a bug.

[CRYPTO] hmac: Fix error truncation by unlikely()

The error return values are truncated by unlikely so we need to
save it first.  Thanks to Kyle Moffett for spotting this.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/crypto/hmac.c b/crypto/hmac.c
index d52b234..b521bcd 100644
--- a/crypto/hmac.c
+++ b/crypto/hmac.c
@@ -92,13 +92,17 @@ static int hmac_init(struct hash_desc *p
 	struct hmac_ctx *ctx = align_ptr(ipad + bs * 2 + ds, sizeof(void *));
 	struct hash_desc desc;
 	struct scatterlist tmp;
+	int err;
 
 	desc.tfm = ctx->child;
 	desc.flags = pdesc->flags & CRYPTO_TFM_REQ_MAY_SLEEP;
 	sg_set_buf(&tmp, ipad, bs);
 
-	return unlikely(crypto_hash_init(&desc)) ?:
-	       crypto_hash_update(&desc, &tmp, bs);
+	err = crypto_hash_init(&desc);
+	if (unlikely(err))
+		return err;
+
+	return crypto_hash_update(&desc, &tmp, bs);
 }
 
 static int hmac_update(struct hash_desc *pdesc,
@@ -123,13 +127,17 @@ static int hmac_final(struct hash_desc *
 	struct hmac_ctx *ctx = align_ptr(digest + ds, sizeof(void *));
 	struct hash_desc desc;
 	struct scatterlist tmp;
+	int err;
 
 	desc.tfm = ctx->child;
 	desc.flags = pdesc->flags & CRYPTO_TFM_REQ_MAY_SLEEP;
 	sg_set_buf(&tmp, opad, bs + ds);
 
-	return unlikely(crypto_hash_final(&desc, digest)) ?:
-	       crypto_hash_digest(&desc, &tmp, bs + ds, out);
+	err = crypto_hash_final(&desc, digest);
+	if (unlikely(err))
+		return err;
+
+	return crypto_hash_digest(&desc, &tmp, bs + ds, out);
 }
 
 static int hmac_digest(struct hash_desc *pdesc, struct scatterlist *sg,
@@ -145,6 +153,7 @@ static int hmac_digest(struct hash_desc 
 	struct hash_desc desc;
 	struct scatterlist sg1[2];
 	struct scatterlist sg2[1];
+	int err;
 
 	desc.tfm = ctx->child;
 	desc.flags = pdesc->flags & CRYPTO_TFM_REQ_MAY_SLEEP;
@@ -154,8 +163,11 @@ static int hmac_digest(struct hash_desc 
 	sg1[1].length = 0;
 	sg_set_buf(sg2, opad, bs + ds);
 
-	return unlikely(crypto_hash_digest(&desc, sg1, nbytes + bs, digest)) ?:
-	       crypto_hash_digest(&desc, sg2, bs + ds, out);
+	err = crypto_hash_digest(&desc, sg1, nbytes + bs, digest);
+	if (unlikely(err))
+		return err;
+
+	return crypto_hash_digest(&desc, sg2, bs + ds, out);
 }
 
 static int hmac_init_tfm(struct crypto_tfm *tfm)

Re: [PATCH]: Fix ALIGN() macro

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 23 Sep 2006 22:54:59 +1000

> On Sat, Sep 23, 2006 at 10:46:33PM +1000, herbert wrote:
> >
> > Thanks for fixing this Dave.  I recall being bitten earlier
> > by the same thing as well.  I really need to start testing
> > on 64-bit.
> > 
> > BTW could you describe the other regression?
> 
> Nevermind, I saw your note on IRC.  If you haven't found the
> problem yet, could you pleas try modprobe tcrypt mode=100?
> 
> That should better pin-point the problem.

I'll do that, but currently I believe that the error has something to
do with the "tail" skb handling in the ESP conversions within that
changeset.

I should have a good handle on this bug by the end of today.

Thanks.