HELP: IPsec reordering issue

HELP: IPsec reordering issue

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

Hi, all:

I encountered a IPsec packets reordering issue, following is the setup 
and scenario

     There is a IPSec IKEv1 tunnel between B & C
     The traffic is UDP from C to A @ 40 mbps
     Packets are coming in order at B but leaving out of order towards A
     If IPSec is disabled between B & C, there is no packet reordering.

The input and output of B is same physical interface but separated by 
two VLANs, and we have directed all our network interrupts to one core.

As per our analysis we are suspecting below is the root cause of the 
problem.
All the packets which are out of order have got -EINPROGRESS error in 
below part of the code.

File: net/ipv4/esp4.c: function esp_input
.....
   aead_request_set_callback(req, 0, esp_input_done, skb);
   aead_request_set_crypt(req, sg, sg, elen, iv);
   aead_request_set_assoc(req, asg, sizeof(*esph));

    err = crypto_aead_decrypt(req);
    if (err == -EINPROGRESS)
         goto out;

    err = esp_input_done2(skb, err);
.....

Below is the place where the packets are either decrypted immediately or 
queue for later decryption.

static int ablk_decrypt(struct ablkcipher_request *req)
{
     struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
     struct async_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);

     if (!irq_fpu_usable()) {
         struct ablkcipher_request *cryptd_req =
             ablkcipher_request_ctx(req);
         memcpy(cryptd_req, req, sizeof(*req));
         ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
         return crypto_ablkcipher_decrypt(cryptd_req);
     } else {
         struct blkcipher_desc desc;
         desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
         desc.info = req->info;
         desc.flags = 0;
         return crypto_blkcipher_crt(desc.tfm)->decrypt(
             &desc, req->dst, req->src, req->nbytes);
     }
}

Now the problem scenario is, if a packet came for decryption and 
"irq_fpu_usable()" is not usable, then it will be queued for later 
decryption and crypto_aead_decrypt will be reuturned with "-EINPROGRESS" 
error. When the packets are in queue, if some more packets came and 
"irq_fpu_usable()" is usable then those packets will be decrypted before 
the queued packets and queued packets will get out of order.

And we've figure out a patch as the attached, the basic idea is just 
queue the packets if "irq_fpu_usable()" is not usable or if there are 
already few packets queued for decryption. Else decrypt the packets.

Could anybody tell if this is a appropriate fix? Or is this reordering 
thing a real probelm? 'cause I know the IPsec doesn't guarantee order at 
all. Appreciate it very much!

the best,
thank you



