cryptoapi: Fix sleeping

PATCH In review Matt Mackall 5 participants 15 messages Aug 13, 2003 – Aug 15, 2003

Subsystems (2)

Crypto API Scheduler

lore

Share & tools

lore

Matt Mackall

author

2003-08-13 23:40 UTC #0 lore raw

PATCH 1/1 cryptoapi: Fix sleeping

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

crypto/internal.h +1 -1

diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
@@ -37,7 +37,7 @@
 
 static inline void crypto_yield(struct crypto_tfm *tfm)
 {
-	if (!in_softirq())
+	if (!in_atomic())
 		cond_resched();
 }

Signature · 1 line

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

Jeff Garzik

2003-08-14 00:21 UTC #1 lore raw

crypto/internal.h line 41

Quoted 13 lines · 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
crypto/internal.h +1 -1
@@ -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

Robert Love

2003-08-14 00:34 UTC #2 lore raw

Quoted 1 line · Jeff Garzik

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

Matt Mackall

author

2003-08-14 01:58 UTC #3 lore raw

Quoted 13 lines · Robert Love

Quoted 1 line · Jeff Garzik
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).

Quoted 3 lines

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.

Signature · 1 line

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

Robert Love

2003-08-14 16:29 UTC #4 lore raw

Quoted 5 lines · Matt Mackall

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

Matt Mackall

author

2003-08-14 16:33 UTC #5 lore raw

Quoted 10 lines · Robert Love

Quoted 5 lines · Matt Mackall
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.

Signature · 1 line

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

Andrew Morton

2003-08-14 00:58 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 10 lines · Jeff Garzik

Quoted 8 lines · 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
crypto/internal.h +0 -0
@@ -37,7 +37,7 @@
 

Quoted 9 lines

crypto/internal.h line 41
Quoted 5 lines
crypto/internal.h +1 -1
 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.

Matt Mackall

author

2003-08-14 02:03 UTC #7 lore raw

Quoted 20 lines · Andrew Morton

Quoted 9 lines · Jeff Garzik
Quoted 7 lines · 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
+++ work/crypto/internal.h 2003-08-12 14:38:54.000000000 -0500
(quoted diff) +0 -0
@@ -37,7 +37,7 @@
 
Quoted 9 lines
crypto/internal.h line 41
Quoted 5 lines
crypto/internal.h +1 -1
 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() ?

Quoted 8 lines

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.

Signature · 1 line

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

Matt Mackall

author

2003-08-14 07:15 UTC #8 lore raw

Quoted 23 lines · Matt Mackall

Quoted 22 lines · Andrew Morton
Quoted 9 lines · Jeff Garzik
Quoted 7 lines · 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
+++ work/crypto/internal.h 2003-08-12 14:38:54.000000000 -0500
(quoted diff) +0 -0
@@ -37,7 +37,7 @@
 
Quoted 9 lines
Review line 41
Quoted 5 lines
(quoted diff) +1 -1
 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.

Quoted 8 lines

Quoted 6 lines
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:

crypto/api.c +2 -1

diff -urN -X dontdiff orig/crypto/api.c work/crypto/api.c
@@ -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:

crypto/internal.h +1 -1

diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
@@ -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();
 }

include/linux/crypto.h +5 -1

diff -urN -X dontdiff orig/include/linux/crypto.h work/include/linux/crypto.h
@@ -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.
  */

include/linux/sched.h +1 -0

diff -urN -X dontdiff orig/include/linux/sched.h work/include/linux/sched.h
@@ -824,6 +824,7 @@
 extern void __cond_resched(void);
 static inline void cond_resched(void)
 {
+	might_sleep();
 	if (need_resched())
 		__cond_resched();
 }

Signature · 1 line

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

James Morris

2003-08-14 17:08 UTC #9 lore raw

Quoted 3 lines · Matt Mackall

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.

include/linux/crypto.h line 40

Quoted 5 lines

#define CRYPTO_TFM_MODE_MASK 0x000000ff
#define CRYPTO_TFM_REQ_MASK 0x000fff00
include/linux/crypto.h +2 -1
-#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

Signature · 2 lines

James Morris
<[email protected]>

Matt Mackall

author

2003-08-14 20:19 UTC #10 lore raw

Quoted 8 lines · James Morris

Quoted 3 lines · Matt Mackall
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.

Quoted 8 lines

include/linux/crypto.h line 40
Quoted 5 lines
#define CRYPTO_TFM_MODE_MASK 0x000000ff
#define CRYPTO_TFM_REQ_MASK 0x000fff00
include/linux/crypto.h +2 -1
-#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?

Signature · 1 line

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

James Morris

2003-08-15 03:39 UTC #11 lore raw

Quoted 2 lines · Matt Mackall

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

Ok.

Quoted 3 lines

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

Signature · 2 lines

James Morris
<[email protected]>

Matt Mackall

author

2003-08-15 03:51 UTC #12 lore raw

Quoted 7 lines · James Morris

Quoted 2 lines · Matt Mackall
Leaves no room actually. I figured this would be easy to move around after the fact.
Ok.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 1 line

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

James Morris

2003-08-15 04:07 UTC #13 lore raw

Quoted 4 lines · Matt Mackall

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

Signature · 2 lines

James Morris
<[email protected]>

Matt Mackall

author

2003-08-14 01:54 UTC #14 lore raw

In reply to Jeff Garzik

Quoted 8 lines · Jeff Garzik

Quoted 7 lines · 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
+++ work/crypto/internal.h 2003-08-12 14:38:54.000000000 -0500
(quoted diff) +0 -0
@@ -37,7 +37,7 @@
 

Quoted 8 lines

crypto/internal.h line 41
Quoted 5 lines
crypto/internal.h +1 -1
 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.

Signature · 1 line

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