Bug when modprobing tcrypt

Bug when modprobing tcrypt

[Eric Sesterhenn]

Hi,

i can repeatedly trigger the following bug when modprobing tcrypt
to test the crypto algorithms with todays -git

[  122.967099] BUG: sleeping function called from invalid context at
kernel/rwsem.c:21
[  122.967398] in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name:
modprobe
[  122.967643] INFO: lockdep is turned off.
[  122.967858] Pid: 4926, comm: modprobe Tainted: G   M
2.6.31-rc1-22297-g5298976 #24
[  122.968176] Call Trace:
[  122.968411]  [<c011dd93>] __might_sleep+0xf9/0x101
[  122.968677]  [<c0777aa0>] down_read+0x16/0x68
[  122.968928]  [<c048bf04>] crypto_alg_lookup+0x16/0x34
[  122.969479]  [<c048bf52>] crypto_larval_lookup+0x30/0xf9
[  122.969722]  [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
[  122.969977]  [<c048c13e>] crypto_alloc_base+0x1e/0x64
[  122.970271]  [<c04bf991>] reset_prng_context+0xab/0x13f
[  122.970523]  [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
[  122.970777]  [<c04bfce1>] cprng_init+0x2a/0x42
[  122.971012]  [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
[  122.971304]  [<c048c153>] crypto_alloc_base+0x33/0x64
[  122.971556]  [<c04933c9>] alg_test_cprng+0x30/0x1f4
[  122.971809]  [<c0493329>] alg_test+0x12f/0x19f
[  122.972103]  [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
[  122.972356]  [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
[  122.972613]  [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
[  122.972855]  [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
[  122.973488]  [<c010113c>] _stext+0x54/0x12c
[  122.974575]  [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
[  122.974836]  [<c01398a3>] ? up_read+0x16/0x2b
[  122.975126]  [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
[  122.975376]  [<c014ee8d>] sys_init_module+0xa9/0x1bf
[  122.975635]  [<c010292b>] sysenter_do_call+0x12/0x32

(gdb) l *(crypto_alg_lookup+0x16)
0xc048bf04 is in crypto_alg_lookup (crypto/api.c:201).
196	struct crypto_alg *crypto_alg_lookup(const char *name, u32 type, u32
mask)
197	{
198		struct crypto_alg *alg;
199	
200		down_read(&crypto_alg_sem);
201		alg = __crypto_alg_lookup(name, type, mask);
202		up_read(&crypto_alg_sem);
203	
204		return alg;
205	}

Please let me know if you need further information or have patches to
test.

Regards, Eric

Re: Bug when modprobing tcrypt

[Sebastian Andrzej Siewior]

* Eric Sesterhenn | 2009-06-29 15:17:05 [+0200]:

>[  122.967099] BUG: sleeping function called from invalid context at
>kernel/rwsem.c:21
>[  122.967398] in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name:
>modprobe
>[  122.967643] INFO: lockdep is turned off.
>[  122.967858] Pid: 4926, comm: modprobe Tainted: G   M
>2.6.31-rc1-22297-g5298976 #24
>[  122.968176] Call Trace:
>[  122.968411]  [<c011dd93>] __might_sleep+0xf9/0x101
>[  122.968677]  [<c0777aa0>] down_read+0x16/0x68
>[  122.968928]  [<c048bf04>] crypto_alg_lookup+0x16/0x34
>[  122.969479]  [<c048bf52>] crypto_larval_lookup+0x30/0xf9
>[  122.969722]  [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
>[  122.969977]  [<c048c13e>] crypto_alloc_base+0x1e/0x64
>[  122.970271]  [<c04bf991>] reset_prng_context+0xab/0x13f
>[  122.970523]  [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
>[  122.970777]  [<c04bfce1>] cprng_init+0x2a/0x42
>[  122.971012]  [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
>[  122.971304]  [<c048c153>] crypto_alloc_base+0x33/0x64
>[  122.971556]  [<c04933c9>] alg_test_cprng+0x30/0x1f4
>[  122.971809]  [<c0493329>] alg_test+0x12f/0x19f
>[  122.972103]  [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
>[  122.972356]  [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
>[  122.972613]  [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
>[  122.972855]  [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
>[  122.973488]  [<c010113c>] _stext+0x54/0x12c
>[  122.974575]  [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
>[  122.974836]  [<c01398a3>] ? up_read+0x16/0x2b
>[  122.975126]  [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
>[  122.975376]  [<c014ee8d>] sys_init_module+0xa9/0x1bf
>[  122.975635]  [<c010292b>] sysenter_do_call+0x12/0x32

reset_prng_context() grabs a spinlock. That prng_lock spinlock is taken
with irqsave and sometimes without what does not look good on the first
look.

Neil: It is legal for crypto_alloc_cipher() because it might load
another module. Are you fine with the replacement of the spinlock with a
mutex? rngapi_reset() in crypto/rng.c does kmalloc() with GFP_KERNEL so
it looks like it is okay to sleep there.

Sebastian

Re: Bug when modprobing tcrypt

[Neil Horman]

On Mon, Jun 29, 2009 at 04:07:04PM +0200, Sebastian Andrzej Siewior wrote:
> * Eric Sesterhenn | 2009-06-29 15:17:05 [+0200]:
> 
> >[  122.967099] BUG: sleeping function called from invalid context at
> >kernel/rwsem.c:21
> >[  122.967398] in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name:
> >modprobe
> >[  122.967643] INFO: lockdep is turned off.
> >[  122.967858] Pid: 4926, comm: modprobe Tainted: G   M
> >2.6.31-rc1-22297-g5298976 #24
> >[  122.968176] Call Trace:
> >[  122.968411]  [<c011dd93>] __might_sleep+0xf9/0x101
> >[  122.968677]  [<c0777aa0>] down_read+0x16/0x68
> >[  122.968928]  [<c048bf04>] crypto_alg_lookup+0x16/0x34
> >[  122.969479]  [<c048bf52>] crypto_larval_lookup+0x30/0xf9
> >[  122.969722]  [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
> >[  122.969977]  [<c048c13e>] crypto_alloc_base+0x1e/0x64
> >[  122.970271]  [<c04bf991>] reset_prng_context+0xab/0x13f
> >[  122.970523]  [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
> >[  122.970777]  [<c04bfce1>] cprng_init+0x2a/0x42
> >[  122.971012]  [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
> >[  122.971304]  [<c048c153>] crypto_alloc_base+0x33/0x64
> >[  122.971556]  [<c04933c9>] alg_test_cprng+0x30/0x1f4
> >[  122.971809]  [<c0493329>] alg_test+0x12f/0x19f
> >[  122.972103]  [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
> >[  122.972356]  [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
> >[  122.972613]  [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
> >[  122.972855]  [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
> >[  122.973488]  [<c010113c>] _stext+0x54/0x12c
> >[  122.974575]  [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
> >[  122.974836]  [<c01398a3>] ? up_read+0x16/0x2b
> >[  122.975126]  [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
> >[  122.975376]  [<c014ee8d>] sys_init_module+0xa9/0x1bf
> >[  122.975635]  [<c010292b>] sysenter_do_call+0x12/0x32
> 
> reset_prng_context() grabs a spinlock. That prng_lock spinlock is taken
> with irqsave and sometimes without what does not look good on the first
> look.
> 
> Neil: It is legal for crypto_alloc_cipher() because it might load
> another module. Are you fine with the replacement of the spinlock with a
> mutex? rngapi_reset() in crypto/rng.c does kmalloc() with GFP_KERNEL so
> it looks like it is okay to sleep there.
> 
I'm a bit concerned about making prng_lock a mutex, since that seems like it
would prevent the use of the cprng in interrupt context.  I agree that the mixed
use of spin_lock and spin_lock_irqsave with prng_lock is prone to deadlock, but
I think that means we should probably convert all uses of prng_lock to be
irqsave/restore variants to prevent the single cpu deadlock case.  As for the
BUG above, I think we can work around that by reorganizing reset_prng_context a
bit, as we really only need the lock around ctx modifications, and the
PRNG_NEED_RESET flag guards against concurrent use while the lock isn't held.

I'd be happy to write a patch if you like, or you can propose one.  Just let me
know.

Alternatively, if you can think of a way that mutexes here would still allow for
interrupt context use, I'd certainly be ok with that, I just don't see how to
make that happen at the moment.

Regards
Neil

> Sebastian
> 

[PATCH 1/2] crypto/ansi prng: use just a BH lock

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

the current code uses a mix of sping_lock() & spin_lock_irqsave(). This can
lead to deadlock with the correct timming & cprng_get_random() + cprng_reset()
sequence.
I've converted them to bottom half locks since all three user grab just a BH
lock so this runs probably in softirq :)

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 crypto/ansi_cprng.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index d80ed4c..ff00b58 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -187,7 +187,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
 /* Our exported functions */
 static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
 {
-	unsigned long flags;
 	unsigned char *ptr = buf;
 	unsigned int byte_count = (unsigned int)nbytes;
 	int err;
@@ -196,7 +195,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
 	if (nbytes < 0)
 		return -EINVAL;
 
-	spin_lock_irqsave(&ctx->prng_lock, flags);
+	spin_lock_bh(&ctx->prng_lock);
 
 	err = -EINVAL;
 	if (ctx->flags & PRNG_NEED_RESET)
@@ -268,7 +267,7 @@ empty_rbuf:
 		goto remainder;
 
 done:
-	spin_unlock_irqrestore(&ctx->prng_lock, flags);
+	spin_unlock_bh(&ctx->prng_lock);
 	dbgprint(KERN_CRIT "returning %d from get_prng_bytes in context %p\n",
 		err, ctx);
 	return err;
@@ -287,7 +286,7 @@ static int reset_prng_context(struct prng_context *ctx,
 	int rc = -EINVAL;
 	unsigned char *prng_key;
 
-	spin_lock(&ctx->prng_lock);
+	spin_lock_bh(&ctx->prng_lock);
 	ctx->flags |= PRNG_NEED_RESET;
 
 	prng_key = (key != NULL) ? key : (unsigned char *)DEFAULT_PRNG_KEY;
@@ -332,7 +331,7 @@ static int reset_prng_context(struct prng_context *ctx,
 	rc = 0;
 	ctx->flags &= ~PRNG_NEED_RESET;
 out:
-	spin_unlock(&ctx->prng_lock);
+	spin_unlock_bh(&ctx->prng_lock);
 
 	return rc;
 
-- 
1.6.3.3

[PATCH 2/2] crypto/ansi prng: alloc cipher just in in init()

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads
to:
|BUG: sleeping function called from invalid context at kernel/rwsem.c:21
|in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name: modprobe
|INFO: lockdep is turned off.
|Pid: 4926, comm: modprobe Tainted: G   M 2.6.31-rc1-22297-g5298976 #24
|Call Trace:
| [<c011dd93>] __might_sleep+0xf9/0x101
| [<c0777aa0>] down_read+0x16/0x68
| [<c048bf04>] crypto_alg_lookup+0x16/0x34
| [<c048bf52>] crypto_larval_lookup+0x30/0xf9
| [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
| [<c048c13e>] crypto_alloc_base+0x1e/0x64
| [<c04bf991>] reset_prng_context+0xab/0x13f
| [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
| [<c04bfce1>] cprng_init+0x2a/0x42
| [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
| [<c048c153>] crypto_alloc_base+0x33/0x64
| [<c04933c9>] alg_test_cprng+0x30/0x1f4
| [<c0493329>] alg_test+0x12f/0x19f
| [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
| [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
| [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
| [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
| [<c010113c>] _stext+0x54/0x12c
| [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
| [<c01398a3>] ? up_read+0x16/0x2b
| [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
| [<c014ee8d>] sys_init_module+0xa9/0x1bf
| [<c010292b>] sysenter_do_call+0x12/0x32

because a spin lock is held and crypto_alloc_base() may sleep.
There is no reason to re-allocate the cipher, the state is resetted in
->setkey(). This move it to init.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 crypto/ansi_cprng.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index ff00b58..259d2de 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -307,17 +307,6 @@ static int reset_prng_context(struct prng_context *ctx,
 	memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
 	memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
 
-	if (ctx->tfm)
-		crypto_free_cipher(ctx->tfm);
-
-	ctx->tfm = crypto_alloc_cipher("aes", 0, 0);
-	if (IS_ERR(ctx->tfm)) {
-		dbgprint(KERN_CRIT "Failed to alloc tfm for context %p\n",
-			ctx);
-		ctx->tfm = NULL;
-		goto out;
-	}

Re: [PATCH 2/2] crypto/ansi prng: alloc cipher just in in init()

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2009-06-29 23:45:08 [+0200]:

>From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
>
>As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads

Neil, the patches are untested. Could you please verify them?

Sebastian

Re: [PATCH 2/2] crypto/ansi prng: alloc cipher just in in init()

[Neil Horman]

On Mon, Jun 29, 2009 at 11:48:21PM +0200, Sebastian Andrzej Siewior wrote:
> * Sebastian Andrzej Siewior | 2009-06-29 23:45:08 [+0200]:
> 
> >From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> >
> >As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads
> 
> Neil, the patches are untested. Could you please verify them?
> 
> Sebastian
> 

Yeah, you'll have to give me a day or two to get to it, but I'll test them

Neil

Re: [PATCH 1/2] crypto/ansi prng: use just a BH lock

[Neil Horman]

On Mon, Jun 29, 2009 at 11:44:30PM +0200, Sebastian Andrzej Siewior wrote:
> From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> 
> the current code uses a mix of sping_lock() & spin_lock_irqsave(). This can
> lead to deadlock with the correct timming & cprng_get_random() + cprng_reset()
> sequence.
> I've converted them to bottom half locks since all three user grab just a BH
> lock so this runs probably in softirq :)
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

There are more than just 3 users of this rng.  This is the rng that gets
allocated in the event that someone calls crypto_alloc_rng("stdrng"), and
several ciphers use that, and in turn various ipsec implementations use those
ciphers.  That said, I've built it and tested it, and don't currently see any
users of this api from within interrupt context (i.e. no lockdep warnings).  I
think we may want to make this api accessible within interrupt context, but that
can probably wait until we have a user in said context, to find the best way to
do that.  Herbert, can you apply this to your tree?  Thanks!

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
>  crypto/ansi_cprng.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
> index d80ed4c..ff00b58 100644
> --- a/crypto/ansi_cprng.c
> +++ b/crypto/ansi_cprng.c
> @@ -187,7 +187,6 @@ static int _get_more_prng_bytes(struct prng_context *ctx)
>  /* Our exported functions */
>  static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
>  {
> -	unsigned long flags;
>  	unsigned char *ptr = buf;
>  	unsigned int byte_count = (unsigned int)nbytes;
>  	int err;
> @@ -196,7 +195,7 @@ static int get_prng_bytes(char *buf, size_t nbytes, struct prng_context *ctx)
>  	if (nbytes < 0)
>  		return -EINVAL;
>  
> -	spin_lock_irqsave(&ctx->prng_lock, flags);
> +	spin_lock_bh(&ctx->prng_lock);
>  
>  	err = -EINVAL;
>  	if (ctx->flags & PRNG_NEED_RESET)
> @@ -268,7 +267,7 @@ empty_rbuf:
>  		goto remainder;
>  
>  done:
> -	spin_unlock_irqrestore(&ctx->prng_lock, flags);
> +	spin_unlock_bh(&ctx->prng_lock);
>  	dbgprint(KERN_CRIT "returning %d from get_prng_bytes in context %p\n",
>  		err, ctx);
>  	return err;
> @@ -287,7 +286,7 @@ static int reset_prng_context(struct prng_context *ctx,
>  	int rc = -EINVAL;
>  	unsigned char *prng_key;
>  
> -	spin_lock(&ctx->prng_lock);
> +	spin_lock_bh(&ctx->prng_lock);
>  	ctx->flags |= PRNG_NEED_RESET;
>  
>  	prng_key = (key != NULL) ? key : (unsigned char *)DEFAULT_PRNG_KEY;
> @@ -332,7 +331,7 @@ static int reset_prng_context(struct prng_context *ctx,
>  	rc = 0;
>  	ctx->flags &= ~PRNG_NEED_RESET;
>  out:
> -	spin_unlock(&ctx->prng_lock);
> +	spin_unlock_bh(&ctx->prng_lock);
>  
>  	return rc;
>  
> -- 
> 1.6.3.3
> 
> 

Re: [PATCH 2/2] crypto/ansi prng: alloc cipher just in in init()

[Neil Horman]

On Mon, Jun 29, 2009 at 11:45:08PM +0200, Sebastian Andrzej Siewior wrote:
> From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> 
> As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads
> to:
> |BUG: sleeping function called from invalid context at kernel/rwsem.c:21
> |in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name: modprobe
> |INFO: lockdep is turned off.
> |Pid: 4926, comm: modprobe Tainted: G   M 2.6.31-rc1-22297-g5298976 #24
> |Call Trace:
> | [<c011dd93>] __might_sleep+0xf9/0x101
> | [<c0777aa0>] down_read+0x16/0x68
> | [<c048bf04>] crypto_alg_lookup+0x16/0x34
> | [<c048bf52>] crypto_larval_lookup+0x30/0xf9
> | [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
> | [<c048c13e>] crypto_alloc_base+0x1e/0x64
> | [<c04bf991>] reset_prng_context+0xab/0x13f
> | [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
> | [<c04bfce1>] cprng_init+0x2a/0x42
> | [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
> | [<c048c153>] crypto_alloc_base+0x33/0x64
> | [<c04933c9>] alg_test_cprng+0x30/0x1f4
> | [<c0493329>] alg_test+0x12f/0x19f
> | [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
> | [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
> | [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
> | [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
> | [<c010113c>] _stext+0x54/0x12c
> | [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
> | [<c01398a3>] ? up_read+0x16/0x2b
> | [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
> | [<c014ee8d>] sys_init_module+0xa9/0x1bf
> | [<c010292b>] sysenter_do_call+0x12/0x32
> 
> because a spin lock is held and crypto_alloc_base() may sleep.
> There is no reason to re-allocate the cipher, the state is resetted in
> ->setkey(). This move it to init.
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

NAK.  Functionally its, ok. But in the conversion, you set ctx->tfm to NULL, and
then return PTR_ERR(ctx->tfm), which is going to return success erroneously
(NULL == 0 == success).. Grab the value of tfm before setting it to null and
return that instead, please

Regards
Neil

> ---
>  crypto/ansi_cprng.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
> index ff00b58..259d2de 100644
> --- a/crypto/ansi_cprng.c
> +++ b/crypto/ansi_cprng.c
> @@ -307,17 +307,6 @@ static int reset_prng_context(struct prng_context *ctx,
>  	memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
>  	memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
>  
> -	if (ctx->tfm)
> -		crypto_free_cipher(ctx->tfm);
> -
> -	ctx->tfm = crypto_alloc_cipher("aes", 0, 0);
> -	if (IS_ERR(ctx->tfm)) {
> -		dbgprint(KERN_CRIT "Failed to alloc tfm for context %p\n",
> -			ctx);
> -		ctx->tfm = NULL;
> -		goto out;
> -	}
> -
>  	ctx->rand_data_valid = DEFAULT_BLK_SZ;
>  
>  	ret = crypto_cipher_setkey(ctx->tfm, prng_key, klen);
> @@ -342,6 +331,13 @@ static int cprng_init(struct crypto_tfm *tfm)
>  	struct prng_context *ctx = crypto_tfm_ctx(tfm);
>  
>  	spin_lock_init(&ctx->prng_lock);
> +	ctx->tfm = crypto_alloc_cipher("aes", 0, 0);
> +	if (IS_ERR(ctx->tfm)) {
> +		dbgprint(KERN_CRIT "Failed to alloc tfm for context %p\n",
> +				ctx);
> +		ctx->tfm = NULL;
> +		return PTR_ERR(ctx->tfm);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Use after assignment is bad here :)

> +	}
>  
>  	if (reset_prng_context(ctx, NULL, DEFAULT_PRNG_KSZ, NULL, NULL) < 0)
>  		return -EINVAL;
> -- 
> 1.6.3.3
> 
> 

[PATCH v2] crypto/ansi prng: alloc cipher just in in init()

[Sebastian Andrzej Siewior]

As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads
to:
|BUG: sleeping function called from invalid context at kernel/rwsem.c:21
|in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name: modprobe
|INFO: lockdep is turned off.
|Pid: 4926, comm: modprobe Tainted: G   M 2.6.31-rc1-22297-g5298976 #24
|Call Trace:
| [<c011dd93>] __might_sleep+0xf9/0x101
| [<c0777aa0>] down_read+0x16/0x68
| [<c048bf04>] crypto_alg_lookup+0x16/0x34
| [<c048bf52>] crypto_larval_lookup+0x30/0xf9
| [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
| [<c048c13e>] crypto_alloc_base+0x1e/0x64
| [<c04bf991>] reset_prng_context+0xab/0x13f
| [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
| [<c04bfce1>] cprng_init+0x2a/0x42
| [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
| [<c048c153>] crypto_alloc_base+0x33/0x64
| [<c04933c9>] alg_test_cprng+0x30/0x1f4
| [<c0493329>] alg_test+0x12f/0x19f
| [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
| [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
| [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
| [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
| [<c010113c>] _stext+0x54/0x12c
| [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
| [<c01398a3>] ? up_read+0x16/0x2b
| [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
| [<c014ee8d>] sys_init_module+0xa9/0x1bf
| [<c010292b>] sysenter_do_call+0x12/0x32

because a spin lock is held and crypto_alloc_base() may sleep.
There is no reason to re-allocate the cipher, the state is resetted in
->setkey(). This patches makes the cipher allocation a one time thing and
moves it to init.

Reported-by: Eric Sesterhenn <eric.sesterhenn@lsexperts.de>
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
v2: - remove crypto_free_cipher() from reset
    - don'r return 0 in error case in reset
v1: initial shot

Neil, what about that one? Setting tfm to NULL and grabbing the return
code earler would make the patch a little longer.

 crypto/ansi_cprng.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index ff00b58..5357ba7 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -283,7 +283,6 @@ static int reset_prng_context(struct prng_context *ctx,
 			      unsigned char *V, unsigned char *DT)
 {
 	int ret;
-	int rc = -EINVAL;
 	unsigned char *prng_key;
 
 	spin_lock_bh(&ctx->prng_lock);
@@ -307,34 +306,20 @@ static int reset_prng_context(struct prng_context *ctx,
 	memset(ctx->rand_data, 0, DEFAULT_BLK_SZ);
 	memset(ctx->last_rand_data, 0, DEFAULT_BLK_SZ);
 
-	if (ctx->tfm)
-		crypto_free_cipher(ctx->tfm);
-
-	ctx->tfm = crypto_alloc_cipher("aes", 0, 0);
-	if (IS_ERR(ctx->tfm)) {
-		dbgprint(KERN_CRIT "Failed to alloc tfm for context %p\n",
-			ctx);
-		ctx->tfm = NULL;
-		goto out;
-	}
-
 	ctx->rand_data_valid = DEFAULT_BLK_SZ;
 
 	ret = crypto_cipher_setkey(ctx->tfm, prng_key, klen);
 	if (ret) {
 		dbgprint(KERN_CRIT "PRNG: setkey() failed flags=%x\n",
 			crypto_cipher_get_flags(ctx->tfm));
-		crypto_free_cipher(ctx->tfm);
 		goto out;
 	}
 
-	rc = 0;
+	ret = 0;
 	ctx->flags &= ~PRNG_NEED_RESET;
 out:
 	spin_unlock_bh(&ctx->prng_lock);
-
-	return rc;

Re: [PATCH v2] crypto/ansi prng: alloc cipher just in in init()

[Neil Horman]

On Tue, Jun 30, 2009 at 09:55:18PM +0200, Sebastian Andrzej Siewior wrote:
> As reported by Eric Sesterhenn the re-allocation of the cipher in reset leads
> to:
> |BUG: sleeping function called from invalid context at kernel/rwsem.c:21
> |in_atomic(): 1, irqs_disabled(): 0, pid: 4926, name: modprobe
> |INFO: lockdep is turned off.
> |Pid: 4926, comm: modprobe Tainted: G   M 2.6.31-rc1-22297-g5298976 #24
> |Call Trace:
> | [<c011dd93>] __might_sleep+0xf9/0x101
> | [<c0777aa0>] down_read+0x16/0x68
> | [<c048bf04>] crypto_alg_lookup+0x16/0x34
> | [<c048bf52>] crypto_larval_lookup+0x30/0xf9
> | [<c048c038>] crypto_alg_mod_lookup+0x1d/0x62
> | [<c048c13e>] crypto_alloc_base+0x1e/0x64
> | [<c04bf991>] reset_prng_context+0xab/0x13f
> | [<c04e5cfc>] ? __spin_lock_init+0x27/0x51
> | [<c04bfce1>] cprng_init+0x2a/0x42
> | [<c048bb4c>] __crypto_alloc_tfm+0xfa/0x128
> | [<c048c153>] crypto_alloc_base+0x33/0x64
> | [<c04933c9>] alg_test_cprng+0x30/0x1f4
> | [<c0493329>] alg_test+0x12f/0x19f
> | [<c0177f1f>] ? __alloc_pages_nodemask+0x14d/0x481
> | [<d09219e2>] do_test+0xf9d/0x163f [tcrypt]
> | [<d0920de6>] do_test+0x3a1/0x163f [tcrypt]
> | [<d0926035>] tcrypt_mod_init+0x35/0x7c [tcrypt]
> | [<c010113c>] _stext+0x54/0x12c
> | [<d0926000>] ? tcrypt_mod_init+0x0/0x7c [tcrypt]
> | [<c01398a3>] ? up_read+0x16/0x2b
> | [<c0139fc4>] ? __blocking_notifier_call_chain+0x40/0x4c
> | [<c014ee8d>] sys_init_module+0xa9/0x1bf
> | [<c010292b>] sysenter_do_call+0x12/0x32
> 
> because a spin lock is held and crypto_alloc_base() may sleep.
> There is no reason to re-allocate the cipher, the state is resetted in
> ->setkey(). This patches makes the cipher allocation a one time thing and
> moves it to init.
> 
> Reported-by: Eric Sesterhenn <eric.sesterhenn@lsexperts.de>
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

I think this looks better, yeah, have you tested this?  If not, give it a quick
run please, and I'll ack it.
Neil


> 

Re: [PATCH v2] crypto/ansi prng: alloc cipher just in in init()

[Neil Horman]

On Wed, Jul 01, 2009 at 09:25:17AM +0200, Sebastian Andrzej Siewior wrote:
> * Neil Horman | 2009-06-30 20:06:48 [-0400]:
> 
> >I think this looks better, yeah, have you tested this?  If not, give it a quick
> >run please, and I'll ack it.
> I've built it and started
> | modprobe tcrypt mode=150
> and I ended up with:
> | alg: No test for stdrng (ansi_cprng)
> 
> So this should be fine I guess.
> 
> >Neil
> 
> Sebastian
> 

Yeah, ok, that should be good, although I'll make a note and fix the aliasing
issue in the testmgr code

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Herbert, can you pull this into cryptodev please?

Re: [PATCH v2] crypto/ansi prng: alloc cipher just in in init()

[Herbert Xu]

On Wed, Jul 01, 2009 at 06:36:16AM -0400, Neil Horman wrote:
>
> Herbert, can you pull this into cryptodev please?

Both patches applied.  Thanks everyone!
-- 
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