dm-crypt: Fix deadlock when algo returns -EBUSY

I suspect this doesn't show up for most anyone because software algorithms typically don't have a sense of being too busy. However, when working with the Freescale CAAM driver, it would, under heavy load, return -EBUSY on occasion, which would leave dm-crypt dead locked and quite unhappy.

After checking the logic in some other drivers, the scheme for crypt_convert() and it's callback, kcryptd_async_done() were not correctly laid out to handle -EBUSY properly (nor -EINPROGRESS for that matter).

This patch corrects that and makes things pretty happy. Where-as I could cause a lockup withing 1-2 minutes, I've not been able to run the reproduction test with CAAM drivers fully installed for over 24 hours now. I've regression tested it against software algorithms on PPC32 and x86_64, and things seem perfectly happy there as well.

I've tested this all the way back to 3.13, and a few iterations in-between. Please consider pushing this to various stable trees.

Cc [email protected]

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

Signed-off-by Ben Collins <[email protected]>

---
 drivers/md/dm-crypt.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/md/dm-crypt.c +5 -6

index 713a962..3891f26 100644
@@ -925,11 +925,10 @@ static int crypt_convert(struct crypt_config *cc,
 
 		switch (r) {
 		/* async */
+		case -EINPROGRESS:
 		case -EBUSY:
 			wait_for_completion(&ctx->restart);
 			reinit_completion(&ctx->restart);
-			/* fall through*/
-		case -EINPROGRESS:
 			ctx->req = NULL;
 			ctx->cc_sector++;
 			continue;

@@ -1346,10 +1345,8 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 	struct dm_crypt_io *io = container_of(ctx, struct dm_crypt_io, ctx);
 	struct crypt_config *cc = io->cc;
 
-	if (error == -EINPROGRESS) {
-		complete(&ctx->restart);
+	if (error == -EINPROGRESS)
 		return;
-	}
 
 	if (!error && cc->iv_gen_ops && cc->iv_gen_ops->post)
 		error = cc->iv_gen_ops->post(cc, iv_of_dmreq(cc, dmreq), dmreq);

@@ -1359,13 +1356,17 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 
 	crypt_free_req(cc, req_of_dmreq(cc, dmreq), io->base_bio);
 
-	if (!atomic_dec_and_test(&ctx->cc_pending))
+	if (!atomic_dec_and_test(&ctx->cc_pending)) {
+		complete(&ctx->restart);
 		return;
+	}

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Quoted 3 lines · Mike Snitzer

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

Quoted 3 lines

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Actually, how about using !completion_done() before calling complete()? If you think this would be OK, any chance you could re-test with this?

From: Ben Collins <[email protected]>
Date: Fri, 3 Apr 2015 16:09:46 +0000
Subject: [PATCH] dm crypt: fix deadlock when algo returns -EBUSY

I suspect this doesn't show up for most anyone because software algorithms typically don't have a sense of being too busy. However, when working with the Freescale CAAM driver it will return -EBUSY on occasion under heavy -- which resulted in dm-crypt deadlock.

After checking the logic in some other drivers, the scheme for crypt_convert() and it's callback, kcryptd_async_done(), were not correctly laid out to properly handle -EBUSY or -EINPROGRESS.

Fix this by using the completion for both -EBUSY and -EINPROGRESS. Before this fix dm-crypt would lockup within 1-2 minutes running with the CAAM driver. Fix was regression tested against software algorithms on PPC32 and x86_64, and things seem perfectly happy there as well.

Signed-off-by Ben Collins <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

Cc [email protected]

---
drivers/md/dm-crypt.c | 12 ++++++------

1 file changed, 6 insertions(+), 6 deletions(-)

drivers/md/dm-crypt.c +6 -7

index ea09d54..cab96ca 100644
@@ -925,11 +925,10 @@ static int crypt_convert(struct crypt_config *cc,
 
 		switch (r) {
 		/* async */
+		case -EINPROGRESS:
 		case -EBUSY:
 			wait_for_completion(&ctx->restart);
 			reinit_completion(&ctx->restart);
-			/* fall through*/
-		case -EINPROGRESS:
 			ctx->req = NULL;
 			ctx->cc_sector++;
 			continue;

@@ -1346,10 +1345,8 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 	struct dm_crypt_io *io = container_of(ctx, struct dm_crypt_io, ctx);
 	struct crypt_config *cc = io->cc;
 
-	if (error == -EINPROGRESS) {
-		complete(&ctx->restart);
+	if (error == -EINPROGRESS)
 		return;
-	}
 
 	if (!error && cc->iv_gen_ops && cc->iv_gen_ops->post)
 		error = cc->iv_gen_ops->post(cc, iv_of_dmreq(cc, dmreq), dmreq);

@@ -1360,12 +1357,15 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 	crypt_free_req(cc, req_of_dmreq(cc, dmreq), io->base_bio);
 
 	if (!atomic_dec_and_test(&ctx->cc_pending))
-		return;
+		goto done;
 
 	if (bio_data_dir(io->base_bio) == READ)
 		kcryptd_crypt_read_done(io);
 	else
 		kcryptd_crypt_write_io_submit(io, 1);
+done:
+	if (!completion_done(&ctx->restart))
+		complete(&ctx->restart);
 }
 
 static void kcryptd_crypt(struct work_struct *work)
--

1.9.5 (Apple Git-50.3)

Quoted 9 lines · Mike Snitzer

Quoted 3 lines · Mike Snitzer

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

Quoted 6 lines

Quoted 3 lines

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Actually, how about using !completion_done() before calling complete()? If you think this would be OK, any chance you could re-test with this?

I'll be able to test it before Friday (out of town). Thanks

Quoted 16 lines · Ben Collins

Quoted 15 lines · Mike Snitzer

Quoted 6 lines · Mike Snitzer

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Actually, how about using !completion_done() before calling complete()? If you think this would be OK, any chance you could re-test with this?

Quoted 1 line

I'll be able to test it before Friday (out of town). Thanks

Hi Ben,

I'm still waiting for test feedback from you on v2. Fairly certain you'll have the same results but I'd like to be certain before pushing this upstream.

Quoted 16 lines · Ben Collins

Quoted 15 lines · Mike Snitzer

Quoted 6 lines · Mike Snitzer

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Actually, how about using !completion_done() before calling complete()? If you think this would be OK, any chance you could re-test with this?

Quoted 1 line

I'll be able to test it before Friday (out of town). Thanks

Hi Ben,

I'm still waiting for test feedback from you on v2. Fairly certain you'll have the same results but I'd like to be certain before pushing this upstream.

Quoted 24 lines · Mike Snitzer

Quoted 17 lines · Ben Collins

Quoted 9 lines · Mike Snitzer

Quoted 3 lines · Mike Snitzer

It looks like you're _always_ using the completion regardless of whether crypt_convert() will be waiting (e.g. even if error is 0).

Quoted 6 lines

Quoted 3 lines

I can see this "working" but it seems less than ideal. Would it be better to record the need to use the completion in ctx and then conditionally call complete()?

Actually, how about using !completion_done() before calling complete()? If you think this would be OK, any chance you could re-test with this?

I'll be able to test it before Friday (out of town). Thanks

Quoted 5 lines

Hi Ben,

I'm still waiting for test feedback from you on v2. Fairly certain you'll have the same results but I'd like to be certain before pushing this upstream.

As expected, this patch works as well as what I had originally done. Thanks for reviewing it.