IV copy strategy

IV copy strategy

[Sebastian Siewior]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 2424 bytes --]

Hello Herbert,

I just run in a bug which I caused. Actually I don't understand it at
all. The bad patch seems to be:

|75a8ae21dfd08f425b72906cc30b53103b2e5105 is first bad commit
| commit 75a8ae21dfd08f425b72906cc30b53103b2e5105
| Author: Sebastian Siewior <sebastian@breakpoint.cc>
| Date:   Sun Oct 21 16:04:23 2007 +0800
| 
|     [CRYPTO] geode: use consistent IV copy

and the bug report is:

|=============================================================================
|BUG kmalloc-64: Poison overwritten
|-----------------------------------------------------------------------------
|
|INFO: 0xc21dc3a0-0xc21dc3af. First byte 0xe3 instead of 0x6b
|INFO: Allocated in blkcipher_walk_first+0xe0/0x1a9 age=1 cpu=0 pid=2569
|INFO: Freed in blkcipher_walk_done+0x19d/0x1b7 age=0 cpu=0 pid=2569
|INFO: Slab 0xc1043b80 used=4 fp=0xc21dc380 flags=0x400000c3
|INFO: Object 0xc21dc380 @offset=896 fp=0xc21dc7e0
|
|Bytes b4 0xc21dc370:  18 09 00 00 39 73 00 00 5a 5a 5a 5a 5a 5a 5a 5a ....9s..ZZZZZZZZ
|  Object 0xc21dc380:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
|  Object 0xc21dc390:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
|  Object 0xc21dc3a0:  e3 53 77 9c 10 79 ae b8 27 08 94 2d be 77 18 1a ãSw..y®¸'..-¾w..
|  Object 0xc21dc3b0:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk¥
| Redzone 0xc21dc3c0:  bb bb bb bb                                     »»»»
| Padding 0xc21dc3e8:  5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ
| [<c0150040>] check_bytes_and_report+0x8d/0xae
| [<c015029f>] check_object+0xbf/0x1b5
| [<c01b8131>] blkcipher_walk_first+0xe0/0x1a9
| [<c0150e97>] __slab_alloc+0x33a/0x433
| [<c01b8131>] blkcipher_walk_first+0xe0/0x1a9
| [<c0151b1f>] __kmalloc+0x7d/0xe8
| [<c01b8131>] blkcipher_walk_first+0xe0/0x1a9
| [<c01b8131>] blkcipher_walk_first+0xe0/0x1a9
| [<c01b8131>] blkcipher_walk_first+0xe0/0x1a9
| [<c015031e>] check_object+0x13e/0x1b5
| [<d00134e0>] geode_cbc_encrypt+0x32/0xca [geode_aes]
| [<c01510fb>] kfree+0xc0/0xca
|

I removed the write back of the IV 
    memcpy(walk.iv, op->iv, AES_IV_LENGTH);

and everything goes back to normal. I checked walk.iv and it doesn't
change, it is still the same pointer. Do you free the walk.iv in the
meantime or is there another BUG I don't see? The IV length is 16 bytes.
Currently I'm lost ...

Sebastian

Re: IV copy strategy

[Herbert Xu]

On Wed, Nov 14, 2007 at 12:11:32AM +0100, Sebastian Siewior wrote:
> 
> and everything goes back to normal. I checked walk.iv and it doesn't
> change, it is still the same pointer. Do you free the walk.iv in the
> meantime or is there another BUG I don't see? The IV length is 16 bytes.
> Currently I'm lost ...

Indeed the last call to blkcipher_walk_done will free the IV if
we had to copy it due to an alignment mismatch.  Since geode has
an alignment of 16 bytes, that's almost a given.

You could copy from/to desc->info instead which would solve the
problem.

However, why does op->iv exist at all? Surely we can just use
walk->iv directly and the problem goes away completely?

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: IV copy strategy

[Sebastian Siewior]

* Herbert Xu | 2007-11-14 22:22:53 [+0800]:

>Indeed the last call to blkcipher_walk_done will free the IV if
>we had to copy it due to an alignment mismatch.  Since geode has
>an alignment of 16 bytes, that's almost a given.
Ach

