Re: Desynchronizing dm-raid1

Re: Desynchronizing dm-raid1

[Mikulas Patocka]

On Mon, 7 Apr 2008, Martin K. Petersen wrote:

>>>>>> "Malahal" == malahal  <malahal@us.ibm.com> writes:
>
> [I sent this last week but it never made it to the list]
>
> Malahal> Very few drivers require it, so how about an interface to
> Malahal> lock the pages of an I/O available to drivers. Only needed
> Malahal> RAID drivers would lock the I/O while it is in progress and
> Malahal> they only pay the performance penalty.  mmap pages are a bit
> Malahal> tricky. They need to go into read-only mode when an I/O is in
> Malahal> progress. I know this would likely be rejected too!!!
>
> I have exactly the same problem with the data integrity stuff I'm
> working on.
>
> Essentially a checksum gets generated when a bio is submitted, and
> both the I/O controller and the disk drive verify the checksum.
>
> With ext2 in particular I often experience that the page (usually
> containing directory metadata) has been modified before the controller
> does the DMA.  And the I/O will then be rejected by the controller or
> drive because the checksum doesn't match the data.
>
> So this problem is not specific to DM/MD...
>
> -- 
> Martin K. Petersen	Oracle Linux Engineering

BTW: is it guaranteed for crypto functions that they read the source data 
only once?

I.e. if you encrypt a block while another CPU modifies it, and then 
decrypt it, is it guaranteed that each byte of the result will be either 
old byte or new byte of the original data?

If not, we have a subtle case of disk corruption here too. (imagine that 
you update for example atime field in inode while the block is being 
written and the crypto interface will trash the inode block).

Mikulas

Re: [dm-devel] Desynchronizing dm-raid1

[Herbert Xu]

Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> BTW: is it guaranteed for crypto functions that they read the source data 
> only once?

There is no such guarantee.

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: Desynchronizing dm-raid1

[Mikulas Patocka]

On Tue, 6 May 2008, Herbert Xu wrote:

> Mikulas Patocka <mpatocka@redhat.com> wrote:
>>
>> BTW: is it guaranteed for crypto functions that they read the source data
>> only once?
>
> There is no such guarantee.
>
> Cheers,

So we'll have to copy the sector to temporary space before encrypting it.

I'll look at it.

Mikulas

Re: Desynchronizing dm-raid1

[Mikulas Patocka]

On Tue, 6 May 2008, Mikulas Patocka wrote:

> On Tue, 6 May 2008, Herbert Xu wrote:
>
>> Mikulas Patocka <mpatocka@redhat.com> wrote:
>>> 
>>> BTW: is it guaranteed for crypto functions that they read the source data
>>> only once?
>> 
>> There is no such guarantee.
>> 
>> Cheers,
>
> So we'll have to copy the sector to temporary space before encrypting it.
>
> I'll look at it.
>
> Mikulas

Do you think that it would make sense to add a flag to crypto API that 
says that the encryption function reads the input only once? --- so that 
we wouldn't have to copy the data when using an algorithm which doesn't 
need it.

Or do you thint it would be useless (all major disk-encryption algorithms 
read input buffer more times?) or it would create too much code 
complexity?

Mikulas

Re: [dm-devel] Desynchronizing dm-raid1

[Herbert Xu]

On Mon, May 12, 2008 at 11:28:44PM -0400, Mikulas Patocka wrote:
> 
> Or do you thint it would be useless (all major disk-encryption algorithms 
> read input buffer more times?) or it would create too much code 
> complexity?

I'm open to this approach.  As long as you do the work and come up
with the patch that is :)

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: Desynchronizing dm-raid1

[Mikulas Patocka]

On Tue, 13 May 2008, Herbert Xu wrote:

> On Mon, May 12, 2008 at 11:28:44PM -0400, Mikulas Patocka wrote:
>>
>> Or do you thint it would be useless (all major disk-encryption algorithms
>> read input buffer more times?) or it would create too much code
>> complexity?
>
> I'm open to this approach.  As long as you do the work and come up
> with the patch that is :)
>
> Cheers,

And where would you propose to place this bit?

