[BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Huang Ying]

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

In async cryption complete function (kcryptd_async_done), the
crypto_async_request passed in may be different from the one passed to
crypto_ablkcipher_encrypt/decrypt. Only crypto_async_request->data is
guaranteed to be same as the passed in one. Current kcryptd_async_done
uses passed in crypto_async_request directly, which may cause AES-NI
based AES algorithm implementation panic.

This patch fix this bug by using crypto_async_request->data only,
which point to dm_crypt_request, the crypto_async_request passed in
and original data (convert_context) can be gotten from
dm_crypt_request.

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 drivers/md/dm-crypt.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -60,6 +60,8 @@ struct dm_crypt_io {
 };
 
 struct dm_crypt_request {
+	struct ablkcipher_request *req;
+	struct convert_context *ctx;
 	struct scatterlist sg_in;
 	struct scatterlist sg_out;
 };
@@ -349,6 +351,8 @@ static int crypt_convert_block(struct cr
 	iv = (u8 *)ALIGN((unsigned long)(dmreq + 1),
 			 crypto_ablkcipher_alignmask(cc->tfm) + 1);
 
+	dmreq->req = req;
+	dmreq->ctx = ctx;
 	sg_init_table(&dmreq->sg_in, 1);
 	sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
 		    bv_in->bv_offset + ctx->offset_in);