>You could copy from/to desc->info instead which would solve the
>problem.
>
>However, why does op->iv exist at all? Surely we can just use
>walk->iv directly and the problem goes away completely?
Yes, you are absolutely right. I get rid of op->iv and copy the IV back
to walk->iv in the crypt function (where I have to do it anyway).

In this case, the s390 has the same bug (they copy the IV back after
blkcipher_walk_done()). Howevere it will probably never get triggered
because they have an aligment of 0 (what gets pushed to 3 by the crypto
API if I remenber correcrtly). 
So a general question: Is it a must (requirement by the crypto API) to
copy the IV back or not? I guess not if we move completely to async (one
day) :)

>
>Cheers,

Sebastian

Re: IV copy strategy

[Herbert Xu]

On Thu, Nov 15, 2007 at 10:10:05PM +0100, Sebastian Siewior wrote:
>
> In this case, the s390 has the same bug (they copy the IV back after
> blkcipher_walk_done()). Howevere it will probably never get triggered
> because they have an aligment of 0 (what gets pushed to 3 by the crypto
> API if I remenber correcrtly). 

It only gets pushed to 3 if you use the generic CBC template, they
don't so they will stay at 0.  In their case I also see why they
can't just use walk->iv directly.

> So a general question: Is it a must (requirement by the crypto API) to
> copy the IV back or not? I guess not if we move completely to async (one
> day) :)

You must copy it back to allow chaining.  Even when we go async
someone may wish to chain.  So in that sense you've just found a
bug in the hifn driver :)

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: IV copy strategy

[Sebastian Siewior]

* Herbert Xu | 2007-11-16 10:08:51 [+0800]:

>You must copy it back to allow chaining.  Even when we go async
>someone may wish to chain.  So in that sense you've just found a
>bug in the hifn driver :)
Not only in hfin. My SPU-AES has the same bug. Do you know someone who
wants to chain? I can remember that you said once "that this is
currently the case but we can change this since IPsec brings a new IV
for ever packet".
So, both of us have to fix it or must the crypto users complete their
encryption/decryption process in one go once they use async (we have no
documentation so we are very flexible here I guess :) )?

>Cheers,

Sebastian

Re: IV copy strategy

[Herbert Xu]

On Fri, Nov 16, 2007 at 09:19:13AM +0100, Sebastian Siewior wrote:
> Not only in hfin. My SPU-AES has the same bug. Do you know someone who
> wants to chain? I can remember that you said once "that this is
> currently the case but we can change this since IPsec brings a new IV
> for ever packet".
> So, both of us have to fix it or must the crypto users complete their
> encryption/decryption process in one go once they use async (we have no
> documentation so we are very flexible here I guess :) )?

IPsec wouldn't need to chain but it is conceivable that others
may wish to chain.  More importantly if you don't copy it out
then chaining would be impossible in general so you're taking
the choice away from the user.

I just did a grep and RXKAD seems to be the only user that uses
the IV (apart from IPsec that is).  So if we take chaining away
then we might lose the ability to ever convert RXKAD to the
ablkcipher interface.

I do recognise that for DMA devices copying the IV back may be
expensive so perhaps we can add a request flag for this.

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: IV copy strategy

[Evgeniy Polyakov]

On Fri, Nov 16, 2007 at 10:08:51AM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> > So a general question: Is it a must (requirement by the crypto API) to
> > copy the IV back or not? I guess not if we move completely to async (one
> > day) :)
> 
> You must copy it back to allow chaining.  Even when we go async
> someone may wish to chain.  So in that sense you've just found a
> bug in the hifn driver :)

That's a question - should it copy IV back or not?
Currently it is not required by crypto users.

P.S. Sebastian, your mailer seems not to setup reply-to, since each time
I reply, your mail is copied to the header :)

-- 
	Evgeniy Polyakov

Re: IV copy strategy

[Herbert Xu]

On Fri, Nov 16, 2007 at 02:11:10PM +0300, Evgeniy Polyakov wrote:
>
> That's a question - should it copy IV back or not?
> Currently it is not required by crypto users.

Well currently we have exactly one crypto user of ablkcipher
in the tree, and that's tcrypt :)