One possibility would be struct crypto_tfm->crt_flags
Another possibility is struct crypto_alg->cra_flags

Can chaining mode change the value of the flag? (I presume that yes)

So make a bit in struct crypto_alg. And propagate it to 
crypto_alg->cra_flags of chaining modes that read input data only once?

I'm not familiar with Linux crypto API, so the location of the flag should 
be selected by someone who knows it well.

Here I'm sending the patch for dm-crypt. To optimize it further, you 
should add the test after bio_data_dir(ctx->bio_in) == WRITE test --- i.e. 
if the algorithm is safe to read input only once, the smaller and faster 
branch can be executed.

Mikukas

---

Copy data when writing to the device.

This patch fixes damaged blocks on encrypted device when the computer crashes.

Linux IO architecture allows the data to be modified while they are being
written.

While we are processing write requests, the data may change, and it is expected,
that each byte written to the device is either old byte or new byte.

Some crypto functions may read the input buffer more than once and so they'll
produce garbage if the buffer changes under them. When this garbage is
read again and decrypted, it may produce damage in all bytes in the block. This
damage is visible only if the computer crashes; if it didn't crash, the memory
manager writes correct buffer or page contents some times later.

So we first copy data to a temporary buffer with memcpy and then encrypt them
in place.

Mikulas Patocka <mpatocka@redhat.com>

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

Index: linux-2.6.25.3/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.25.3.orig/drivers/md/dm-crypt.c	2008-05-13 21:48:32.000000000 +0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c	2008-05-13 22:14:12.000000000 +0200
@@ -360,8 +360,18 @@ static int crypt_convert_block(struct cr
  			 crypto_ablkcipher_alignmask(cc->tfm) + 1);

  	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);
+	if (bio_data_dir(ctx->bio_in) == WRITE) {
+		char *page_in = kmap_atomic(bv_in->bv_page, KM_USER0);
+		char *page_out = kmap_atomic(bv_out->bv_page, KM_USER1);
+		memcpy(page_out + bv_out->bv_offset + ctx->offset_out, page_in + bv_in->bv_offset + ctx->offset_in, 1 << SECTOR_SHIFT);
+		kunmap_atomic(page_in, KM_USER0);
+		kunmap_atomic(page_out, KM_USER1);
+
+		sg_set_page(&dmreq->sg_in, bv_out->bv_page, 1 << SECTOR_SHIFT,
+			    bv_out->bv_offset + ctx->offset_out);
+	} else
+		sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
+			    bv_in->bv_offset + ctx->offset_in);

  	sg_init_table(&dmreq->sg_out, 1);
  	sg_set_page(&dmreq->sg_out, bv_out->bv_page, 1 << SECTOR_SHIFT,

Re: [dm-devel] Desynchronizing dm-raid1

[Herbert Xu]

On Tue, May 13, 2008 at 04:35:03PM -0400, Mikulas Patocka wrote:
>
> And where would you propose to place this bit?
> 
> One possibility would be struct crypto_tfm->crt_flags
> Another possibility is struct crypto_alg->cra_flags

The latter definitely because this is an algorithm property.

> Can chaining mode change the value of the flag? (I presume that yes)

If you mean templates like CBC then it depends.  You should
set it to zero by default for safety but most of them should
be able to turn it on once audited.

If it turns out that the majority of algorithms support this,
you could even decide to only select those algorithms that do.
Suppose your bit is

	CRYPTO_ALG_FOO

then you could do

	crypto_alloc_blkcipher(name, CRYPTO_ALG_FOO, CRYPTO_ALG_FOO)

to demand only those algorithms that comply.

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: Desynchronizing dm-raid1

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1529 bytes --]



On Wed, 14 May 2008, Herbert Xu wrote:

> On Tue, May 13, 2008 at 04:35:03PM -0400, Mikulas Patocka wrote:
>>
>> And where would you propose to place this bit?
>>
>> One possibility would be struct crypto_tfm->crt_flags
>> Another possibility is struct crypto_alg->cra_flags
>
> The latter definitely because this is an algorithm property.
>
>> Can chaining mode change the value of the flag? (I presume that yes)
>
> If you mean templates like CBC then it depends.  You should
> set it to zero by default for safety but most of them should
> be able to turn it on once audited.
>
> If it turns out that the majority of algorithms support this,
> you could even decide to only select those algorithms that do.
> Suppose your bit is
>
> 	CRYPTO_ALG_FOO
>
> then you could do
>
> 	crypto_alloc_blkcipher(name, CRYPTO_ALG_FOO, CRYPTO_ALG_FOO)
>
> to demand only those algorithms that comply.
>
> Cheers,

