[stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Christophe Saout]

Fix corruption issue with dm-crypt on top of software raid5. Cancelled
readahead bio's that report no error, just have BIO_UPTODATE cleared
were reported as successful reads to the higher layers (and leaving
random content in the buffer cache). Already fixed in 2.6.19.

Signed-off-by: Christophe Saout <christophe@saout.de>


--- linux-2.6.18.orig/drivers/md/dm-crypt.c	2006-09-20 05:42:06.000000000 +0200
+++ linux-2.6.18/drivers/md/dm-crypt.c	2006-12-02 03:03:36.000000000 +0100
@@ -717,13 +717,15 @@
 	if (bio->bi_size)
 		return 1;
 
+	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
+		error = -EIO;
+                        
 	bio_put(bio);
 
 	/*
 	 * successful reads are decrypted by the worker thread
 	 */
-	if ((bio_data_dir(bio) == READ)
-	    && bio_flagged(bio, BIO_UPTODATE)) {
+	if (bio_data_dir(io->bio) == READ && !error) {
 		kcryptd_queue_io(io);
 		return 0;
 	}

Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Chris Wright]

[Note: please Cc: stable@kernel.org on -stable patches]

* Christophe Saout (christophe@saout.de) wrote:
> Fix corruption issue with dm-crypt on top of software raid5. Cancelled
> readahead bio's that report no error, just have BIO_UPTODATE cleared
> were reported as successful reads to the higher layers (and leaving
> random content in the buffer cache). Already fixed in 2.6.19.

I take it this is fixed a different way in 2.6.19?  Mind clarifying the
difference?

> Signed-off-by: Christophe Saout <christophe@saout.de>
> 
> 
> --- linux-2.6.18.orig/drivers/md/dm-crypt.c	2006-09-20 05:42:06.000000000 +0200
> +++ linux-2.6.18/drivers/md/dm-crypt.c	2006-12-02 03:03:36.000000000 +0100
> @@ -717,13 +717,15 @@
>  	if (bio->bi_size)
>  		return 1;
>  
> +	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
> +		error = -EIO;
> +                        

Minor nit:  introduces trailing whitespaces, cleaned it up locally.

thanks,
-chris
--

This is a note to let you know that we have just queued up the patch titled

    Subject: dm crypt: Fix data corruption with dm-crypt over RAID5

to the 2.6.18-stable tree.  Its filename is

    dm-crypt-fix-data-corruption-with-dm-crypt-over-raid5.patch

A git repo of this tree can be found at 
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary


>From linux-kernel-owner+chrisw=40sous-sol.org-S1162719AbWLBC2Z@vger.kernel.org  Fri Dec  1 18:36:19 2006
Date: 	Sat, 02 Dec 2006 03:27:56 +0100
From: Christophe Saout <christophe@saout.de>
To: linux-kernel@vger.kernel.org
Cc: dm-crypt@saout.de, Andrey <dm-crypt-revealed-address@lelik.org>, Andrew Morton <akpm@osdl.org>, agk@redhat.com, Neil Brown <neilb@suse.de>, Jens Axboe <jens.axboe@oracle.com>, Chris Wright <chrisw@sous-sol.org>
Subject: dm crypt: Fix data corruption with dm-crypt over RAID5

Fix corruption issue with dm-crypt on top of software raid5. Cancelled
readahead bio's that report no error, just have BIO_UPTODATE cleared
were reported as successful reads to the higher layers (and leaving
random content in the buffer cache). Already fixed in 2.6.19.