However, looking at the sync crypto users it would seem that
chaining while not popular is used by at least RXKAD.  So I'd
like to preserve this functionality.  Although in light of the
fact that on DMA devices touching the encrypted result to copy
the IV may be expensive, we could make it conditional on a flag
inside the request, i.e., something like

	CRYPTO_TFM_REQ_COPY_IV

But the point is that this is something that has to be done by
the algorithm since only it knows in general what/where the IV
is.  So if the algorithm doesn't do that then the user can't
easily work around this.

Actually on second thought why don't we change the interface
for ablkcipher so that we allow the IV to be returned by either
copying it to req->info or replacing the req->info pointer?

That way if the destination is linear and lowmem at the end we
can just return a pointer to it without touching the data at all.

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: IV copy strategy

[Evgeniy Polyakov]

On Fri, Nov 16, 2007 at 07:25:30PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> On Fri, Nov 16, 2007 at 02:11:10PM +0300, Evgeniy Polyakov wrote:
> >
> > That's a question - should it copy IV back or not?
> > Currently it is not required by crypto users.
> 
> Well currently we have exactly one crypto user of ablkcipher
> in the tree, and that's tcrypt :)

In acrypto days it was ipsec and dmcrypt too :)
I thought you converted dmcrypt to use ablkcipher already.

> However, looking at the sync crypto users it would seem that
> chaining while not popular is used by at least RXKAD.  So I'd
> like to preserve this functionality.  Although in light of the
> fact that on DMA devices touching the encrypted result to copy
> the IV may be expensive, we could make it conditional on a flag
> inside the request, i.e., something like
> 
> 	CRYPTO_TFM_REQ_COPY_IV

Well, it is doable to copy iv back after data has been processed just
before callback is invoked when this flag is set for tfm.

> But the point is that this is something that has to be done by
> the algorithm since only it knows in general what/where the IV
> is.  So if the algorithm doesn't do that then the user can't
> easily work around this.
> 
> Actually on second thought why don't we change the interface
> for ablkcipher so that we allow the IV to be returned by either
> copying it to req->info or replacing the req->info pointer?

Better copy I think, since otherwise it has to allocate (in interrupt
context) and free iv for each packet. Even if it will be preallocated
during packet setup (in setiv() for example) it is unneded additional
overhead.

> That way if the destination is linear and lowmem at the end we
> can just return a pointer to it without touching the data at all.
> 
> 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

-- 
	Evgeniy Polyakov

Re: IV copy strategy

[Herbert Xu]

On Fri, Nov 16, 2007 at 02:42:24PM +0300, Evgeniy Polyakov wrote:
>
> > Actually on second thought why don't we change the interface
> > for ablkcipher so that we allow the IV to be returned by either
> > copying it to req->info or replacing the req->info pointer?
> 
> Better copy I think, since otherwise it has to allocate (in interrupt
> context) and free iv for each packet. Even if it will be preallocated
> during packet setup (in setiv() for example) it is unneded additional
> overhead.

I think you misunderstood me.  I'm suggesting that for CBC
algorithms where the final IV is simply the last block of the
dst buffer, if the last block is in lowmem and contiguous, that
we simply put a pointer to it in place of the original IV.

There is no allocation involved.

If you can't (i.e., not CBC or if the last block isn't contiguous
or is highmem) then you just revert to copying.

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: IV copy strategy

[Herbert Xu]

On Fri, Nov 16, 2007 at 02:11:10PM +0300, Evgeniy Polyakov wrote:
>
> That's a question - should it copy IV back or not?
> Currently it is not required by crypto users.

OK I've changed my mind :)

The reason is CTR, or rather the CTR as used by IPsec.  CTR
itself should be able to chain, in fact one of the things I'll
do later is to move most of the current CTR code into a chainable
CTR with just the algorithm parameter, i.e., ctr(aes) which would
be what we currently call ctr(0,16,16).  We can then put a non-
chainable wrapper around that, perhaps ctr-ipsec(aes).

In any case, it is clear that some algorithms simply won't be
able to chain because the IV exposed to the outside is not the
complete IV.