Hi

Here I send the patches, the first one copied data in dm-crypt 
unconditionally. The second one adds a flag to coplying algorithms. The 
third one skips the copy for complying algorithms. The fourth one is 
removing useless increment in arc4 that I found while reviewing the 
ciphers.

All the ciphers comply, so the bug is only a theroretical issue (but I 
didn't check assembler versions --- they should be checked by the person 
who wrote them, assembler is write-only language).

Please review my changes to crypto code, I am not crypto developer and I 
do not understand it as well as people maintaining it.

Mikulas

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2242 bytes --]

Copy data when writing to the device.

This patch fixes damaged blocks on encrypted device when the computer crashes.

Linux IO architecture allows the data to be modified while they are being
written.

While we are processing write requests, the data may change, and it is expected,
that each byte written to the device is either old byte or new byte.

Some crypto functions may read the input buffer more than once and so they'll
produce garbage if the buffer changes under them. When this garbage is
read again and decrypted, it may produce damage in all bytes in the block. This
damage is visible only if the computer crashes; if it didn't crash, the memory
manager writes correct buffer or page contents some times later.

So we first copy data to a temporary buffer with memcpy and then encrypt them
in place.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

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

Index: linux-2.6.25.3/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.25.3.orig/drivers/md/dm-crypt.c	2008-05-14 18:41:10.000000000 +0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c	2008-05-14 18:41:11.000000000 +0200
@@ -361,8 +361,18 @@ static int crypt_convert_block(struct cr
 			 crypto_ablkcipher_alignmask(cc->tfm) + 1);
 
 	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);
