Bug of dm-crypt?

Bug of dm-crypt?

[Huang Ying]

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

Hi, Herbert,

I had ever heard from you that the only thing guaranteed in the
completion function of async ablkcipher cryption is the req->data has
the value you set before. The request pointer itself may be changed. But
in dm-crypt, I found they rely on request pointer in completion
function: kcryptd_async_done. This makes my AES-NI cryptd usage panic.

Do you think that is a bug?

Best Regards,
Huang Ying


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

Re: Bug of dm-crypt?

[Herbert Xu]

On Fri, Feb 27, 2009 at 01:31:56PM +0800, Huang Ying wrote:
> Hi, Herbert,
> 
> I had ever heard from you that the only thing guaranteed in the
> completion function of async ablkcipher cryption is the req->data has
> the value you set before. The request pointer itself may be changed. But
> in dm-crypt, I found they rely on request pointer in completion
> function: kcryptd_async_done. This makes my AES-NI cryptd usage panic.
> 
> Do you think that is a bug?

Absolutely.

It should use cc->req instead.

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

Re: Bug of dm-crypt?

[Milan Broz]

Herbert Xu wrote:
> On Fri, Feb 27, 2009 at 01:31:56PM +0800, Huang Ying wrote:
>> I had ever heard from you that the only thing guaranteed in the
>> completion function of async ablkcipher cryption is the req->data has
>> the value you set before. The request pointer itself may be changed. But
>> in dm-crypt, I found they rely on request pointer in completion
>> function: kcryptd_async_done. This makes my AES-NI cryptd usage panic.
>>
>> Do you think that is a bug?
> 
> Absolutely.
> 
> It should use cc->req instead.

ok, I'll check it and try to fix that. 

Can you send me backtrace from that panic?

What's wrong in async callback now - mempool_free using async_req directly?

(Btw in some previous discussuion I asked if cryptd can be used
for some kind of parallel speedup for dm-crypt in async mode and response
was that cryptd "is only meant to be a demo showing how an async
implementation should be written".

So this changed to real working thread for AES-NI?)

Milan
--
mbroz@redhat.com

Re: Bug of dm-crypt?

[Huang Ying]

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

Hi, Milan,

On Fri, 2009-02-27 at 16:41 +0800, Milan Broz wrote:
> Herbert Xu wrote:
> > On Fri, Feb 27, 2009 at 01:31:56PM +0800, Huang Ying wrote:
> >> I had ever heard from you that the only thing guaranteed in the
> >> completion function of async ablkcipher cryption is the req->data has
> >> the value you set before. The request pointer itself may be changed. But
> >> in dm-crypt, I found they rely on request pointer in completion
> >> function: kcryptd_async_done. This makes my AES-NI cryptd usage panic.
> >>
> >> Do you think that is a bug?
> > 
> > Absolutely.
> > 
> > It should use cc->req instead.
> 
> ok, I'll check it and try to fix that. 
> 
> Can you send me backtrace from that panic?

The panic backtrace is just some random kernel page fault or general
protection fault, because the freed pointer is not the allocated one.

> What's wrong in async callback now - mempool_free using async_req directly?

Yes. I have a fix by storing real allocated async_req in
async_req->data. Can you help me to review that?

> (Btw in some previous discussuion I asked if cryptd can be used
> for some kind of parallel speedup for dm-crypt in async mode and response
> was that cryptd "is only meant to be a demo showing how an async
> implementation should be written".
> 
> So this changed to real working thread for AES-NI?)

Yes. Now AES-NI uses it. And because a Per-CPU thread implementation is
used for cryptd, it can be used to speedup dm-crypt in some situation
too I think.

Best Regards,
Huang Ying


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

Re: Bug of dm-crypt?

[Herbert Xu]

On Fri, Feb 27, 2009 at 09:41:02AM +0100, Milan Broz wrote:
> 
> Can you send me backtrace from that panic?
> 
> What's wrong in async callback now - mempool_free using async_req directly?

The req you get in the completion function is not necessarily
the req that you started with.  The only thing you can rely on
is req->data.

> (Btw in some previous discussuion I asked if cryptd can be used
> for some kind of parallel speedup for dm-crypt in async mode and response
> was that cryptd "is only meant to be a demo showing how an async
> implementation should be written".
> 
> So this changed to real working thread for AES-NI?)

Yes we changed it :) cryptd is no longer for fun only.

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