So my plan is to add a new flag, CRYPTO_ALG_CIPHER_NOCHAIN that
you would set on algorithms that cannot be chained.  The semantics
is that everything else remains the same except that on encrypt
calls, the req->info after completion is undefined.

Users requiring chaining would then do

	crypto_alloc_blkcipher("foo", 0, CRYPTO_ALG_CIPHER_NOCHAIN)

For hardware offload devices such as yours where chaining does
not come naturally you could then elect to not implement chaining
and just set the flag.

Please let me know if you see any problems with this approach.

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: IV copy strategy

[Herbert Xu]

On Sun, Nov 18, 2007 at 02:52:37PM +0800, Herbert Xu wrote:
>
> So my plan is to add a new flag, CRYPTO_ALG_CIPHER_NOCHAIN that
> you would set on algorithms that cannot be chained.  The semantics
> is that everything else remains the same except that on encrypt
> calls, the req->info after completion is undefined.

The same would apply to decrypt calls of course.

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: IV copy strategy

[Evgeniy Polyakov]

On Sun, Nov 18, 2007 at 02:52:37PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> On Fri, Nov 16, 2007 at 02:11:10PM +0300, Evgeniy Polyakov wrote:
> >
> > That's a question - should it copy IV back or not?
> > Currently it is not required by crypto users.
> 
> OK I've changed my mind :)
> 
> The reason is CTR, or rather the CTR as used by IPsec.  CTR
> itself should be able to chain, in fact one of the things I'll
> do later is to move most of the current CTR code into a chainable
> CTR with just the algorithm parameter, i.e., ctr(aes) which would
> be what we currently call ctr(0,16,16).  We can then put a non-
> chainable wrapper around that, perhaps ctr-ipsec(aes).
> 
> In any case, it is clear that some algorithms simply won't be
> able to chain because the IV exposed to the outside is not the
> complete IV.
> 
> So my plan is to add a new flag, CRYPTO_ALG_CIPHER_NOCHAIN that
> you would set on algorithms that cannot be chained.  The semantics
> is that everything else remains the same except that on encrypt
> calls, the req->info after completion is undefined.
> 
> Users requiring chaining would then do
> 
> 	crypto_alloc_blkcipher("foo", 0, CRYPTO_ALG_CIPHER_NOCHAIN)

Hmm, users who want chaining will set flag _NOCHAIN :)
I would call it something more informative...

> For hardware offload devices such as yours where chaining does
> not come naturally you could then elect to not implement chaining
> and just set the flag.
> 
> Please let me know if you see any problems with this approach.

I'm not sure what user will do, when it request chaining, but driver
will set CRYPTO_ALG_CIPHER_NOCHAIN itself and return wrong/old in
req->info?
For IPsec it is not an issue though, but I can not say that for all.

-- 
	Evgeniy Polyakov

Re: IV copy strategy

[Herbert Xu]

On Mon, Nov 19, 2007 at 01:38:41PM +0300, Evgeniy Polyakov wrote:
>
> > Users requiring chaining would then do
> > 
> > 	crypto_alloc_blkcipher("foo", 0, CRYPTO_ALG_CIPHER_NOCHAIN)
> 
> Hmm, users who want chaining will set flag _NOCHAIN :)
> I would call it something more informative...

Yes I know, I'm just trying to save a few keystrokes since most
existing algorithms will set NOCHAIN to 0 :)

In any case, we can create a wrapper for it, something like
crypto_alloc_chaining_cipher.

> I'm not sure what user will do, when it request chaining, but driver
> will set CRYPTO_ALG_CIPHER_NOCHAIN itself and return wrong/old in
> req->info?
> For IPsec it is not an issue though, but I can not say that for all.

Drivers that set CRYPTO_ALG_CIPHER_NOCHAIN won't be returned
if the user requests for a chaining cipher.  This is the same
as when you request for a cipher with no fallback that the
system won't return one needing a fallback.

If you're worried about users doing chaining we could even
create a new frontend type for it.  So the user would need
to allocate an object of type crypto_chncipher and use that
for operations that chain.

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: IV copy strategy

[Evgeniy Polyakov]

On Mon, Nov 19, 2007 at 07:56:55PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> > I'm not sure what user will do, when it request chaining, but driver
> > will set CRYPTO_ALG_CIPHER_NOCHAIN itself and return wrong/old in
> > req->info?
> > For IPsec it is not an issue though, but I can not say that for all.
> 
> Drivers that set CRYPTO_ALG_CIPHER_NOCHAIN won't be returned
> if the user requests for a chaining cipher.  This is the same
> as when you request for a cipher with no fallback that the
> system won't return one needing a fallback.
> 
> If you're worried about users doing chaining we could even
> create a new frontend type for it.  So the user would need
> to allocate an object of type crypto_chncipher and use that
> for operations that chain.

Well, this looks like a good way forward.
Not even getting into account, that it requires essentially zero
efforts from driver writers :)

-- 
	Evgeniy Polyakov

Re: IV copy strategy

[Sebastian Siewior]

* Herbert Xu | 2007-11-16 10:08:51 [+0800]:

>On Thu, Nov 15, 2007 at 10:10:05PM +0100, Sebastian Siewior wrote:
>>
>> In this case, the s390 has the same bug (they copy the IV back after
>> blkcipher_walk_done()). Howevere it will probably never get triggered
>> because they have an aligment of 0 (what gets pushed to 3 by the crypto
>> API if I remenber correcrtly). 
>
>It only gets pushed to 3 if you use the generic CBC template, they
>don't so they will stay at 0.  In their case I also see why they
>can't just use walk->iv directly.
It also gets pushed if they use lrw (3) or xts (7). They also use the
cbc template in case of a fallback :) 

Sebastian

[PATCH] [crypto] geode: do not copy the IV too often

[Sebastian Siewior]

There is no reason to keep the IV in the private structre.
This also remove a few memcpy()s

Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---

Herbert, could I please squash that one into the bad one so there are no
broken commits?

diff --git a/drivers/crypto/geode-aes.c b/drivers/crypto/geode-aes.c
index 0ca92d4..061ad58 100644
--- a/drivers/crypto/geode-aes.c
+++ b/drivers/crypto/geode-aes.c
@@ -67,7 +67,7 @@ do_crypt(void *src, void *dst, int len, u32 flags)
 }
 
 static unsigned int
-geode_aes_crypt(struct geode_aes_op *op)
+geode_aes_crypt(struct geode_aes_op *op, u8 *iv)
 {
 	u32 flags = 0;
 	unsigned long iflags;
@@ -92,7 +92,7 @@ geode_aes_crypt(struct geode_aes_op *op)
 
 	if (op->mode == AES_MODE_CBC) {
 		flags |= AES_CTRL_CBC;
-		_writefield(AES_WRITEIV0_REG, op->iv);
+		_writefield(AES_WRITEIV0_REG, iv);
 	}
 
 	if (!(op->flags & AES_FLAGS_HIDDENKEY)) {
@@ -104,7 +104,7 @@ geode_aes_crypt(struct geode_aes_op *op)
 	BUG_ON(ret);
 
 	if (op->mode == AES_MODE_CBC)
-		_readfield(AES_WRITEIV0_REG, op->iv);
+		_readfield(AES_WRITEIV0_REG, iv);
 
 	spin_unlock_irqrestore(&lock, iflags);
 
@@ -229,7 +229,7 @@ geode_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in)
 	op->len = AES_MIN_BLOCK_SIZE;
 	op->dir = AES_DIR_ENCRYPT;
 
-	geode_aes_crypt(op);
+	geode_aes_crypt(op, NULL);
 }
 
 
@@ -250,7 +250,7 @@ geode_decrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in)
 	op->len = AES_MIN_BLOCK_SIZE;
 	op->dir = AES_DIR_DECRYPT;
 
-	geode_aes_crypt(op);
+	geode_aes_crypt(op, NULL);
 }
 
 static int fallback_init_cip(struct crypto_tfm *tfm)
