On Thu, 13 Sep 2012 14:21:05 +0800 hank <p...@redhat.com> wrote:
> On 09/13/2012 01:44 PM, NeilBrown wrote:
>
> > On Thu, 13 Sep 2012 10:28:32 +0800 hank <p...@redhat.com> wrote:
> >
> >> On 09/04/2012 11:07 AM, hank wrote:
> >>
> >>> From 0ba5879082544dc3aa13807087563b1258124b1e Mon Sep 17 00:00:00 2001
> >>> From: hank <p...@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 <p...@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);
>
>
> 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.Thanks. I've queued it for the next merge window. Thanks, NeilBrown
signature.asc
Description: PGP signature