[-- Attachment #2: 0001-crypto-aesni-intel-avoid-encrypt-decrypt-re.patch --]
[-- Type: text/x-diff, Size: 7184 bytes --]

>From eb0fe7074c61b4e57456c578db897928eb951db9 Mon Sep 17 00:00:00 2001
From: Ming Liu <liu.ming50@gmail.com>
Date: Thu, 31 Jul 2014 09:11:51 +0800
Subject: [PATCH] crypto: aesni-intel - avoid encrypt/decrypt re-ordering on
 particular cpu

So far, the encrypt/decrypt are asynchronously processed in softirq and
cryptd which would result in a implicit order of data, therefore leads
IPSec stack also out of order while encapsulating/decapsulating packets.

Fix by letting encrypt/decrypt are processed only in one context for a
particular period.

Signed-off-by: Ming Liu <liu.ming50@gmail.com>
---
 arch/x86/crypto/aesni-intel_glue.c | 32 ++++++++++++++++-------------
 crypto/cryptd.c                    | 42 ++++++++++++++++++++++++++++++++++++--
 include/crypto/cryptd.h            |  3 ++-
 3 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 49c552c..1f66d6e 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -341,20 +341,22 @@ static int ablk_encrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
-	if (!irq_fpu_usable()) {
-		struct ablkcipher_request *cryptd_req =
-			ablkcipher_request_ctx(req);
-		memcpy(cryptd_req, req, sizeof(*req));
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-		return crypto_ablkcipher_encrypt(cryptd_req);
-	} else {
+	if (irq_fpu_usable() && !cryptd_get_encrypt_nums(req_tfm)) {
 		struct blkcipher_desc desc;
 		desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
 		desc.info = req->info;
 		desc.flags = 0;
 		return crypto_blkcipher_crt(desc.tfm)->encrypt(
 			&desc, req->dst, req->src, req->nbytes);
+	} else {
+		struct ablkcipher_request *cryptd_req =
+			ablkcipher_request_ctx(req);
+		memcpy(cryptd_req, req, sizeof(*req));
+		cryptd_req->base.tfm = req_tfm;
+		return crypto_ablkcipher_encrypt(cryptd_req);
 	}
 }
 
@@ -362,20 +364,22 @@ static int ablk_decrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
-	if (!irq_fpu_usable()) {
-		struct ablkcipher_request *cryptd_req =
-			ablkcipher_request_ctx(req);
-		memcpy(cryptd_req, req, sizeof(*req));
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-		return crypto_ablkcipher_decrypt(cryptd_req);
-	} else {
+	if (irq_fpu_usable() && !cryptd_get_decrypt_nums(req_tfm)) {
 		struct blkcipher_desc desc;
 		desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
 		desc.info = req->info;
 		desc.flags = 0;
 		return crypto_blkcipher_crt(desc.tfm)->decrypt(
 			&desc, req->dst, req->src, req->nbytes);
+	} else {
+		struct ablkcipher_request *cryptd_req =
+			ablkcipher_request_ctx(req);
+		memcpy(cryptd_req, req, sizeof(*req));
+		cryptd_req->base.tfm = req_tfm;
+		return crypto_ablkcipher_decrypt(cryptd_req);
 	}
 }
 
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index 6e24164..0175d1a 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -28,6 +28,8 @@
 struct cryptd_cpu_queue {
 	struct crypto_queue queue;
 	struct work_struct work;
+	unsigned int encrypt_nums;
+	unsigned int decrypt_nums;
 };
 
 struct cryptd_queue {
@@ -62,6 +64,8 @@ struct cryptd_hash_request_ctx {
 };
 
 static void cryptd_queue_worker(struct work_struct *work);
+static void cryptd_blkcipher_encrypt(struct crypto_async_request *req, int err);
+static void cryptd_blkcipher_decrypt(struct crypto_async_request *req, int err);
 
 static int cryptd_init_queue(struct cryptd_queue *queue,
 			     unsigned int max_cpu_qlen)
@@ -75,6 +79,8 @@ static int cryptd_init_queue(struct cryptd_queue *queue,
 	for_each_possible_cpu(cpu) {
 		cpu_queue = per_cpu_ptr(queue->cpu_queue, cpu);
 		crypto_init_queue(&cpu_queue->queue, max_cpu_qlen);
+		cpu_queue->encrypt_nums = 0;
+		cpu_queue->decrypt_nums = 0;
 		INIT_WORK(&cpu_queue->work, cryptd_queue_worker);
 	}
 	return 0;
@@ -101,6 +107,10 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
 	cpu = get_cpu();
 	cpu_queue = this_cpu_ptr(queue->cpu_queue);
 	err = crypto_enqueue_request(&cpu_queue->queue, request);
+	if ((err != -EBUSY) && (request->complete == cryptd_blkcipher_encrypt))
+		cpu_queue->encrypt_nums++;
+	if ((err != -EBUSY) && (request->complete == cryptd_blkcipher_decrypt))
+		cpu_queue->decrypt_nums++;
 	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
 	put_cpu();
 
@@ -171,10 +181,15 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 						struct scatterlist *src,
 						unsigned int len))
 {
-	struct cryptd_blkcipher_request_ctx *rctx;
+	struct cryptd_blkcipher_request_ctx *rctx = ablkcipher_request_ctx(req);
+	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct blkcipher_desc desc;
+	struct cryptd_queue *queue;
+	struct cryptd_cpu_queue *cpu_queue;
+	crypto_completion_t complete = req->base.complete;
+	int cpu;
 
-	rctx = ablkcipher_request_ctx(req);
+	queue = cryptd_get_queue(crypto_ablkcipher_tfm(tfm));
 
 	if (unlikely(err == -EINPROGRESS))
 		goto out;
@@ -190,6 +205,13 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 out:
 	local_bh_disable();
 	rctx->complete(&req->base, err);
+	cpu = get_cpu();
+	cpu_queue = this_cpu_ptr(queue->cpu_queue);
+	if ((complete == cryptd_blkcipher_encrypt) && cpu_queue->encrypt_nums)
+		cpu_queue->encrypt_nums--;
+	if ((complete == cryptd_blkcipher_decrypt) && cpu_queue->decrypt_nums)
+		cpu_queue->decrypt_nums--;
+	put_cpu();
 	local_bh_enable();
 }
 
@@ -729,6 +751,22 @@ void cryptd_free_ahash(struct cryptd_ahash *tfm)
 }
 EXPORT_SYMBOL_GPL(cryptd_free_ahash);
 