@@ -315,7 +315,6 @@ geode_cbc_decrypt(struct blkcipher_desc *desc,
 
 	blkcipher_walk_init(&walk, dst, src, nbytes);
 	err = blkcipher_walk_virt(desc, &walk);
-	memcpy(op->iv, walk.iv, AES_IV_LENGTH);
 
 	while((nbytes = walk.nbytes)) {
 		op->src = walk.src.virt.addr,
@@ -324,13 +323,12 @@ geode_cbc_decrypt(struct blkcipher_desc *desc,
 		op->len = nbytes - (nbytes % AES_MIN_BLOCK_SIZE);
 		op->dir = AES_DIR_DECRYPT;
 
-		ret = geode_aes_crypt(op);
+		ret = geode_aes_crypt(op, walk.iv);
 
 		nbytes -= ret;
 		err = blkcipher_walk_done(desc, &walk, nbytes);
 	}
 
-	memcpy(walk.iv, op->iv, AES_IV_LENGTH);
 	return err;
 }
 
@@ -348,7 +346,6 @@ geode_cbc_encrypt(struct blkcipher_desc *desc,
 
 	blkcipher_walk_init(&walk, dst, src, nbytes);
 	err = blkcipher_walk_virt(desc, &walk);
-	memcpy(op->iv, walk.iv, AES_IV_LENGTH);
 
 	while((nbytes = walk.nbytes)) {
 		op->src = walk.src.virt.addr,
@@ -357,12 +354,11 @@ geode_cbc_encrypt(struct blkcipher_desc *desc,
 		op->len = nbytes - (nbytes % AES_MIN_BLOCK_SIZE);
 		op->dir = AES_DIR_ENCRYPT;
 
-		ret = geode_aes_crypt(op);
+		ret = geode_aes_crypt(op, walk.iv);
 		nbytes -= ret;
 		err = blkcipher_walk_done(desc, &walk, nbytes);
 	}
 
-	memcpy(walk.iv, op->iv, AES_IV_LENGTH);
 	return err;
 }
 
@@ -438,7 +434,7 @@ geode_ecb_decrypt(struct blkcipher_desc *desc,
 		op->len = nbytes - (nbytes % AES_MIN_BLOCK_SIZE);
 		op->dir = AES_DIR_DECRYPT;
 
-		ret = geode_aes_crypt(op);
+		ret = geode_aes_crypt(op, NULL);
 		nbytes -= ret;
 		err = blkcipher_walk_done(desc, &walk, nbytes);
 	}
@@ -468,7 +464,7 @@ geode_ecb_encrypt(struct blkcipher_desc *desc,
 		op->len = nbytes - (nbytes % AES_MIN_BLOCK_SIZE);
 		op->dir = AES_DIR_ENCRYPT;
 
-		ret = geode_aes_crypt(op);
+		ret = geode_aes_crypt(op, NULL);
 		nbytes -= ret;
 		ret =  blkcipher_walk_done(desc, &walk, nbytes);
 	}
diff --git a/drivers/crypto/geode-aes.h b/drivers/crypto/geode-aes.h
index 14cc763..d6b2824 100644
--- a/drivers/crypto/geode-aes.h
+++ b/drivers/crypto/geode-aes.h
@@ -65,7 +65,6 @@ struct geode_aes_op {
 	int len;
 
 	u8 key[AES_KEY_LENGTH];
-	u8 iv[AES_IV_LENGTH];
 
 	union {
 		struct crypto_blkcipher *blk;
-- 
1.5.3.4

Re: [PATCH] [crypto] geode: do not copy the IV too often

[Herbert Xu]

On Thu, Nov 15, 2007 at 10:23:35PM +0100, Sebastian Siewior wrote:
> There is no reason to keep the IV in the private structre.
> This also remove a few memcpy()s
> 
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

Thanks Sebastian.

How about just change op->iv to a pointer and assigning walk->iv
to it? That should make a smaller patch.

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: [PATCH] [crypto] geode: do not copy the IV too often

[Sebastian Siewior]

* Sebastian Siewior | 2007-11-15 22:23:35 [+0100]:

>There is no reason to keep the IV in the private structre.
>This also remove a few memcpy()s
>
>Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

Hi Herbert,

I saw that you rebased your cryptodev tree but you did not include
this patch. Should I resend it?

Sebastian

Re: [PATCH] [crypto] geode: do not copy the IV too often

[Herbert Xu]

On Mon, Nov 26, 2007 at 10:25:21AM +0100, Sebastian Siewior wrote:
>
> I saw that you rebased your cryptodev tree but you did not include
> this patch. Should I resend it?

I had to check my archives to refresh my memory :)

OK I dropped it because:

: How about just change op->iv to a pointer and assigning walk->iv
: to it? That should make a smaller patch.

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: [PATCH] [crypto] geode: do not copy the IV too often

[Sebastian Siewior]

* Herbert Xu | 2007-11-26 17:28:32 [+0800]:

>On Mon, Nov 26, 2007 at 10:25:21AM +0100, Sebastian Siewior wrote:
>>
>> I saw that you rebased your cryptodev tree but you did not include
>> this patch. Should I resend it?
>
>I had to check my archives to refresh my memory :)
>
>OK I dropped it because:
>
>: How about just change op->iv to a pointer and assigning walk->iv
>: to it? That should make a smaller patch.

Sorry, I've overlooked it somehow. 
Since extra four bytes don't hurt that much I will change it.

>Thanks,
Sebastian

[PATCH] [crypto] geode: do not copy the IV too often

[Sebastian Siewior]

There is no reason to keep the IV in the private structre. Instead keep
just a pointer to make the patch smaller :)
This also remove a few memcpy()s

Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
 drivers/crypto/geode-aes.c |    6 ++----
 drivers/crypto/geode-aes.h |    2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/geode-aes.c b/drivers/crypto/geode-aes.c
index 0ca92d4..68be7d0 100644
--- a/drivers/crypto/geode-aes.c
+++ b/drivers/crypto/geode-aes.c
@@ -315,7 +315,7 @@ geode_cbc_decrypt(struct blkcipher_desc *desc,
 
 	blkcipher_walk_init(&walk, dst, src, nbytes);
 	err = blkcipher_walk_virt(desc, &walk);
-	memcpy(op->iv, walk.iv, AES_IV_LENGTH);
+	op->iv = walk.iv;
 
 	while((nbytes = walk.nbytes)) {
 		op->src = walk.src.virt.addr,
@@ -330,7 +330,6 @@ geode_cbc_decrypt(struct blkcipher_desc *desc,
 		err = blkcipher_walk_done(desc, &walk, nbytes);
 	}
 
-	memcpy(walk.iv, op->iv, AES_IV_LENGTH);
 	return err;
 }
 
@@ -348,7 +347,7 @@ geode_cbc_encrypt(struct blkcipher_desc *desc,
 
 	blkcipher_walk_init(&walk, dst, src, nbytes);
 	err = blkcipher_walk_virt(desc, &walk);
-	memcpy(op->iv, walk.iv, AES_IV_LENGTH);
+	op->iv = walk.iv;
 
 	while((nbytes = walk.nbytes)) {
 		op->src = walk.src.virt.addr,
@@ -362,7 +361,6 @@ geode_cbc_encrypt(struct blkcipher_desc *desc,
 		err = blkcipher_walk_done(desc, &walk, nbytes);
 	}
 
-	memcpy(walk.iv, op->iv, AES_IV_LENGTH);
 	return err;
 }
 
diff --git a/drivers/crypto/geode-aes.h b/drivers/crypto/geode-aes.h
index 14cc763..f1855b5 100644
--- a/drivers/crypto/geode-aes.h
+++ b/drivers/crypto/geode-aes.h
@@ -65,7 +65,7 @@ struct geode_aes_op {
 	int len;
 
 	u8 key[AES_KEY_LENGTH];
-	u8 iv[AES_IV_LENGTH];
+	u8 *iv;
 
 	union {
 		struct crypto_blkcipher *blk;
-- 
1.5.3.4

Re: [PATCH] [crypto] geode: do not copy the IV too often

[Herbert Xu]

On Tue, Nov 27, 2007 at 08:33:58PM +0100, Sebastian Siewior wrote:
> There is no reason to keep the IV in the private structre. Instead keep
> just a pointer to make the patch smaller :)
> This also remove a few memcpy()s
> 
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

Patch applied.  Thanks a lot Sebastian!
-- 
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