@@ -391,12 +395,15 @@ static void kcryptd_async_done(struct cr
 static void crypt_alloc_req(struct crypt_config *cc,
 			    struct convert_context *ctx)
 {
+	struct dm_crypt_request *dmreq;
+
 	if (!cc->req)
 		cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
 	ablkcipher_request_set_tfm(cc->req, cc->tfm);
+	dmreq = (struct dm_crypt_request *)((char *)cc->req + cc->dmreq_start);
 	ablkcipher_request_set_callback(cc->req, CRYPTO_TFM_REQ_MAY_BACKLOG |
 					     CRYPTO_TFM_REQ_MAY_SLEEP,
-					     kcryptd_async_done, ctx);
+					     kcryptd_async_done, dmreq);
 }
 
 /*
@@ -821,7 +828,8 @@ static void kcryptd_crypt_read_convert(s
 static void kcryptd_async_done(struct crypto_async_request *async_req,
 			       int error)
 {
-	struct convert_context *ctx = async_req->data;
+	struct dm_crypt_request *dmreq = async_req->data;
+	struct convert_context *ctx = dmreq->ctx;
 	struct dm_crypt_io *io = container_of(ctx, struct dm_crypt_io, ctx);
 	struct crypt_config *cc = io->target->private;
 
@@ -830,7 +838,7 @@ static void kcryptd_async_done(struct cr
 		return;
 	}
 
-	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
+	mempool_free(dmreq->req, cc->req_pool);
 
 	if (!atomic_dec_and_test(&ctx->pending))
 		return;


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Herbert Xu]

On Fri, Feb 27, 2009 at 04:56:11PM +0800, Huang Ying wrote:
>
> @@ -830,7 +838,7 @@ static void kcryptd_async_done(struct cr
>  		return;
>  	}
>  
> -	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
> +	mempool_free(dmreq->req, cc->req_pool);

Why do we need all this complexity? Can't just fix it by using
cc->req?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Milan Broz]

Herbert Xu wrote:
> On Fri, Feb 27, 2009 at 04:56:11PM +0800, Huang Ying wrote:
>> @@ -830,7 +838,7 @@ static void kcryptd_async_done(struct cr
>>  		return;
>>  	}
>>  
>> -	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
>> +	mempool_free(dmreq->req, cc->req_pool);
> 
> Why do we need all this complexity? Can't just fix it by using
> cc->req?

No. There can be parallel req allocated, also cc->req can be NULL.
(seems that these structs are overcomplicated already:-)

(And becuse sometimes the bio request is split into 2 pieces because
of hw restrictions, there can be two ablkcipher_requests for one bio...)

I think that patch is the best what we can do now as bugfix.
(I am just running some tests with that.)

Milan
--
mbroz@redhat.com

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Herbert Xu]

On Fri, Feb 27, 2009 at 12:52:05PM +0100, Milan Broz wrote:
> Herbert Xu wrote:
> > On Fri, Feb 27, 2009 at 04:56:11PM +0800, Huang Ying wrote:
> >> @@ -830,7 +838,7 @@ static void kcryptd_async_done(struct cr
> >>  		return;
> >>  	}
> >>  
> >> -	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
> >> +	mempool_free(dmreq->req, cc->req_pool);
> > 
> > Why do we need all this complexity? Can't just fix it by using
> > cc->req?
> 
> No. There can be parallel req allocated, also cc->req can be NULL.
> (seems that these structs are overcomplicated already:-)

Fair enough.  However we still shouldn't need to have dmreq->req
since

	dmreq->req == (char *)dmreq - sizeof(dmreq->req)

In fact just pass the request itself as data and derive dmreq
from that.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Milan Broz]

Herbert Xu wrote:
> On Fri, Feb 27, 2009 at 12:52:05PM +0100, Milan Broz wrote:
>> Herbert Xu wrote:
>>> On Fri, Feb 27, 2009 at 04:56:11PM +0800, Huang Ying wrote:
>>>> @@ -830,7 +838,7 @@ static void kcryptd_async_done(struct cr
>>>>  		return;
>>>>  	}
>>>>  
>>>> -	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
>>>> +	mempool_free(dmreq->req, cc->req_pool);
>>> Why do we need all this complexity? Can't just fix it by using
>>> cc->req?
>> No. There can be parallel req allocated, also cc->req can be NULL.
>> (seems that these structs are overcomplicated already:-)
> 
> Fair enough.  However we still shouldn't need to have dmreq->req
> since
> 
> 	dmreq->req == (char *)dmreq - sizeof(dmreq->req)
> 
> In fact just pass the request itself as data and derive dmreq
> from that.

Like this?

struct ablkcipher_request *req = (char *)dmreq - cc->dmreq_start;
mempool_free(req, cc->req_pool);

Yes, this should be enough. Just some nice inline function will be
better for such  pointer game...

So we need add just dmreq->ctx field now.

Milan
--
mbroz@redhat.com

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Herbert Xu]

On Fri, Feb 27, 2009 at 01:28:46PM +0100, Milan Broz wrote:
>
> Like this?
> 
> struct ablkcipher_request *req = (char *)dmreq - cc->dmreq_start;
> mempool_free(req, cc->req_pool);

Exactly.  You could also embed the ablkcipher_request at the
end of dmreq, as in

struct dm_crypt_request {
	struct scatterlist sg_in;
	struct scatterlist sg_out;
	struct ablkcipher_request req;
};

Then you can use container_of.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Milan Broz]

Herbert Xu wrote:
> On Fri, Feb 27, 2009 at 01:28:46PM +0100, Milan Broz wrote:
>> Like this?
>>
>> struct ablkcipher_request *req = (char *)dmreq - cc->dmreq_start;
>> mempool_free(req, cc->req_pool);
>
> Exactly.  You could also embed the ablkcipher_request at the
> end of dmreq, as in
>
> struct dm_crypt_request {
> 	struct scatterlist sg_in;
> 	struct scatterlist sg_out;
> 	struct ablkcipher_request req;
> };
>
> Then you can use container_of.

Hm, I better keep explicitly this pointer retyping as reminder that
the structures need some revision in future...

Is the attached and reworked patch ok?

Alasdair, please can we queue this for 2.6.29-rc as urgent bugfix?

Milan
-- 
mbroz@redhat.com

----
dm-crypt: Fix async completion to not use crypto_async_request directly

In async cryption complete function (kcryptd_async_done), the
crypto_async_request passed in may be different from the one passed to
crypto_ablkcipher_encrypt/decrypt. Only crypto_async_request->data is
guaranteed to be same as the passed in one. Current kcryptd_async_done
uses passed in crypto_async_request directly, which may cause AES-NI
based AES algorithm implementation panic.

This patch fix this bug by using crypto_async_request->data only,
which point to dm_crypt_request, the crypto_async_request passed in
and original data (convert_context) can be gotten from
dm_crypt_request.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Milan Broz <mbroz@redhat.com>

---
 drivers/md/dm-crypt.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 35bda49..ebab49f 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -60,6 +60,7 @@ struct dm_crypt_io {
 };
 
 struct dm_crypt_request {
+	struct convert_context *ctx;
 	struct scatterlist sg_in;
 	struct scatterlist sg_out;
 };
@@ -335,6 +336,18 @@ static void crypt_convert_init(struct crypt_config *cc,
 	init_completion(&ctx->restart);
 }
 
+static struct dm_crypt_request *dmreq_of_req(struct crypt_config *cc,
+					     struct ablkcipher_request *req)
+{
+	return (struct dm_crypt_request *)((char *)req + cc->dmreq_start);
+}
+
+static struct ablkcipher_request *req_of_dmreq(struct crypt_config *cc,
+					       struct dm_crypt_request *dmreq)
+{
+	return (struct ablkcipher_request *)((char *)dmreq - cc->dmreq_start);
+}
+
 static int crypt_convert_block(struct crypt_config *cc,
 			       struct convert_context *ctx,
 			       struct ablkcipher_request *req)
@@ -345,10 +358,11 @@ static int crypt_convert_block(struct crypt_config *cc,
 	u8 *iv;
 	int r = 0;
 
-	dmreq = (struct dm_crypt_request *)((char *)req + cc->dmreq_start);
+	dmreq = dmreq_of_req(cc, req);
 	iv = (u8 *)ALIGN((unsigned long)(dmreq + 1),
 			 crypto_ablkcipher_alignmask(cc->tfm) + 1);
 
+	dmreq->ctx = ctx;
 	sg_init_table(&dmreq->sg_in, 1);
 	sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
 		    bv_in->bv_offset + ctx->offset_in);
@@ -395,8 +409,9 @@ static void crypt_alloc_req(struct crypt_config *cc,
 		cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
 	ablkcipher_request_set_tfm(cc->req, cc->tfm);
 	ablkcipher_request_set_callback(cc->req, CRYPTO_TFM_REQ_MAY_BACKLOG |
-					     CRYPTO_TFM_REQ_MAY_SLEEP,
-					     kcryptd_async_done, ctx);
+					CRYPTO_TFM_REQ_MAY_SLEEP,
+					kcryptd_async_done,
+					dmreq_of_req(cc, cc->req));
 }
 
 /*
@@ -821,7 +836,8 @@ static void kcryptd_crypt_read_convert(struct dm_crypt_io *io)
 static void kcryptd_async_done(struct crypto_async_request *async_req,
 			       int error)
 {
-	struct convert_context *ctx = async_req->data;
+	struct dm_crypt_request *dmreq = async_req->data;
+	struct convert_context *ctx = dmreq->ctx;
 	struct dm_crypt_io *io = container_of(ctx, struct dm_crypt_io, ctx);
 	struct crypt_config *cc = io->target->private;
 
@@ -830,7 +846,7 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 		return;
 	}
 
-	mempool_free(ablkcipher_request_cast(async_req), cc->req_pool);
+	mempool_free(req_of_dmreq(cc, dmreq), cc->req_pool);
 
 	if (!atomic_dec_and_test(&ctx->pending))
 		return;

Re: [BUGFIX] dm-crypt: Fix a bug of async cryption complete function

[Herbert Xu]

On Fri, Feb 27, 2009 at 02:51:15PM +0100, Milan Broz wrote:
>
> Is the attached and reworked patch ok?

Looks good to me.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt