lkml.org 
[lkml]   [2012]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: Subject: [PATCH 1/1] drivers/md/raid1.c: fix NULL pointer bug in fix_read_error function
On Thu, 13 Sep 2012 10:28:32 +0800 hank <pyu@redhat.com> wrote:

> On 09/04/2012 11:07 AM, hank wrote:
>
> > From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
> > From: hank <pyu@redhat.com>
> > 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 <pyu@redhat.com>
> > ---
> > drivers/md/raid1.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 611b5f7..fd8de28 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;
> > }
>
>
>
> 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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 611b5f7..eb1f8a3 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)
@@ -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);[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-09-13 08:01    [W:0.046 / U:39.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site