+	if (bio_data_dir(ctx->bio_in) == WRITE) {
+		char *page_in = kmap_atomic(bv_in->bv_page, KM_USER0);
+		char *page_out = kmap_atomic(bv_out->bv_page, KM_USER1);
+		memcpy(page_out + bv_out->bv_offset + ctx->offset_out, page_in + bv_in->bv_offset + ctx->offset_in, 1 << SECTOR_SHIFT);
+		kunmap_atomic(page_in, KM_USER0);
+		kunmap_atomic(page_out, KM_USER1);
+
+		sg_set_page(&dmreq->sg_in, bv_out->bv_page, 1 << SECTOR_SHIFT,
+			    bv_out->bv_offset + ctx->offset_out);
+	} else
+		sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
+			    bv_in->bv_offset + ctx->offset_in);
 
 	sg_init_table(&dmreq->sg_out, 1);
 	sg_set_page(&dmreq->sg_out, bv_out->bv_page, 1 << SECTOR_SHIFT,

[-- Attachment #3: Type: TEXT/PLAIN, Size: 18242 bytes --]

Add a flag that tells that the algorithm reads the data only once when
encrypting.

It will be used by dm-crypt to skip a copy operation if not needed.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 crypto/aes_generic.c     |    2 +-
 crypto/anubis.c          |    2 +-
 crypto/arc4.c            |    2 +-
 crypto/blkcipher.c       |    2 +-
 crypto/blowfish.c        |    2 +-
 crypto/camellia.c        |    2 +-
 crypto/cast5.c           |    2 +-
 crypto/cast6.c           |    2 +-
 crypto/cbc.c             |    2 +-
 crypto/cryptd.c          |    2 +-
 crypto/crypto_null.c     |    2 +-
 crypto/ctr.c             |    2 +-
 crypto/des_generic.c     |    4 ++--
 crypto/ecb.c             |    2 +-
 crypto/fcrypt.c          |    2 +-
 crypto/khazad.c          |    2 +-
 crypto/lrw.c             |    2 +-
 crypto/pcbc.c            |    2 +-
 crypto/salsa20_generic.c |    2 +-
 crypto/seed.c            |    2 +-
 crypto/serpent.c         |    4 ++--
 crypto/tea.c             |    4 ++--
 crypto/twofish.c         |    2 +-
 crypto/xts.c             |    2 +-
 include/linux/crypto.h   |    9 +++++++++
 25 files changed, 36 insertions(+), 27 deletions(-)

Index: linux-2.6.25.3/include/linux/crypto.h
===================================================================
--- linux-2.6.25.3.orig/include/linux/crypto.h	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/include/linux/crypto.h	2008-05-21 18:54:43.000000000 +0200
@@ -59,6 +59,15 @@
 #define CRYPTO_ALG_GENIV		0x00000200
 
 /*
+ * The encryption algorithm reads the data only once. So it can encrypt
+ * data while they change.
+ *
+ * This is useful for disk encryption, so that the sector doesn't have
+ * to be copied to temporary space before encrypting.
+ */
+#define CRYPTO_ALG_ENCRYPT_READ_ONCE	0x00000400
+
+/*
  * Transform masks and values (for crt_flags).
  */
 #define CRYPTO_TFM_REQ_MASK		0x000fff00
Index: linux-2.6.25.3/crypto/cbc.c
===================================================================
--- linux-2.6.25.3.orig/crypto/cbc.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/cbc.c	2008-05-21 19:32:49.000000000 +0200
@@ -234,7 +234,7 @@ static struct crypto_instance *crypto_cb
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | (alg->cra_flags & CRYPTO_ALG_ENCRYPT_READ_ONCE);
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;
Index: linux-2.6.25.3/crypto/ecb.c
===================================================================
--- linux-2.6.25.3.orig/crypto/ecb.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/ecb.c	2008-05-21 16:37:48.000000000 +0200
@@ -134,7 +134,7 @@ static struct crypto_instance *crypto_ec
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | (alg->cra_flags & CRYPTO_ALG_ENCRYPT_READ_ONCE);
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;
Index: linux-2.6.25.3/crypto/ctr.c
===================================================================
--- linux-2.6.25.3.orig/crypto/ctr.c	2008-05-21 16:43:23.000000000 +0200
+++ linux-2.6.25.3/crypto/ctr.c	2008-05-21 16:48:35.000000000 +0200
@@ -200,7 +200,7 @@ static struct crypto_instance *crypto_ct
 	if (IS_ERR(inst))
 		goto out;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE;
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = 1;
 	inst->alg.cra_alignmask = alg->cra_alignmask | (__alignof__(u32) - 1);
Index: linux-2.6.25.3/crypto/lrw.c
===================================================================
--- linux-2.6.25.3.orig/crypto/lrw.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/lrw.c	2008-05-21 17:37:11.000000000 +0200
@@ -247,7 +247,7 @@ static struct crypto_instance *alloc(str
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE;
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 
Index: linux-2.6.25.3/crypto/xts.c
===================================================================
--- linux-2.6.25.3.orig/crypto/xts.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/xts.c	2008-05-21 17:38:23.000000000 +0200
@@ -230,7 +230,7 @@ static struct crypto_instance *alloc(str
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE;
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 
Index: linux-2.6.25.3/crypto/aes_generic.c
===================================================================
--- linux-2.6.25.3.orig/crypto/aes_generic.c	2008-05-21 17:54:16.000000000 +0200
+++ linux-2.6.25.3/crypto/aes_generic.c	2008-05-21 17:54:27.000000000 +0200
@@ -434,7 +434,7 @@ static struct crypto_alg aes_alg = {
 	.cra_name		=	"aes",
 	.cra_driver_name	=	"aes-generic",
 	.cra_priority		=	100,
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	AES_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct crypto_aes_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/cryptd.c
===================================================================
--- linux-2.6.25.3.orig/crypto/cryptd.c	2008-05-21 17:41:48.000000000 +0200
+++ linux-2.6.25.3/crypto/cryptd.c	2008-05-21 17:42:32.000000000 +0200
@@ -238,7 +238,7 @@ static struct crypto_instance *cryptd_al
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC | (alg->cra_flags & CRYPTO_ALG_ENCRYPT_READ_ONCE);
 	inst->alg.cra_type = &crypto_ablkcipher_type;
 
 	inst->alg.cra_ablkcipher.ivsize = alg->cra_blkcipher.ivsize;
Index: linux-2.6.25.3/crypto/pcbc.c
===================================================================
--- linux-2.6.25.3.orig/crypto/pcbc.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/pcbc.c	2008-05-21 17:40:21.000000000 +0200
@@ -240,7 +240,7 @@ static struct crypto_instance *crypto_pc
 	if (IS_ERR(inst))
 		goto out_put_alg;
 
-	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER;
+	inst->alg.cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER | (alg->cra_flags & CRYPTO_ALG_ENCRYPT_READ_ONCE);
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;
Index: linux-2.6.25.3/crypto/salsa20_generic.c
===================================================================
--- linux-2.6.25.3.orig/crypto/salsa20_generic.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/salsa20_generic.c	2008-05-21 17:47:22.000000000 +0200
@@ -218,7 +218,7 @@ static struct crypto_alg alg = {
 	.cra_name           =   "salsa20",
 	.cra_driver_name    =   "salsa20-generic",
 	.cra_priority       =   100,
-	.cra_flags          =   CRYPTO_ALG_TYPE_BLKCIPHER,
+	.cra_flags          =   CRYPTO_ALG_TYPE_BLKCIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_type           =   &crypto_blkcipher_type,
 	.cra_blocksize      =   1,
 	.cra_ctxsize        =   sizeof(struct salsa20_ctx),
Index: linux-2.6.25.3/crypto/anubis.c
===================================================================
--- linux-2.6.25.3.orig/crypto/anubis.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/anubis.c	2008-05-21 17:56:04.000000000 +0200
@@ -673,7 +673,7 @@ static void anubis_decrypt(struct crypto
 
 static struct crypto_alg anubis_alg = {
 	.cra_name		=	"anubis",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	ANUBIS_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof (struct anubis_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/arc4.c
===================================================================
--- linux-2.6.25.3.orig/crypto/arc4.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/arc4.c	2008-05-21 18:00:38.000000000 +0200
@@ -72,7 +72,7 @@ static void arc4_crypt(struct crypto_tfm
 
 static struct crypto_alg arc4_alg = {
 	.cra_name		=	"arc4",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	ARC4_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct arc4_ctx),
 	.cra_module		=	THIS_MODULE,
Index: linux-2.6.25.3/crypto/blowfish.c
===================================================================
--- linux-2.6.25.3.orig/crypto/blowfish.c	2008-05-21 18:00:38.000000000 +0200
+++ linux-2.6.25.3/crypto/blowfish.c	2008-05-21 18:00:55.000000000 +0200
@@ -451,7 +451,7 @@ static int bf_setkey(struct crypto_tfm *
 
 static struct crypto_alg alg = {
 	.cra_name		=	"blowfish",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	BF_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct bf_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/camellia.c
===================================================================
--- linux-2.6.25.3.orig/crypto/camellia.c	2008-05-21 18:02:11.000000000 +0200
+++ linux-2.6.25.3/crypto/camellia.c	2008-05-21 18:02:24.000000000 +0200
@@ -1094,7 +1094,7 @@ static struct crypto_alg camellia_alg = 
 	.cra_name		=	"camellia",
 	.cra_driver_name	=	"camellia-generic",
 	.cra_priority		=	100,
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	CAMELLIA_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct camellia_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/cast5.c
===================================================================
--- linux-2.6.25.3.orig/crypto/cast5.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/cast5.c	2008-05-21 18:03:39.000000000 +0200
@@ -800,7 +800,7 @@ static int cast5_setkey(struct crypto_tf
 
 static struct crypto_alg alg = {
 	.cra_name 	= "cast5",
-	.cra_flags 	= CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags 	= CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize 	= CAST5_BLOCK_SIZE,
 	.cra_ctxsize 	= sizeof(struct cast5_ctx),
 	.cra_alignmask	= 3,
Index: linux-2.6.25.3/crypto/cast6.c
===================================================================
--- linux-2.6.25.3.orig/crypto/cast6.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/cast6.c	2008-05-21 18:04:32.000000000 +0200
@@ -512,7 +512,7 @@ static void cast6_decrypt(struct crypto_
 
 static struct crypto_alg alg = {
 	.cra_name = "cast6",
-	.cra_flags = CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags = CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize = CAST6_BLOCK_SIZE,
 	.cra_ctxsize = sizeof(struct cast6_ctx),
 	.cra_alignmask = 3,
Index: linux-2.6.25.3/crypto/crypto_null.c
===================================================================
--- linux-2.6.25.3.orig/crypto/crypto_null.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/crypto_null.c	2008-05-21 18:05:41.000000000 +0200
@@ -123,7 +123,7 @@ static struct crypto_alg skcipher_null =
 	.cra_name		=	"ecb(cipher_null)",
 	.cra_driver_name	=	"ecb-cipher_null",
 	.cra_priority		=	100,
-	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	NULL_BLOCK_SIZE,
 	.cra_type		=	&crypto_blkcipher_type,
 	.cra_ctxsize		=	0,
Index: linux-2.6.25.3/crypto/des_generic.c
===================================================================
--- linux-2.6.25.3.orig/crypto/des_generic.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/des_generic.c	2008-05-21 18:07:33.000000000 +0200
@@ -945,7 +945,7 @@ static void des3_ede_decrypt(struct cryp
 
 static struct crypto_alg des_alg = {
 	.cra_name		=	"des",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	DES_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct des_ctx),
 	.cra_module		=	THIS_MODULE,
@@ -961,7 +961,7 @@ static struct crypto_alg des_alg = {
 
 static struct crypto_alg des3_ede_alg = {
 	.cra_name		=	"des3_ede",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	DES3_EDE_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct des3_ede_ctx),
 	.cra_module		=	THIS_MODULE,
Index: linux-2.6.25.3/crypto/fcrypt.c
===================================================================
--- linux-2.6.25.3.orig/crypto/fcrypt.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/fcrypt.c	2008-05-21 18:08:16.000000000 +0200
@@ -391,7 +391,7 @@ static int fcrypt_setkey(struct crypto_t
 
 static struct crypto_alg fcrypt_alg = {
 	.cra_name		=	"fcrypt",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	8,
 	.cra_ctxsize		=	sizeof(struct fcrypt_ctx),
 	.cra_module		=	THIS_MODULE,
Index: linux-2.6.25.3/crypto/khazad.c
===================================================================
--- linux-2.6.25.3.orig/crypto/khazad.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/khazad.c	2008-05-21 18:09:07.000000000 +0200
@@ -848,7 +848,7 @@ static void khazad_decrypt(struct crypto
 
 static struct crypto_alg khazad_alg = {
 	.cra_name		=	"khazad",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	KHAZAD_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof (struct khazad_ctx),
 	.cra_alignmask		=	7,
Index: linux-2.6.25.3/crypto/seed.c
===================================================================
--- linux-2.6.25.3.orig/crypto/seed.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/seed.c	2008-05-21 18:09:52.000000000 +0200
@@ -444,7 +444,7 @@ static struct crypto_alg seed_alg = {
 	.cra_name		=	"seed",
 	.cra_driver_name	=	"seed-generic",
 	.cra_priority		=	100,
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	SEED_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct seed_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/serpent.c
===================================================================
--- linux-2.6.25.3.orig/crypto/serpent.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/serpent.c	2008-05-21 18:12:08.000000000 +0200
@@ -475,7 +475,7 @@ static void serpent_decrypt(struct crypt
 
 static struct crypto_alg serpent_alg = {
 	.cra_name		=	"serpent",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	SERPENT_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct serpent_ctx),
 	.cra_alignmask		=	3,
@@ -543,7 +543,7 @@ static void tnepres_decrypt(struct crypt
 
 static struct crypto_alg tnepres_alg = {
 	.cra_name		=	"tnepres",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	SERPENT_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof(struct serpent_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/tea.c
===================================================================
--- linux-2.6.25.3.orig/crypto/tea.c	2008-04-17 04:49:44.000000000 +0200
+++ linux-2.6.25.3/crypto/tea.c	2008-05-21 18:14:39.000000000 +0200
@@ -221,7 +221,7 @@ static void xeta_decrypt(struct crypto_t
 
 static struct crypto_alg tea_alg = {
 	.cra_name		=	"tea",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	TEA_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof (struct tea_ctx),
 	.cra_alignmask		=	3,
@@ -253,7 +253,7 @@ static struct crypto_alg xtea_alg = {
 
 static struct crypto_alg xeta_alg = {
 	.cra_name		=	"xeta",
-	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize		=	XTEA_BLOCK_SIZE,
 	.cra_ctxsize		=	sizeof (struct xtea_ctx),
 	.cra_alignmask		=	3,
Index: linux-2.6.25.3/crypto/twofish.c
===================================================================
--- linux-2.6.25.3.orig/crypto/twofish.c	2008-05-21 18:16:51.000000000 +0200
+++ linux-2.6.25.3/crypto/twofish.c	2008-05-21 18:16:59.000000000 +0200
@@ -183,7 +183,7 @@ static struct crypto_alg alg = {
 	.cra_name           =   "twofish",
 	.cra_driver_name    =   "twofish-generic",
 	.cra_priority       =   100,
-	.cra_flags          =   CRYPTO_ALG_TYPE_CIPHER,
+	.cra_flags          =   CRYPTO_ALG_TYPE_CIPHER | CRYPTO_ALG_ENCRYPT_READ_ONCE,
 	.cra_blocksize      =   TF_BLOCK_SIZE,
 	.cra_ctxsize        =   sizeof(struct twofish_ctx),
 	.cra_alignmask      =	3,
Index: linux-2.6.25.3/crypto/blkcipher.c
===================================================================
--- linux-2.6.25.3.orig/crypto/blkcipher.c	2008-05-21 19:43:31.000000000 +0200
+++ linux-2.6.25.3/crypto/blkcipher.c	2008-05-21 20:09:11.000000000 +0200
@@ -640,7 +640,7 @@ struct crypto_instance *skcipher_geniv_a
 	}
 
 	inst->alg.cra_flags = CRYPTO_ALG_TYPE_GIVCIPHER | CRYPTO_ALG_GENIV;
-	inst->alg.cra_flags |= alg->cra_flags & CRYPTO_ALG_ASYNC;
+	inst->alg.cra_flags |= alg->cra_flags & (CRYPTO_ALG_ASYNC | CRYPTO_ALG_ENCRYPT_READ_ONCE);
 	inst->alg.cra_priority = alg->cra_priority;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;

[-- Attachment #4: Type: TEXT/PLAIN, Size: 1116 bytes --]

Use the flag CRYPTO_ALG_ENCRYPT_READ_ONCE. If the flag is set, we don't have
to copy when encrypting and we can revert to old behavior.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-crypt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.25.3/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.25.3.orig/drivers/md/dm-crypt.c	2008-05-21 20:09:05.000000000 +0200
+++ linux-2.6.25.3/drivers/md/dm-crypt.c	2008-05-21 20:12:03.000000000 +0200
@@ -361,7 +361,7 @@ static int crypt_convert_block(struct cr
 			 crypto_ablkcipher_alignmask(cc->tfm) + 1);
 
 	sg_init_table(&dmreq->sg_in, 1);
-	if (bio_data_dir(ctx->bio_in) == WRITE) {
+	if (bio_data_dir(ctx->bio_in) == WRITE && !(cc->tfm->base.__crt_alg->cra_flags & CRYPTO_ALG_ENCRYPT_READ_ONCE)) {
 		char *page_in = kmap_atomic(bv_in->bv_page, KM_USER0);
 		char *page_out = kmap_atomic(bv_out->bv_page, KM_USER1);
 		memcpy(page_out + bv_out->bv_offset + ctx->offset_out, page_in + bv_in->bv_offset + ctx->offset_in, 1 << SECTOR_SHIFT);

[-- Attachment #5: Type: TEXT/PLAIN, Size: 642 bytes --]

Remove an useless increment in arc4.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 crypto/arc4.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.25.3/crypto/arc4.c
===================================================================
--- linux-2.6.25.3.orig/crypto/arc4.c	2008-05-21 17:57:24.000000000 +0200
+++ linux-2.6.25.3/crypto/arc4.c	2008-05-21 17:58:28.000000000 +0200
@@ -64,7 +64,7 @@ static void arc4_crypt(struct crypto_tfm
 	S[x] = b;
 	S[y] = a;
 	x = (x + 1) & 0xff;
-	*out++ = *in ^ S[(a + b) & 0xff];
+	*out = *in ^ S[(a + b) & 0xff];
 
 	ctx->x = x;
 	ctx->y = y;

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

Re: [dm-devel] Desynchronizing dm-raid1

[Herbert Xu]

On Wed, May 21, 2008 at 10:18:43PM -0400, Mikulas Patocka wrote:
>
> All the ciphers comply, so the bug is only a theroretical issue (but I 
> didn't check assembler versions --- they should be checked by the person 
> who wrote them, assembler is write-only language).

Since every current algorithm sets the flag could you invert
its sense? Sorry to have to do this to you :)

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: Desynchronizing dm-raid1

[Mikulas Patocka]

>> All the ciphers comply, so the bug is only a theroretical issue (but I
>> didn't check assembler versions --- they should be checked by the person
>> who wrote them, assembler is write-only language).
>
> Since every current algorithm sets the flag could you invert
> its sense? Sorry to have to do this to you :)
>
> Thanks,

There may be external modules.

If you don't set the flag when it should be set, nothing happens (just a 
slight performance drop), if you set the flag when it shouldn't be set, 
you get data corruption. So the safest way is this meaning of flag, so 
that not-yet-reviewed algorithms set the flag to 0 and prevent data 
corruption.

Mikulas

Re: [dm-devel] Desynchronizing dm-raid1

[Herbert Xu]

On Thu, May 22, 2008 at 08:32:45AM -0400, Mikulas Patocka wrote:
>
> There may be external modules.

Sorry but we don't support external modules.  They should be merged
upstream rather than distributed in the wild.

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: Desynchronizing dm-raid1

[Mikulas Patocka]

> On Thu, May 22, 2008 at 08:32:45AM -0400, Mikulas Patocka wrote:
>>
>> There may be external modules.
>
> Sorry but we don't support external modules.  They should be merged
> upstream rather than distributed in the wild.
>
> Cheers,

If you want to negate the meaning of the flag, then you have to write it 
yourself. I, as non-developer of crypto code, can prove that on given path 
the input data are read only once --- but I can't prove that on all paths 
and all possible chaining modes of algorithms the data are read once, 
because I don't know about all of them.

Mikulas

Re: Desynchronizing dm-raid1

[Herbert Xu]

On Fri, May 23, 2008 at 10:59:33AM -0400, Mikulas Patocka wrote:
>
> If you want to negate the meaning of the flag, then you have to write it 
> yourself. I, as non-developer of crypto code, can prove that on given path 
> the input data are read only once --- but I can't prove that on all paths 
> and all possible chaining modes of algorithms the data are read once, 
> because I don't know about all of them.

Huh? Inverting it would give exactly the same result as your current
patch.  If you're not confident with it inverted, then I can't see
how you could be confident about the patch as it is.

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: Desynchronizing dm-raid1

[Mikulas Patocka]

>> If you want to negate the meaning of the flag, then you have to write it
>> yourself. I, as non-developer of crypto code, can prove that on given path
>> the input data are read only once --- but I can't prove that on all paths
>> and all possible chaining modes of algorithms the data are read once,
>> because I don't know about all of them.
>
> Huh? Inverting it would give exactly the same result as your current
> patch.  If you're not confident with it inverted, then I can't see
> how you could be confident about the patch as it is.

If you don't set the bit when it should be set => you get slight 
performance drop (problem 1)

You set the bit when it shouldn't be set => you get data corruption 
(problem 2)


Problem 1 is acceptable, problem 2 is not.

So if I forgot some code path when the bit should be set, I created 
problem 1, but I am sure that I didn't create problem 2.


If you invert the meaning of the bit, you risk creating problem 2. And you 
need more review (by someone who understands what all structures are being 
created in crypto code).

Mikulas