+unsigned int cryptd_get_encrypt_nums(struct crypto_tfm *tfm)
+{
+	struct cryptd_queue *queue = cryptd_get_queue(tfm);
+	struct cryptd_cpu_queue *cpu_queue = this_cpu_ptr(queue->cpu_queue);
+	return cpu_queue->encrypt_nums;
+}
+EXPORT_SYMBOL_GPL(cryptd_get_encrypt_nums);
+
+unsigned int cryptd_get_decrypt_nums(struct crypto_tfm *tfm)
+{
+        struct cryptd_queue *queue = cryptd_get_queue(tfm);
+        struct cryptd_cpu_queue *cpu_queue = this_cpu_ptr(queue->cpu_queue);
+        return cpu_queue->decrypt_nums;
+}
+EXPORT_SYMBOL_GPL(cryptd_get_decrypt_nums);
+
 static int __init cryptd_init(void)
 {
 	int err;
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index 1c96b25..cacc717 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -41,5 +41,6 @@ struct cryptd_ahash *cryptd_alloc_ahash(const char *alg_name,
 struct crypto_shash *cryptd_ahash_child(struct cryptd_ahash *tfm);
 struct shash_desc *cryptd_shash_desc(struct ahash_request *req);
 void cryptd_free_ahash(struct cryptd_ahash *tfm);
-
+unsigned int cryptd_get_encrypt_nums(struct crypto_tfm *tfm);
+unsigned int cryptd_get_decrypt_nums(struct crypto_tfm *tfm);
 #endif
-- 
1.8.5.2.233.g932f7e4

Re: HELP: IPsec reordering issue

[Herbert Xu]

On Thu, Jul 31, 2014 at 11:07:26AM +0800, Ming Liu wrote:
>
> And we've figure out a patch as the attached, the basic idea is just
> queue the packets if "irq_fpu_usable()" is not usable or if there
> are already few packets queued for decryption. Else decrypt the
> packets.

Yes your description makes sense and I will review your patch
for inclusion.

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

Re: HELP: IPsec reordering issue

[Ming Liu]

On 07/31/2014 02:23 PM, Herbert Xu wrote:
> On Thu, Jul 31, 2014 at 11:07:26AM +0800, Ming Liu wrote:
>> And we've figure out a patch as the attached, the basic idea is just
>> queue the packets if "irq_fpu_usable()" is not usable or if there
>> are already few packets queued for decryption. Else decrypt the
>> packets.
> Yes your description makes sense and I will review your patch
> for inclusion.
Thanks, Herbert!

//Ming Liu
>
> Thanks,

Re: HELP: IPsec reordering issue

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On 07/31/2014 02:23 PM, Herbert Xu wrote:
> On Thu, Jul 31, 2014 at 11:07:26AM +0800, Ming Liu wrote:
>> And we've figure out a patch as the attached, the basic idea is just
>> queue the packets if "irq_fpu_usable()" is not usable or if there
>> are already few packets queued for decryption. Else decrypt the
>> packets.
> Yes your description makes sense and I will review your patch
> for inclusion.
Hi, Herbert:

Please review this attached patch instead, the original one has a 
problem causing the kernel crash.

the best,
thank you
>
> Thanks,


[-- Attachment #2: 0001-crypto-aesni-intel-avoid-encrypt-decrypt-re-ordering.patch --]
[-- Type: text/x-diff, Size: 7287 bytes --]

>From 0b3113eb2e690c3899382360f89281d6a64e8f3a Mon Sep 17 00:00:00 2001
From: Ming Liu <ming.liu@windriver.com>
Date: Sun, 3 Aug 2014 16:23:39 +0800
Subject: [PATCH] crypto: aesni-intel- avoid encrypt/decrypt re-ordering with
 debug

So far, the encrypt/decrypt are asynchronously processed in softirq and
cryptd which would result in a implicit order of data, therefore leads
IPSec stack also out of order while encapsulating/decapsulating packets.

Fix by letting encrypt/decrypt are processed only in one context for a
particular period.
---
 arch/x86/crypto/aesni-intel_glue.c | 32 ++++++++++++----------
 crypto/cryptd.c                    | 55 ++++++++++++++++++++++++++++++++++++--
 include/crypto/cryptd.h            |  3 ++-
 3 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 49c552c..1f66d6e 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -341,20 +341,22 @@ static int ablk_encrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
-	if (!irq_fpu_usable()) {
-		struct ablkcipher_request *cryptd_req =
-			ablkcipher_request_ctx(req);
-		memcpy(cryptd_req, req, sizeof(*req));
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-		return crypto_ablkcipher_encrypt(cryptd_req);
-	} else {
+	if (irq_fpu_usable() && !cryptd_get_encrypt_nums(req_tfm)) {
 		struct blkcipher_desc desc;
 		desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
 		desc.info = req->info;
 		desc.flags = 0;
 		return crypto_blkcipher_crt(desc.tfm)->encrypt(
 			&desc, req->dst, req->src, req->nbytes);
+	} else {
+		struct ablkcipher_request *cryptd_req =
+			ablkcipher_request_ctx(req);
+		memcpy(cryptd_req, req, sizeof(*req));
+		cryptd_req->base.tfm = req_tfm;
+		return crypto_ablkcipher_encrypt(cryptd_req);
 	}
 }
 
@@ -362,20 +364,22 @@ static int ablk_decrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_aes_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
-	if (!irq_fpu_usable()) {
-		struct ablkcipher_request *cryptd_req =
-			ablkcipher_request_ctx(req);
-		memcpy(cryptd_req, req, sizeof(*req));
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-		return crypto_ablkcipher_decrypt(cryptd_req);
-	} else {
+	if (irq_fpu_usable() && !cryptd_get_decrypt_nums(req_tfm)) {
 		struct blkcipher_desc desc;
 		desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
 		desc.info = req->info;
 		desc.flags = 0;
 		return crypto_blkcipher_crt(desc.tfm)->decrypt(
 			&desc, req->dst, req->src, req->nbytes);
+	} else {
+		struct ablkcipher_request *cryptd_req =
+			ablkcipher_request_ctx(req);
+		memcpy(cryptd_req, req, sizeof(*req));
+		cryptd_req->base.tfm = req_tfm;
+		return crypto_ablkcipher_decrypt(cryptd_req);
 	}
 }
 
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index 6e24164..6710395 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -28,6 +28,8 @@
 struct cryptd_cpu_queue {
 	struct crypto_queue queue;
 	struct work_struct work;
+	unsigned int encrypt_nums;
+	unsigned int decrypt_nums;
 };
 
 struct cryptd_queue {
@@ -62,6 +64,8 @@ struct cryptd_hash_request_ctx {
 };
 
 static void cryptd_queue_worker(struct work_struct *work);
+static void cryptd_blkcipher_encrypt(struct crypto_async_request *req, int err);
+static void cryptd_blkcipher_decrypt(struct crypto_async_request *req, int err);
 
 static int cryptd_init_queue(struct cryptd_queue *queue,
 			     unsigned int max_cpu_qlen)
@@ -75,6 +79,8 @@ static int cryptd_init_queue(struct cryptd_queue *queue,
 	for_each_possible_cpu(cpu) {
 		cpu_queue = per_cpu_ptr(queue->cpu_queue, cpu);
 		crypto_init_queue(&cpu_queue->queue, max_cpu_qlen);
+		cpu_queue->encrypt_nums = 0;
+		cpu_queue->decrypt_nums = 0;
 		INIT_WORK(&cpu_queue->work, cryptd_queue_worker);
 	}
 	return 0;
@@ -101,6 +107,10 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
 	cpu = get_cpu();
 	cpu_queue = this_cpu_ptr(queue->cpu_queue);
 	err = crypto_enqueue_request(&cpu_queue->queue, request);
+	if ((err != -EBUSY) && (request->complete == cryptd_blkcipher_encrypt))
+		cpu_queue->encrypt_nums++;
+	if ((err != -EBUSY) && (request->complete == cryptd_blkcipher_decrypt))
+		cpu_queue->decrypt_nums++;
 	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
 	put_cpu();
 
@@ -171,10 +181,14 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 						struct scatterlist *src,
 						unsigned int len))
 {
-	struct cryptd_blkcipher_request_ctx *rctx;
+	struct cryptd_blkcipher_request_ctx *rctx = ablkcipher_request_ctx(req);
+	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct blkcipher_desc desc;
+	struct cryptd_queue *queue;
+	struct cryptd_cpu_queue *cpu_queue;
+	crypto_completion_t complete = req->base.complete;
 
-	rctx = ablkcipher_request_ctx(req);
+	queue = cryptd_get_queue(crypto_ablkcipher_tfm(tfm));
 
 	if (unlikely(err == -EINPROGRESS))
 		goto out;
@@ -190,6 +204,13 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 out:
 	local_bh_disable();
 	rctx->complete(&req->base, err);
+	preempt_disable();
+	cpu_queue = this_cpu_ptr(queue->cpu_queue);
+	if ((complete == cryptd_blkcipher_encrypt) && cpu_queue->encrypt_nums)
+		cpu_queue->encrypt_nums--;
+	if ((complete == cryptd_blkcipher_decrypt) && cpu_queue->decrypt_nums)
+		cpu_queue->decrypt_nums--;
+	preempt_enable();
 	local_bh_enable();
 }
 
@@ -729,6 +750,36 @@ void cryptd_free_ahash(struct cryptd_ahash *tfm)
 }
 EXPORT_SYMBOL_GPL(cryptd_free_ahash);
 
+unsigned int cryptd_get_encrypt_nums(struct crypto_tfm *tfm)
+{
+	struct cryptd_queue *queue = cryptd_get_queue(tfm);
+	struct cryptd_cpu_queue *cpu_queue;
+	unsigned int nums;
+
+	preempt_disable();
+	cpu_queue = this_cpu_ptr(queue->cpu_queue);
+	nums = cpu_queue->encrypt_nums;
+	preempt_enable();
+
+	return nums;
+}
+EXPORT_SYMBOL_GPL(cryptd_get_encrypt_nums);
+
+unsigned int cryptd_get_decrypt_nums(struct crypto_tfm *tfm)
+{
+	struct cryptd_queue *queue = cryptd_get_queue(tfm);
+	struct cryptd_cpu_queue *cpu_queue;
+	unsigned int nums;
+
+	preempt_disable();
+	cpu_queue = this_cpu_ptr(queue->cpu_queue);
+	nums = cpu_queue->decrypt_nums;
+	preempt_enable();
+
+	return nums;
+}
+EXPORT_SYMBOL_GPL(cryptd_get_decrypt_nums);
+
 static int __init cryptd_init(void)
 {
 	int err;
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index 1c96b25..cacc717 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -41,5 +41,6 @@ struct cryptd_ahash *cryptd_alloc_ahash(const char *alg_name,
 struct crypto_shash *cryptd_ahash_child(struct cryptd_ahash *tfm);
 struct shash_desc *cryptd_shash_desc(struct ahash_request *req);
 void cryptd_free_ahash(struct cryptd_ahash *tfm);
-
+unsigned int cryptd_get_encrypt_nums(struct crypto_tfm *tfm);
+unsigned int cryptd_get_decrypt_nums(struct crypto_tfm *tfm);
 #endif
-- 
1.8.5.2.233.g932f7e4

Re: HELP: IPsec reordering issue

[Herbert Xu]

On Sun, Aug 03, 2014 at 05:57:06PM +0800, Ming Liu wrote:
> 
> Please review this attached patch instead, the original one has a
> problem causing the kernel crash.

Thanks for the patch.  I think it would better to enforce ordering
for all operations rather than treat encryptions separately from
decryptions.  We could conceivably have more complex operations made
up from both encryptions and decryptions that could then get
out-of-order.

It would also make the code simpler.

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

Re: HELP: IPsec reordering issue

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

Hi, Herbert:

I've figured out a new patch for this issue reported by me previously, 
the basic idea is adding a cryptd_flush_queue function fixing it by 
being called from softirq to flush all previous queued elements before 
processing a new one, and it works very well so far per my test,  would 
you please review it?

the best,
thank you

On 08/14/2014 04:47 PM, Herbert Xu wrote:
> On Sun, Aug 03, 2014 at 05:57:06PM +0800, Ming Liu wrote:
>> Please review this attached patch instead, the original one has a
>> problem causing the kernel crash.
> Thanks for the patch.  I think it would better to enforce ordering
> for all operations rather than treat encryptions separately from
> decryptions.  We could conceivably have more complex operations made
> up from both encryptions and decryptions that could then get
> out-of-order.
>
> It would also make the code simpler.
>
> Cheers,


[-- Attachment #2: 0001-crypto-aesni-intel-avoid-IPsec-re-ordering.patch --]
[-- Type: text/x-patch, Size: 9528 bytes --]

>From 5e00cd925755015d4057ded1b24fd994e507a21e Mon Sep 17 00:00:00 2001
From: Ming Liu <ming.liu@windriver.com>
Date: Wed, 12 Nov 2014 11:11:57 +0800
Subject: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

So far, the encryption/decryption are asynchronously processed in
softirq and cryptd which would result in a implicit order of data,
therefore leads IPSec stack also out of order while encapsulating
or decapsulating packets.

Consider the following scenario:

             DECRYPTION INBOUND
                      |
                      |
             +-----Packet A
             |        |
             |        |
             |     Packet B
             |        |
    (cryptd) |        | (software interrupts)
             |      ......
             |        |
             |        |
             |     Packet B(decrypted)
             |        |
             |        |
             +---> Packet A(decrypted)
                      |
                      |
             DECRYPTION OUTBOUND

Add cryptd_flush_queue function fixing it by being called from softirq
to flush all previous queued elements before processing a new one. To
prevent cryptd_flush_queue() being accessed from software interrupts,
local_bh_disable/enable needs to be relocated in several places.

Signed-off-by: Ming Liu <ming.liu@windriver.com>
---
 crypto/ablk_helper.c    |  10 ++++-
 crypto/cryptd.c         | 107 ++++++++++++++++++++++++++++++++++++++++--------
 include/crypto/cryptd.h |  13 ++++++
 3 files changed, 111 insertions(+), 19 deletions(-)

diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
index ffe7278..600a70f 100644
--- a/crypto/ablk_helper.c
+++ b/crypto/ablk_helper.c
@@ -70,16 +70,19 @@ int ablk_encrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
 	if (!may_use_simd()) {
 		struct ablkcipher_request *cryptd_req =
 			ablkcipher_request_ctx(req);
 
 		*cryptd_req = *req;
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+		cryptd_req->base.tfm = req_tfm;
 
 		return crypto_ablkcipher_encrypt(cryptd_req);
 	} else {
+		cryptd_flush_queue(req_tfm, CRYPTD_ENCRYPT);
 		return __ablk_encrypt(req);
 	}
 }
@@ -89,13 +92,15 @@ int ablk_decrypt(struct ablkcipher_request *req)
 {
 	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
 	struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+	struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
+		crypto_ablkcipher_crt(&ctx->cryptd_tfm->base)->base);
 
 	if (!may_use_simd()) {
 		struct ablkcipher_request *cryptd_req =
 			ablkcipher_request_ctx(req);
 
 		*cryptd_req = *req;
-		ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+		cryptd_req->base.tfm = req_tfm;
 
 		return crypto_ablkcipher_decrypt(cryptd_req);
 	} else {
@@ -105,6 +110,7 @@ int ablk_decrypt(struct ablkcipher_request *req)
 		desc.info = req->info;
 		desc.flags = 0;
 
+		cryptd_flush_queue(req_tfm, CRYPTD_DECRYPT);
 		return crypto_blkcipher_crt(desc.tfm)->decrypt(
 			&desc, req->dst, req->src, req->nbytes);
 	}
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index e592c90..0b387a1 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -119,11 +119,13 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
 	int cpu, err;
 	struct cryptd_cpu_queue *cpu_queue;
 
+	local_bh_disable();
 	cpu = get_cpu();
 	cpu_queue = this_cpu_ptr(queue->cpu_queue);
 	err = crypto_enqueue_request(&cpu_queue->queue, request);
 	queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
 	put_cpu();
+	local_bh_enable();
 
 	return err;
 }
@@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
 	preempt_disable();
 	backlog = crypto_get_backlog(&cpu_queue->queue);
 	req = crypto_dequeue_request(&cpu_queue->queue);
-	preempt_enable();
-	local_bh_enable();
 
 	if (!req)
-		return;
+		goto out;
 
 	if (backlog)
 		backlog->complete(backlog, -EINPROGRESS);
@@ -159,6 +159,9 @@ static void cryptd_queue_worker(struct work_struct *work)
 
 	if (cpu_queue->queue.qlen)
 		queue_work(kcrypto_wq, &cpu_queue->work);
+out:
+	preempt_enable();
+	local_bh_enable();
 }
 
 static inline struct cryptd_queue *cryptd_get_queue(struct crypto_tfm *tfm)
@@ -209,9 +212,7 @@ static void cryptd_blkcipher_crypt(struct ablkcipher_request *req,
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static void cryptd_blkcipher_encrypt(struct crypto_async_request *req, int err)
@@ -446,9 +447,7 @@ static void cryptd_hash_init(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_init_enqueue(struct ahash_request *req)
@@ -471,9 +470,7 @@ static void cryptd_hash_update(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_update_enqueue(struct ahash_request *req)
@@ -494,9 +491,7 @@ static void cryptd_hash_final(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_final_enqueue(struct ahash_request *req)
@@ -517,9 +512,7 @@ static void cryptd_hash_finup(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_finup_enqueue(struct ahash_request *req)
@@ -546,9 +539,7 @@ static void cryptd_hash_digest(struct crypto_async_request *req_async, int err)
 	req->base.complete = rctx->complete;
 
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static int cryptd_hash_digest_enqueue(struct ahash_request *req)
@@ -641,9 +632,7 @@ static void cryptd_aead_crypt(struct aead_request *req,
 	err = crypt( req );
 	req->base.complete = rctx->complete;
 out:
-	local_bh_disable();
 	rctx->complete(&req->base, err);
-	local_bh_enable();
 }
 
 static void cryptd_aead_encrypt(struct crypto_async_request *areq, int err)
@@ -895,6 +884,90 @@ void cryptd_free_ahash(struct cryptd_ahash *tfm)
 }
 EXPORT_SYMBOL_GPL(cryptd_free_ahash);
 
+void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type)
+{
+	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
+	struct cryptd_instance_ctx *ictx = crypto_instance_ctx(inst);
+	struct cryptd_queue *cryptd_queue = ictx->queue;
+	struct cryptd_cpu_queue *cpu_queue;
+	struct crypto_queue *queue;
+	struct crypto_async_request *req, *tmp, *backlog;
+	crypto_completion_t complete;
+	int cpu;
+	unsigned int len;
+
+	switch (type) {
+	case CRYPTD_BLKCIPHER_ENCRYPT:
+		complete = cryptd_blkcipher_encrypt;
+		break;
+	case CRYPTD_BLKCIPHER_DECRYPT:
+		complete = cryptd_blkcipher_decrypt;
+		break;
+	case CRYPTD_HASH_INIT:
+		complete = cryptd_hash_init;
+		break;
+	case CRYPTD_HASH_UPDATE:
+		complete = cryptd_hash_update;
+		break;
+	case CRYPTD_HASH_FINAL:
+		complete = cryptd_hash_final;
+		break;
+	case CRYPTD_HASH_FINUP:
+		complete = cryptd_hash_finup;
+		break;
+	case CRYPTD_HASH_DIGEST:
+		complete = cryptd_hash_digest;
+		break;
+	case CRYPTD_AEAD_ENCRYPT:
+		complete = cryptd_aead_encrypt;
+		break;
+	case CRYPTD_AEAD_DECRYPT:
+		complete = cryptd_aead_decrypt;
+		break;
+	default:
+		complete = NULL;
+	}
+
+	if (complete == NULL)
+		return;
+
+	local_bh_disable();
+	cpu = get_cpu();
+	cpu_queue = this_cpu_ptr(cryptd_queue->cpu_queue);
+	queue = &cpu_queue->queue;
+	len = queue->qlen;
+
+	if (!len)
+		goto out;
+
+	list_for_each_entry_safe(req, tmp, &queue->list, list) {
+		if (req->complete == complete) {
+			queue->qlen--;
+
+			if (queue->backlog != &queue->list) {
+				backlog = container_of(queue->backlog,
+					struct crypto_async_request, list);
+				queue->backlog = queue->backlog->next;
+			} else
+				backlog = NULL;
+
+			list_del(&req->list);
+
+			if (backlog)
+				backlog->complete(backlog, -EINPROGRESS);
+			req->complete(req, 0);
+		}
+
+		if (--len == 0)
+			goto out;
+	}
+
+out:
+	put_cpu();
+	local_bh_enable();
+}
+EXPORT_SYMBOL_GPL(cryptd_flush_queue);
+
 struct cryptd_aead *cryptd_alloc_aead(const char *alg_name,
 						  u32 type, u32 mask)
 {
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index ba98918..a63a296 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -16,6 +16,18 @@
 #include <linux/kernel.h>
 #include <crypto/hash.h>
 
+typedef enum {
+	CRYPTD_BLKCIPHER_ENCRYPT,
+	CRYPTD_BLKCIPHER_DECRYPT,
+	CRYPTD_HASH_INIT,
+	CRYPTD_HASH_UPDATE,
+	CRYPTD_HASH_FINAL,
+	CRYPTD_HASH_FINUP,
+	CRYPTD_HASH_DIGEST,
+	CRYPTD_AEAD_ENCRYPT,
+	CRYPTD_AEAD_DECRYPT,
+} cryptd_type_t;
+
 struct cryptd_ablkcipher {
 	struct crypto_ablkcipher base;
 };
@@ -48,6 +60,7 @@ struct cryptd_ahash *cryptd_alloc_ahash(const char *alg_name,
 struct crypto_shash *cryptd_ahash_child(struct cryptd_ahash *tfm);
 struct shash_desc *cryptd_shash_desc(struct ahash_request *req);
 void cryptd_free_ahash(struct cryptd_ahash *tfm);
+void cryptd_flush_queue(struct crypto_tfm *tfm, cryptd_type_t type);
 
 struct cryptd_aead {
 	struct crypto_aead base;
-- 
1.8.4.1