Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in fix_read_error function

From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
From: hank <[email protected]>
Date: Tue, 4 Sep 2012 10:23:45 +0800
Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in
fix_read_error function

in fix_read_error function, the conf->mirrors[read_disk].rdev may become NULL, as in this function, rdev->nr_pending may be zero, anyone can delete it. So should check if it is NULL before use.

Signed-off-by hank <[email protected]>

---
 drivers/md/raid1.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/md/raid1.c +0 -0

index 611b5f7..fd8de28 100644

Quoted 24 lines · hank

From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
From: hank <[email protected]>
Date: Tue, 4 Sep 2012 10:23:45 +0800
Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in
fix_read_error function

in fix_read_error function, the conf->mirrors[read_disk].rdev may become NULL, as in this function, rdev->nr_pending may be zero, anyone can delete it. So should check if it is NULL before use.

Signed-off-by hank <[email protected]>

---
 drivers/md/raid1.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/md/raid1.c +0 -1

index 611b5f7..fd8de28 100644
@@ -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))

drivers/md/raid1.c static void fix_read_error(struct r1conf *conf, int read_disk, · line 2011

Quoted 4 lines

drivers/md/raid1.c +1 -0

+			if (!rdev || !rdev_set_badblocks(rdev, sect, s, 0))
 				md_error(mddev, rdev);
 			break;
 		}

Anyone can review this patch? I think it is a bug and should be fixed.

Quoted 26 lines · On Thu, 13 Sep 2012 10:28:32 +0800 hank <[email protected]> wrote:

Quoted 24 lines · hank

From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
From: hank <[email protected]>
Date: Tue, 4 Sep 2012 10:23:45 +0800
Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in
fix_read_error function

in fix_read_error function, the conf->mirrors[read_disk].rdev may become NULL, as in this function, rdev->nr_pending may be zero, anyone can delete it. So should check if it is NULL before use.

Signed-off-by hank <[email protected]>

---
 drivers/md/raid1.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/md/raid1.c +0 -1

index 611b5f7..fd8de28 100644
@@ -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))

Quoted 8 lines

drivers/md/raid1.c static void fix_read_error(struct r1conf *conf, int read_disk, · line 2011

Quoted 4 lines

drivers/md/raid1.c +1 -0

+			if (!rdev || !rdev_set_badblocks(rdev, sect, s, 0))
 				md_error(mddev, rdev);
 			break;
 		}

Anyone can review this patch? I think it is a bug and should be fixed.

I agree there is a bug there but I don't think this is the right fix. If rdev could be NULL there, then it could also be NULL in

		md_error(mddev, conf->mirrors[r1_bio->read_disk].rdev);

in handle_read_error().
I think we should just hold on to the reference to the rdev until we are done with it, like the follow.

Would you agree?

Thanks,
NeilBrown

drivers/md/raid1.c +4 -4

index 611b5f7..eb1f8a3 100644
@@ -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)

Quoted 78 lines · NeilBrown

Quoted 26 lines · On Thu, 13 Sep 2012 10:28:32 +0800 hank <[email protected]> wrote:

Quoted 24 lines · hank

From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
From: hank <[email protected]>
Date: Tue, 4 Sep 2012 10:23:45 +0800
Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in
fix_read_error function

in fix_read_error function, the conf->mirrors[read_disk].rdev may become NULL, as in this function, rdev->nr_pending may be zero, anyone can delete it. So should check if it is NULL before use.

Signed-off-by hank <[email protected]>

---
 drivers/md/raid1.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/md/raid1.c +0 -1

index 611b5f7..fd8de28 100644
@@ -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))

Quoted 8 lines

drivers/md/raid1.c static void fix_read_error(struct r1conf *conf, int read_disk, · line 2011

Quoted 4 lines

drivers/md/raid1.c +1 -0

+			if (!rdev || !rdev_set_badblocks(rdev, sect, s, 0))
 				md_error(mddev, rdev);
 			break;
 		}

Anyone can review this patch? I think it is a bug and should be fixed.

I agree there is a bug there but I don't think this is the right fix. If rdev could be NULL there, then it could also be NULL in

		md_error(mddev, conf->mirrors[r1_bio->read_disk].rdev);

in handle_read_error().
I think we should just hold on to the reference to the rdev until we are done with it, like the follow.

Would you agree?

Thanks,
NeilBrown

drivers/md/raid1.c +4 -4

index 611b5f7..eb1f8a3 100644
@@ -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)

drivers/md/raid1.c static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio) · line 2226

Quoted 7 lines

drivers/md/raid1.c +1 -0

@@ -2220,6 +2220,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);

The md_error function will check if rdev is NULL, if it is NULL, md_error will return directly, so I think it is doesn't matther if we pass a NULL rdev to md_error function.

But anyway, I can't find any problem in your patch, it is correct doubtless.