[PATCH] cryptoapi: Fix sleeping

[PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

We need in_atomic() so that we can call from regions where preempt is
disabled, for instance when using per_cpu crypto tfms.

diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
--- orig/crypto/internal.h	2003-07-13 22:29:11.000000000 -0500
+++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
@@ -37,7 +37,7 @@
 
 static inline void crypto_yield(struct crypto_tfm *tfm)
 {
-	if (!in_softirq())
+	if (!in_atomic())
 		cond_resched();
 }
 


-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[Jeff Garzik]

Matt Mackall wrote:
> We need in_atomic() so that we can call from regions where preempt is
> disabled, for instance when using per_cpu crypto tfms.
> 
> diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
> --- orig/crypto/internal.h	2003-07-13 22:29:11.000000000 -0500
> +++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
> @@ -37,7 +37,7 @@
>  
>  static inline void crypto_yield(struct crypto_tfm *tfm)
>  {
> -	if (!in_softirq())
> +	if (!in_atomic())
>  		cond_resched();


Do you really want to schedule inside preempt_disable() ?

As an aside, I think it would be nice if cond_resched could figure out 
for itself whether it is ok to schedule().  Completely eliminate the 
above test entirely, moving it into cond_resched().

	Jeff

Re: [PATCH] cryptoapi: Fix sleeping

[Robert Love]

On Wed, 2003-08-13 at 17:21, Jeff Garzik wrote:

> Do you really want to schedule inside preempt_disable() ?

in_atomic() includes a check for preempt_disable() ... that is actually
all it checks (the preempt_count).  So this fix prevents that.

This patch is interesting, though, because if right now we are
scheduling in the middle of per-CPU code there is a bug (regardless of
kernel preemption -- and with kernel preemption off, the in_atomic()
check might return false even though the code is accessing per-processor
data).

So I think what we really want is to just never call this crypto_yield()
thing when in any sort of critical section, which includes any
per-processor data.

	Robert Love

Re: [PATCH] cryptoapi: Fix sleeping

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Matt Mackall wrote:
> > We need in_atomic() so that we can call from regions where preempt is
> > disabled, for instance when using per_cpu crypto tfms.
> > 
> > diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
> > --- orig/crypto/internal.h	2003-07-13 22:29:11.000000000 -0500
> > +++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
> > @@ -37,7 +37,7 @@
> >  
> >  static inline void crypto_yield(struct crypto_tfm *tfm)
> >  {
> > -	if (!in_softirq())
> > +	if (!in_atomic())
> >  		cond_resched();
> 
> 
> Do you really want to schedule inside preempt_disable() ?
> 

in_atomic() returns false inside spin_lock() on non-preemptive kernels.

Either this code needs to be removed altogether or it should be changed to

	BUG_ON(in_atomic());
	cond_resched();

and the callers should be fixed up.

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Wed, Aug 13, 2003 at 08:21:05PM -0400, Jeff Garzik wrote:
> Matt Mackall wrote:
> >We need in_atomic() so that we can call from regions where preempt is
> >disabled, for instance when using per_cpu crypto tfms.
> >
> >diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
> >+++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
> >@@ -37,7 +37,7 @@
> > 
> > static inline void crypto_yield(struct crypto_tfm *tfm)
> > {
> >-	if (!in_softirq())
> >+	if (!in_atomic())
> > 		cond_resched();
> 
> 
> Do you really want to schedule inside preempt_disable() ?

No, that's the point. Cryptoapi has hidden scheduling points and the
above code is what's used to check that it doesn't do so when it ought
not. But it was broken inside of no-preempt sections.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Wed, Aug 13, 2003 at 05:34:12PM -0700, Robert Love wrote:
> On Wed, 2003-08-13 at 17:21, Jeff Garzik wrote:
> 
> > Do you really want to schedule inside preempt_disable() ?
> 
> in_atomic() includes a check for preempt_disable() ... that is actually
> all it checks (the preempt_count).  So this fix prevents that.
> 
> This patch is interesting, though, because if right now we are
> scheduling in the middle of per-CPU code there is a bug (regardless of
> kernel preemption -- and with kernel preemption off, the in_atomic()
> check might return false even though the code is accessing per-processor
> data).
> 
> So I think what we really want is to just never call this crypto_yield()
> thing when in any sort of critical section, which includes any
> per-processor data.

This is part of cryptoapi and given the large chunks of work you could
potentially hand to it, it's probably a good idea for it to work this
way. You hand it a long list of sg segments, it does the transform and
reschedules if it thinks it's safe. But its test of when it was safe
was not complete.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Wed, Aug 13, 2003 at 05:44:36PM -0700, Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> > Matt Mackall wrote:
> > > We need in_atomic() so that we can call from regions where preempt is
> > > disabled, for instance when using per_cpu crypto tfms.
> > > 
> > > diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
> > > +++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
> > > @@ -37,7 +37,7 @@
> > >  
> > >  static inline void crypto_yield(struct crypto_tfm *tfm)
> > >  {
> > > -	if (!in_softirq())
> > > +	if (!in_atomic())
> > >  		cond_resched();
> > 
> > 
> > Do you really want to schedule inside preempt_disable() ?
> > 
> 
> in_atomic() returns false inside spin_lock() on non-preemptive kernels.
> 
> Either this code needs to be removed altogether or it should be changed to
> 
> 	BUG_ON(in_atomic());
> 	cond_resched();
> 
> and the callers should be fixed up.

Cryptoapi probably needs a flag for skipping the yield. I'll look into it.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Wed, Aug 13, 2003 at 09:03:23PM -0500, Matt Mackall wrote:
> On Wed, Aug 13, 2003 at 05:44:36PM -0700, Andrew Morton wrote:
> > Jeff Garzik <jgarzik@pobox.com> wrote:
> > >
> > > Matt Mackall wrote:
> > > > We need in_atomic() so that we can call from regions where preempt is
> > > > disabled, for instance when using per_cpu crypto tfms.
> > > > 
> > > > diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
> > > > +++ work/crypto/internal.h	2003-08-12 14:38:54.000000000 -0500
> > > > @@ -37,7 +37,7 @@
> > > >  
> > > >  static inline void crypto_yield(struct crypto_tfm *tfm)
> > > >  {
> > > > -	if (!in_softirq())
> > > > +	if (!in_atomic())
> > > >  		cond_resched();
> > > 
> > > 
> > > Do you really want to schedule inside preempt_disable() ?
> > > 
> > 
> > in_atomic() returns false inside spin_lock() on non-preemptive kernels.
> > 
> > Either this code needs to be removed altogether or it should be changed to
> > 
> > 	BUG_ON(in_atomic());
> > 	cond_resched();
> > 
> > and the callers should be fixed up.
> 
> Cryptoapi probably needs a flag for skipping the yield. I'll look into it.

Ok, for reference, the code calling yield is stuff like this buried in
cryptoapi:

static void update(struct crypto_tfm *tfm,
                   struct scatterlist *sg, unsigned int nsg)
{
        unsigned int i;
         
        for (i = 0; i < nsg; i++) {
                char *p = crypto_kmap(sg[i].page, 0) + sg[i].offset;
                tfm->__crt_alg->cra_digest.dia_update(crypto_tfm_ctx(tfm),
                                                      p,
						      sg[i].length);
                crypto_kunmap(p, 0);
                crypto_yield(tfm);
        }
}

It's basically trying to be friendly. Since we can't really detect
when it's safe to do such yields, we should be explicitly flag the
uses where its ok. Something like this:

diff -urN -X dontdiff orig/crypto/api.c work/crypto/api.c
--- orig/crypto/api.c	2003-07-13 22:38:38.000000000 -0500
+++ work/crypto/api.c	2003-08-14 01:53:07.000000000 -0500
@@ -53,7 +53,8 @@
 
 static int crypto_init_flags(struct crypto_tfm *tfm, u32 flags)
 {
-	tfm->crt_flags = 0;
+	tfm->crt_flags = flags & CRYPTO_TFM_API_MASK;
+	flags &= ~CRYPTO_TFM_API_MASK;
 	
 	switch (crypto_tfm_alg_type(tfm)) {
 	case CRYPTO_ALG_TYPE_CIPHER:
diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
--- orig/crypto/internal.h	2003-07-13 22:29:11.000000000 -0500
+++ work/crypto/internal.h	2003-08-14 01:53:28.000000000 -0500
@@ -37,7 +37,7 @@
 
 static inline void crypto_yield(struct crypto_tfm *tfm)
 {
-	if (!in_softirq())
+	if (tfm->crt_flags & CRYPTO_TFM_API_YIELD)
 		cond_resched();
 }
 
diff -urN -X dontdiff orig/include/linux/crypto.h work/include/linux/crypto.h
--- orig/include/linux/crypto.h	2003-07-13 22:32:32.000000000 -0500
+++ work/include/linux/crypto.h	2003-08-14 01:53:07.000000000 -0500
@@ -36,7 +36,8 @@
  */
 #define CRYPTO_TFM_MODE_MASK		0x000000ff
 #define CRYPTO_TFM_REQ_MASK		0x000fff00
-#define CRYPTO_TFM_RES_MASK		0xfff00000
+#define CRYPTO_TFM_RES_MASK		0x7ff00000
+#define CRYPTO_TFM_API_MASK		0x80000000
 
 #define CRYPTO_TFM_MODE_ECB		0x00000001
 #define CRYPTO_TFM_MODE_CBC		0x00000002
@@ -50,6 +51,9 @@
 #define CRYPTO_TFM_RES_BAD_BLOCK_LEN 	0x00800000
 #define CRYPTO_TFM_RES_BAD_FLAGS 	0x01000000
 
+/* Allow for rescheduling after processing each sg element */
+#define CRYPTO_TFM_API_YIELD		0x80000000
+
 /*
  * Miscellaneous stuff.
  */
diff -urN -X dontdiff orig/include/linux/sched.h work/include/linux/sched.h
--- orig/include/linux/sched.h	2003-08-14 01:53:04.000000000 -0500
+++ work/include/linux/sched.h	2003-08-14 01:53:07.000000000 -0500
@@ -824,6 +824,7 @@
 extern void __cond_resched(void);
 static inline void cond_resched(void)
 {
+	might_sleep();
 	if (need_resched())
 		__cond_resched();
 }



-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[Robert Love]

On Wed, 2003-08-13 at 18:58, Matt Mackall wrote:

> This is part of cryptoapi and given the large chunks of work you could
> potentially hand to it, it's probably a good idea for it to work this
> way. You hand it a long list of sg segments, it does the transform and
> reschedules if it thinks it's safe. But its test of when it was safe
> was not complete.

Right.  My concern is that you said sometimes it is called when
preemption is disabled.

I assume because it is accessing per-processor data.

It is illegal to reschedule -- regardless of kernel preemption -- while
in the middle of a critical section accessing per-processor data.

	Robert Love

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Thu, Aug 14, 2003 at 09:29:20AM -0700, Robert Love wrote:
> On Wed, 2003-08-13 at 18:58, Matt Mackall wrote:
> 
> > This is part of cryptoapi and given the large chunks of work you could
> > potentially hand to it, it's probably a good idea for it to work this
> > way. You hand it a long list of sg segments, it does the transform and
> > reschedules if it thinks it's safe. But its test of when it was safe
> > was not complete.
> 
> Right.  My concern is that you said sometimes it is called when
> preemption is disabled.

Sure, but it's called here rather indirectly. Apparently I'm the first
person to try using cryptoapi with per_cpu. And at present there's
nothing in the api to tell it "don't do that" - but see my followup
patch.
 
-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[James Morris]

On Thu, 14 Aug 2003, Matt Mackall wrote:

> It's basically trying to be friendly. Since we can't really detect
> when it's safe to do such yields, we should be explicitly flag the
> uses where its ok. Something like this:

I think this is the best approach.

>  #define CRYPTO_TFM_MODE_MASK		0x000000ff
>  #define CRYPTO_TFM_REQ_MASK		0x000fff00
> -#define CRYPTO_TFM_RES_MASK		0xfff00000
> +#define CRYPTO_TFM_RES_MASK		0x7ff00000
> +#define CRYPTO_TFM_API_MASK		0x80000000

This doesn't leave much room for API flags -- the CRYPTO_TFM_REQ_MASK 
could be made smaller.


- James
-- 
James Morris
<jmorris@intercode.com.au>

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Fri, Aug 15, 2003 at 03:08:12AM +1000, James Morris wrote:
> On Thu, 14 Aug 2003, Matt Mackall wrote:
> 
> > It's basically trying to be friendly. Since we can't really detect
> > when it's safe to do such yields, we should be explicitly flag the
> > uses where its ok. Something like this:
> 
> I think this is the best approach.
> 
> >  #define CRYPTO_TFM_MODE_MASK		0x000000ff
> >  #define CRYPTO_TFM_REQ_MASK		0x000fff00
> > -#define CRYPTO_TFM_RES_MASK		0xfff00000
> > +#define CRYPTO_TFM_RES_MASK		0x7ff00000
> > +#define CRYPTO_TFM_API_MASK		0x80000000
> 
> This doesn't leave much room for API flags -- the CRYPTO_TFM_REQ_MASK 
> could be made smaller.

Leaves no room actually. I figured this would be easy to move around
after the fact.

On the subject of flags, what's the best way for an algorithm init
function to get at the tfm structures (and thereby the flags) given a
ctxt? Pointer math on a ctxt?

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[James Morris]

On Thu, 14 Aug 2003, Matt Mackall wrote:

> Leaves no room actually. I figured this would be easy to move around
> after the fact.

Ok.

> On the subject of flags, what's the best way for an algorithm init
> function to get at the tfm structures (and thereby the flags) given a
> ctxt? Pointer math on a ctxt?

The algorithms should not access the tfm structure.  In the case of 
ciphers, we pass the tfm flags in via setkey.

What do you need this for?


- James
-- 
James Morris
<jmorris@intercode.com.au>

Re: [PATCH] cryptoapi: Fix sleeping

[Matt Mackall]

On Fri, Aug 15, 2003 at 01:39:03PM +1000, James Morris wrote:
> On Thu, 14 Aug 2003, Matt Mackall wrote:
> 
> > Leaves no room actually. I figured this would be easy to move around
> > after the fact.
> 
> Ok.
> 
> > On the subject of flags, what's the best way for an algorithm init
> > function to get at the tfm structures (and thereby the flags) given a
> > ctxt? Pointer math on a ctxt?
> 
> The algorithms should not access the tfm structure.  In the case of 
> ciphers, we pass the tfm flags in via setkey.
> 
> What do you need this for?

This is back to turning off padding and getting at the raw hash
transforms, and we were talking about doing this via flags. The
FIPS-180-1 padding is done in sha1.c:digest which has no visibility to
such flags.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] cryptoapi: Fix sleeping

[James Morris]

On Thu, 14 Aug 2003, Matt Mackall wrote:

> This is back to turning off padding and getting at the raw hash
> transforms, and we were talking about doing this via flags. The
> FIPS-180-1 padding is done in sha1.c:digest which has no visibility to
> such flags.

One possibility is that we add a padding parameter to the dia_final 
method to indicate whether to perform padding.  This would be extracted 
from the tfm flags internally.

Also, a side note, please cc Ted on the hash folding discussions.


- James
-- 
James Morris
<jmorris@intercode.com.au>