[PATCH] dm-crypt: Fix deadlock when algo returns -EBUSY

[PATCH] dm-crypt: Fix deadlock when algo returns -EBUSY

[Ben Collins]

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: dm-devel@redhat.com
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Ben Collins <ben.c@servergy.com>
---
 drivers/md/dm-crypt.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 713a962..3891f26 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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;
+	}
 
 	if (bio_data_dir(io->base_bio) == READ)
 		kcryptd_crypt_read_done(io);
 	else
 		kcryptd_crypt_write_io_submit(io, 1);
+
+	complete(&ctx->restart);
 }
 
 static void kcryptd_crypt(struct work_struct *work)
-- 
1.9.1

Re: dm-crypt: Fix deadlock when algo returns -EBUSY

[Mike Snitzer]

On Fri, Apr 03 2015 at 12:09pm -0400,
Ben Collins <ben.c@servergy.com> wrote:

> 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: dm-devel@redhat.com
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Ben Collins <ben.c@servergy.com>
> ---
>  drivers/md/dm-crypt.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 713a962..3891f26 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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;
> +	}
>  
>  	if (bio_data_dir(io->base_bio) == READ)
>  		kcryptd_crypt_read_done(io);
>  	else
>  		kcryptd_crypt_write_io_submit(io, 1);
> +
> +	complete(&ctx->restart);
>  }
>  
>  static void kcryptd_crypt(struct work_struct *work)

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()?

Mike

[PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Mike Snitzer]

On Tue, Apr 07 2015 at 11:55P -0400,
Mike Snitzer <snitzer@redhat.com> wrote:
 
> 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?

From: Ben Collins <ben.c@servergy.com>
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 <ben.c@servergy.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/md/dm-crypt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index ea09d54..cab96ca 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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)

Re: [PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Ben Collins]

[-- Attachment #1.1: Type: text/plain, Size: 3573 bytes --]


> On Apr 7, 2015, at 11:28 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> 
> On Tue, Apr 07 2015 at 11:55P -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> 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?

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



> From: Ben Collins <ben.c@servergy.com>
> 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 <ben.c@servergy.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/md/dm-crypt.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index ea09d54..cab96ca 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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)
> 

——
Ben Collins
Cyphre Champion
————————————————
Principal Architect
Servergy, Inc.
469-919-5634 (O)
757-243-7557 (M)


[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Mike Snitzer]

On Tue, Apr 07 2015 at  1:10pm -0400,
Ben Collins <ben.c@servergy.com> wrote:

> 
> > On Apr 7, 2015, at 11:28 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > On Tue, Apr 07 2015 at 11:55P -0400,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> >> 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?
> 
> 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.

Thanks,
Mike

Re: [PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Ben Collins]

[-- Attachment #1.1: Type: text/plain, Size: 1266 bytes --]


> On Apr 10, 2015, at 11:11 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> 
> On Tue, Apr 07 2015 at  1:10pm -0400,
> Ben Collins <ben.c@servergy.com> wrote:
> 
>> 
>>> On Apr 7, 2015, at 11:28 AM, Mike Snitzer <snitzer@redhat.com> wrote:
>>> 
>>> On Tue, Apr 07 2015 at 11:55P -0400,
>>> Mike Snitzer <snitzer@redhat.com> wrote:
>>> 
>>>> 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?
>> 
>> 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.

I'll have a response within a couple hours...

——
Ben Collins
Cyphre Champion
————————————————
Principal Architect
Servergy, Inc.

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Ben Collins]

[-- Attachment #1.1: Type: text/plain, Size: 1333 bytes --]


> On Apr 10, 2015, at 11:11 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> 
> On Tue, Apr 07 2015 at  1:10pm -0400,
> Ben Collins <ben.c@servergy.com> wrote:
> 
>> 
>>> On Apr 7, 2015, at 11:28 AM, Mike Snitzer <snitzer@redhat.com> wrote:
>>> 
>>> On Tue, Apr 07 2015 at 11:55P -0400,
>>> Mike Snitzer <snitzer@redhat.com> wrote:
>>> 
>>>> 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?
>> 
>> 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.

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

——
Ben Collins
Cyphre Champion
————————————————
Principal Architect
Servergy, Inc.
469-919-5634 (O)

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] dm crypt: fix deadlock when algo returns -EBUSY

[Mike Snitzer]

On Fri, Apr 10 2015 at  1:56pm -0400,
Ben Collins <ben.c@servergy.com> wrote:

> 
> > On Apr 10, 2015, at 11:11 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> > 
> > On Tue, Apr 07 2015 at  1:10pm -0400,
> > Ben Collins <ben.c@servergy.com> wrote:
> > 
> >> 
> >>> On Apr 7, 2015, at 11:28 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> >>> 
> >>> On Tue, Apr 07 2015 at 11:55P -0400,
> >>> Mike Snitzer <snitzer@redhat.com> wrote:
> >>> 
> >>>> 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?
> >> 
> >> 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.
> 
> As expected, this patch works as well as what I had originally done.
> Thanks for reviewing it.

OK, thanks.  I've staged the change in linux-next, with a slightly
revised header, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=223c1aa8360a386