Signed-off-by: Christophe Saout <christophe@saout.de>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/md/dm-crypt.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- linux-2.6.18.5.orig/drivers/md/dm-crypt.c
+++ linux-2.6.18.5/drivers/md/dm-crypt.c
@@ -717,13 +717,15 @@ static int crypt_endio(struct bio *bio, 
 	if (bio->bi_size)
 		return 1;
 
+	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
+		error = -EIO;
+
 	bio_put(bio);
 
 	/*
 	 * successful reads are decrypted by the worker thread
 	 */
-	if ((bio_data_dir(bio) == READ)
-	    && bio_flagged(bio, BIO_UPTODATE)) {
+	if (bio_data_dir(io->bio) == READ && !error) {
 		kcryptd_queue_io(io);
 		return 0;
 	}

Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Christophe Saout]

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

Am Freitag, den 01.12.2006, 19:49 -0800 schrieb Chris Wright:

> * Christophe Saout (christophe@saout.de) wrote:
> > Fix corruption issue with dm-crypt on top of software raid5. Cancelled
> > readahead bio's that report no error, just have BIO_UPTODATE cleared
> > were reported as successful reads to the higher layers (and leaving
> > random content in the buffer cache). Already fixed in 2.6.19.
> 
> I take it this is fixed a different way in 2.6.19?  Mind clarifying the
> difference?

It's more or less fixed the same way in 2.6.19. The function was
reordered by Milan Broz to accommodate the changed write path (commit
8b004457168995f2ae2a35327f885183a9e74141):

> [PATCH] dm crypt: restructure for workqueue change
>
> Restructure part of the dm-crypt code in preparation for workqueue
> changes.
>
> Use 'base_bio' or 'clone' variable names consistently throughout.  No
> functional changes are included in this patch.

"No functional changes" actually included the correct fix, accidental or
not.

> Minor nit:  introduces trailing whitespaces, cleaned it up locally.

Ouch, sorry.


[-- Attachment #2: Dies ist ein digital signierter Nachrichtenteil --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [dm-devel] [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Piet Delaney]

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

On Sat, 2006-12-02 at 03:27 +0100, Christophe Saout wrote:

Hi Christophe:

I'm wondering about trying out your patch with dm-crypt on 2.6.12. 
The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the same.

Is there a reason that this isn't necessary or would be a bad idea.
Looks like the existing code isn't checking the BIO_UPTODATE flag
before doing the bio_put(). Looks the the second part of not calling
kcryptd_queue_io() and forwarding the processing to the cryptd is
effectively the same. The 1st change will set error if BIO_UPTODATE
isn't set and that will cause the 2nd change to skip calling 
kcryptd_queue_io().

I'm not sure about the change in the arg to bio_data_dir() 
changing from bio to io->bio. Perhaps they are equivalent;
care to comment on that.

Unless I hear otherwise I'll try it out Tomorrow.

-piet

> Fix corruption issue with dm-crypt on top of software raid5. Cancelled
> readahead bio's that report no error, just have BIO_UPTODATE cleared
> were reported as successful reads to the higher layers (and leaving
> random content in the buffer cache). Already fixed in 2.6.19.
> 
> Signed-off-by: Christophe Saout <christophe-4q3lyFh4P1g@public.gmane.org>
> 
> 
> --- linux-2.6.18.orig/drivers/md/dm-crypt.c	2006-09-20 05:42:06.000000000 +0200
> +++ linux-2.6.18/drivers/md/dm-crypt.c	2006-12-02 03:03:36.000000000 +0100
> @@ -717,13 +717,15 @@
>  	if (bio->bi_size)
>  		return 1;
>  
> +	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
> +		error = -EIO;
> +                        
>  	bio_put(bio);
>  
>  	/*
>  	 * successful reads are decrypted by the worker thread
>  	 */
> -	if ((bio_data_dir(bio) == READ)
> -	    && bio_flagged(bio, BIO_UPTODATE)) {
> +	if (bio_data_dir(io->bio) == READ && !error) {
>  		kcryptd_queue_io(io);
>  		return 0;
>  	}
> 
> 
> --
> dm-devel mailing list
> dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/dm-devel
-- 
Piet Delaney                                    Phone: (408) 200-5256
Blue Lane Technologies                          Fax:   (408) 200-5299
10450 Bubb Rd.
Cupertino, Ca. 95014                            Email: piet-QZGUWFDCMdVWk0Htik3J/w@public.gmane.org

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

Re: [dm-devel] [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Piet Delaney]

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

On Sat, 2006-12-02 at 03:27 +0100, Christophe Saout wrote:

Hi Christophe:

I'm wondering about trying out your patch with dm-crypt on 2.6.12. 
The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the same.

Is there a reason that this isn't necessary or would be a bad idea.
Looks like the existing code isn't checking the BIO_UPTODATE flag
before doing the bio_put(). Looks the the second part of not calling
kcryptd_queue_io() and forwarding the processing to the cryptd is
effectively the same. The 1st change will set error if BIO_UPTODATE
isn't set and that will cause the 2nd change to skip calling 
kcryptd_queue_io().

I'm not sure about the change in the arg to bio_data_dir() 
changing from bio to io->bio. Perhaps they are equivalent;
care to comment on that.

Unless I hear otherwise I'll try it out Tomorrow.

-piet

> Fix corruption issue with dm-crypt on top of software raid5. Cancelled
> readahead bio's that report no error, just have BIO_UPTODATE cleared
> were reported as successful reads to the higher layers (and leaving
> random content in the buffer cache). Already fixed in 2.6.19.
> 
> Signed-off-by: Christophe Saout <christophe@saout.de>
> 
> 
> --- linux-2.6.18.orig/drivers/md/dm-crypt.c	2006-09-20 05:42:06.000000000 +0200
> +++ linux-2.6.18/drivers/md/dm-crypt.c	2006-12-02 03:03:36.000000000 +0100
> @@ -717,13 +717,15 @@
>  	if (bio->bi_size)
>  		return 1;
>  
> +	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
> +		error = -EIO;
> +                        
>  	bio_put(bio);
>  
>  	/*
>  	 * successful reads are decrypted by the worker thread
>  	 */
> -	if ((bio_data_dir(bio) == READ)
> -	    && bio_flagged(bio, BIO_UPTODATE)) {
> +	if (bio_data_dir(io->bio) == READ && !error) {
>  		kcryptd_queue_io(io);
>  		return 0;
>  	}
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
-- 
Piet Delaney                                    Phone: (408) 200-5256
Blue Lane Technologies                          Fax:   (408) 200-5299
10450 Bubb Rd.
Cupertino, Ca. 95014                            Email: piet@bluelane.com

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

Re: [dm-crypt] Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Christophe Saout]

Am Mittwoch, den 17.01.2007, 00:44 -0800 schrieb Piet Delaney:

> I'm wondering about trying out your patch with dm-crypt on 2.6.12. 
> The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the same.

Yes, it should apply as well.

> Is there a reason that this isn't necessary or would be a bad idea.
> Looks like the existing code isn't checking the BIO_UPTODATE flag
> before doing the bio_put(). Looks the the second part of not calling
> kcryptd_queue_io() and forwarding the processing to the cryptd is
> effectively the same. The 1st change will set error if BIO_UPTODATE
> isn't set and that will cause the 2nd change to skip calling 
> kcryptd_queue_io().

Correct.

> I'm not sure about the change in the arg to bio_data_dir() 
> changing from bio to io->bio. Perhaps they are equivalent;
> care to comment on that.

It's just to fix the use-after-free case, apart from that all bio's
involved have the same data direction, of course.

PS: Sorry because of the Wiki defacement, when I find some time I will
look into it. Just reverting won't help this time as the bots are
currently changing the pages at least once per hour.

Re: [dm-crypt] Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Piet Delaney]

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

On Mon, 2007-01-22 at 14:38 +0100, Christophe Saout wrote:
> > I'm wondering about trying out your patch with dm-crypt on 2.6.12. 
> > The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the
> same.
> 
> Yes, it should apply as well.

Christophe Saout wrote:

> Am Mittwoch, den 17.01.2007, 00:44 -0800 schrieb Piet Delaney:
> 
>> I'm wondering about trying out your patch with dm-crypt on 2.6.12.
>> The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the
same.
> 
> Yes, it should apply as well.

I'll assume that also means that applying the patch is reasonable.

In our case we aren't using raid5; how ever we are using raid to mirror
two root partitions. Your explanation to Andrew indicates the problem
should only occur when using raid5. So applying the patch should be only
have the effect of making it now possible to use raid5 with dm-crypt.
Since we are currently running raid with root encryption it feels like
it's better to include your patch.

-piet

-- 
Piet Delaney                                    Phone: (408) 200-5256
Blue Lane Technologies                          Fax:   (408) 200-5299
10450 Bubb Rd.
Cupertino, Ca. 95014                            Email: piet@bluelane.com

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

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

Re: [dm-crypt] Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Piet Delaney]

Christophe Saout wrote:

> Am Mittwoch, den 17.01.2007, 00:44 -0800 schrieb Piet Delaney:
> 
>> I'm wondering about trying out your patch with dm-crypt on 2.6.12.
>> The code in drivers/md/dm-crypt.c`crypt_endio() appears to be the same.
> 
> Yes, it should apply as well.

I'll assume that also means that applying the patch is reasonable.

In our case we aren't using raid5; how ever we are using raid to mirror
two root partitions. Your explanation to Andrew indicates the problem should
only occur when using raid5. So applying the patch should be only have the
effect of making it now possible to use raid5 with dm-crypt. Since we are
currently running raid with root encryption it feels like it's better to
include your patch.

-piet

 

Re: [stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Piet Delaney]

Christophe Saout wrote:

> Fix corruption issue with dm-crypt on top of software raid5. Cancelled
> readahead bio's that report no error, just have BIO_UPTODATE cleared
> were reported as successful reads to the higher layers (and leaving
> random content in the buffer cache). Already fixed in 2.6.19.
> 
> Signed-off-by: Christophe Saout <christophe@saout.de>
> 
> 
> --- linux-2.6.18.orig/drivers/md/dm-crypt.c   2006-09-20 05:42:06.000000000
> +0200
> +++ linux-2.6.18/drivers/md/dm-crypt.c        2006-12-02 03:03:36.000000000 +0100
> @@ -717,13 +717,15 @@
>  if (bio->bi_size)
>  return 1;
>  
> +     if (!bio_flagged(bio, BIO_UPTODATE) && !error)
> +             error = -EIO;
> +
>  bio_put(bio);
>  
>  /*
>  * successful reads are decrypted by the worker thread
>  */
> -     if ((bio_data_dir(bio) == READ)
> -         && bio_flagged(bio, BIO_UPTODATE)) {
> +     if (bio_data_dir(io->bio) == READ && !error) {
>  kcryptd_queue_io(io);

Why doesn't kcryptd_queue_io() check the return value from queue_work()?


    476 static void kcryptd_queue_io(struct crypt_io *io)
    477 {
    478         INIT_WORK(&io->work, kcryptd_do_work, io);
    479         queue_work(_kcryptd_workqueue, &io->work);
    480 }

-piet

>  return 0;
>  }

[stable][PATCH < 2.6.19] Fix data corruption with dm-crypt over RAID5

[Christophe Saout]

Fix corruption issue with dm-crypt on top of software raid5. Cancelled
readahead bio's that report no error, just have BIO_UPTODATE cleared
were reported as successful reads to the higher layers (and leaving
random content in the buffer cache). Already fixed in 2.6.19.

Signed-off-by: Christophe Saout <christophe@saout.de>


--- linux-2.6.18.orig/drivers/md/dm-crypt.c	2006-09-20 05:42:06.000000000 +0200
+++ linux-2.6.18/drivers/md/dm-crypt.c	2006-12-02 03:03:36.000000000 +0100
@@ -717,13 +717,15 @@
 	if (bio->bi_size)
 		return 1;
 
+	if (!bio_flagged(bio, BIO_UPTODATE) && !error)
+		error = -EIO;
+                        
 	bio_put(bio);
 
 	/*
 	 * successful reads are decrypted by the worker thread
 	 */
-	if ((bio_data_dir(bio) == READ)
-	    && bio_flagged(bio, BIO_UPTODATE)) {
+	if (bio_data_dir(io->bio) == READ && !error) {
 		kcryptd_queue_io(io);
 		return 0;
 	}