Re: [PATCH] Fix race between removing raid1 device and I/O error handling on underlying device

[NeilBrown <neilb@suse.de>]

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

On Mon,  8 Oct 2012 15:57:14 +0200 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Avoid calling rdev_set_badblocks() if the underlying device has been
> removed by raid1d() prior to calling fix_read_error().
> 
> This should be applicable to the 3.x stable kernel series as well.
> 
> Original bug analysis from Peng Yu:
> 
> Assume this condition:
> 
> We have a software raid1 device /dev/md0, it has two underlying
> devices, they are /dev/sda and /dev/sdb. Assign an IO to sda,
> rdev->pending add 1, an IO error occur on sda, rdev->pending dec 1,
> then wakeup the raid1d thread. At the same time, a user run such a
> command:
> 
> Kernel will set Faulty bit to rdev->flags, then wakeup raid1d
> thread. The raid1d thread will call md_check_recovery function, this
> function will call raid1_remove_disk function if it see the Faulty
> flag is set. In raid1_remove_disk function, the rdev->nr_pending is
> zero, so it will set rdev to NULL. Then raid1d thread return from
> md_check_recovery function, continue call handle_read_error to hand
> the read error, handle_read_error function will call fix_read_error
> function, in fix_read_error function, see these lines:
> 
> 		if (!success) {
> 			/* Cannot read from anywhere - mark it bad */
> 			struct md_rdev *rdev = conf->mirrors[read_disk].rdev;
> 			if (!rdev_set_badblocks(rdev, sect, s, 0))
> 				md_error(mddev, rdev);
> 			break;
> 		}
> 
> It doesn't check whether the rdev is NULL, just pass it to
> rdev_set_badblocks function, and it maybe a NULL pointer, so the
> kernel crash.
> 
> Reported-by: Peng Yu <pyu@redhat.com>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  drivers/md/raid1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 611b5f7..ffdad74 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2005,7 +2005,7 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
>  		if (!success) {
>  			/* Cannot read from anywhere - mark it bad */
>  			struct md_rdev *rdev = conf->mirrors[read_disk].rdev;
> -			if (!rdev_set_badblocks(rdev, sect, s, 0))
> +			if (rdev && !rdev_set_badblocks(rdev, sect, s, 0))
>  				md_error(mddev, rdev);
>  			break;
>  		}


Hi Jes,
this was already reported on linux-raid and I said I didn't like that fix
because I think it is wrong that the rdev could be removed while we are
trying to fix a read error on it.
So I've applied this following which is in -next.

NeilBrown


From c809064145d88b873c6acdced5a412aabbe8b8e5 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Thu, 27 Sep 2012 12:37:27 +1000
Subject: [PATCH] md/raid1: Don't release reference to device while handling
 read error.

When we get a read error, we arrange for raid1d to handle it.
Currently we release the reference on the device.  This can result
in
   conf->mirrors[read_disk].rdev
being NULL in fix_read_error, if the device happens to get removed
before the read error is handled.

So instead keep the reference until the read error has been fully
handled.

Reported-by: hank <pyu@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 3092afe..d705f9e 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -333,9 +333,10 @@ static void raid1_end_read_request(struct bio *bio, int error)
 		spin_unlock_irqrestore(&conf->device_lock, flags);
 	}
 
-	if (uptodate)
+	if (uptodate) {
 		raid_end_bio_io(r1_bio);
-	else {
+		rdev_dec_pending(conf->mirrors[mirror].rdev, conf->mddev);
+	} else {
 		/*
 		 * oops, read error:
 		 */
@@ -349,9 +350,8 @@ static void raid1_end_read_request(struct bio *bio, int error)
 			(unsigned long long)r1_bio->sector);
 		set_bit(R1BIO_ReadError, &r1_bio->state);
 		reschedule_retry(r1_bio);
+		/* don't drop the reference on read_disk yet */
 	}
-
-	rdev_dec_pending(conf->mirrors[mirror].rdev, conf->mddev);
 }
 
 static void close_write(struct r1bio *r1_bio)
@@ -2228,6 +2228,7 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
 		unfreeze_array(conf);
 	} else
 		md_error(mddev, conf->mirrors[r1_bio->read_disk].rdev);
+	rdev_dec_pending(conf->mirrors[r1_bio->read_disk].rdev, conf->mddev);
 
 	bio = r1_bio->bios[r1_bio->read_disk];
 	bdevname(bio->bi_bdev, b);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]