| Message ID | 20140401113851.GA15317@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 01, 2014 at 12:38:51PM +0100, Russell King - ARM Linux wrote: > Consider what happens when bio_alloc_pages() fails. j starts off as one > for non-recovery operations, and we enter the loop to allocate the pages. > j is post-decremented to zero. So, bio = r1_bio->bios[0]. > > bio_alloc_pages(bio) fails, we jump to out_free_bio. The first thing > that does is increment j, so we free from r1_bio->bios[1] up to the > number of raid disks, leaving r1_bio->bios[0] leaked as the r1_bio is > then freed. Neil, Can you please review commit a07876064a0b7 (block: Add bio_alloc_pages) which seems to have introduced this bug - it seems to have gone in during the v3.10 merge window, and looks like it was never reviewed from the attributations on the commit. The commit message is brief, and inadequately describes the functional change that the patch has - we go from "get up to RESYNC_PAGES into the bio's io_vec" to "get all RESYNC_PAGES or fail completely". Not withstanding the breakage of the error cleanup paths, is this an acceptable change of behaviour here? Thanks.
On Tue, Apr 01, 2014 at 12:38:51PM +0100, Russell King - ARM Linux wrote: > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index aacf6bf352d8..604bad2fa442 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -123,8 +123,14 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) > bio = r1_bio->bios[j]; > bio->bi_vcnt = RESYNC_PAGES; > > - if (bio_alloc_pages(bio, gfp_flags)) > - goto out_free_bio; > + if (bio_alloc_pages(bio, gfp_flags)) { > + /* > + * Mark this as having no pages - bio_alloc_pages > + * removes any it allocated. > + */ > + bio->bi_vcnt = 0; > + goto out_free_all_bios; > + } > } > /* If not user-requests, copy the page pointers to all bios */ > if (!test_bit(MD_RECOVERY_REQUESTED, &pi->mddev->recovery)) { > @@ -138,9 +144,25 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) > > return r1_bio; > > +out_free_all_bios: > + j = -1; > out_free_bio: > - while (++j < pi->raid_disks) > - bio_put(r1_bio->bios[j]); > + while (++j < pi->raid_disks) { > + bio = r1_bio->bios[j]; > + if (bio->bi_vcnt) { > + struct bio_vec *bv; > + int i; > + /* > + * Annoyingly, BIO has no way to do this, so we have > + * to do it manually. Given the trouble here, and > + * the lack of BIO support for cleaning up, I don't > + * care about linux/bio.h's comment about this helper. > + */ > + bio_for_each_segment_all(bv, bio, i) > + __free_page(bv->bv_page); > + } Do you still need the 'if' block here? bio_for_each_segment_all() checks for bio->bi_vcnt which was set to 0 above.
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index aacf6bf352d8..604bad2fa442 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -123,8 +123,14 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) bio = r1_bio->bios[j]; bio->bi_vcnt = RESYNC_PAGES; - if (bio_alloc_pages(bio, gfp_flags)) - goto out_free_bio; + if (bio_alloc_pages(bio, gfp_flags)) { + /* + * Mark this as having no pages - bio_alloc_pages + * removes any it allocated. + */ + bio->bi_vcnt = 0; + goto out_free_all_bios; + } } /* If not user-requests, copy the page pointers to all bios */ if (!test_bit(MD_RECOVERY_REQUESTED, &pi->mddev->recovery)) { @@ -138,9 +144,25 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) return r1_bio; +out_free_all_bios: + j = -1; out_free_bio: - while (++j < pi->raid_disks) - bio_put(r1_bio->bios[j]); + while (++j < pi->raid_disks) { + bio = r1_bio->bios[j]; + if (bio->bi_vcnt) { + struct bio_vec *bv; + int i; + /* + * Annoyingly, BIO has no way to do this, so we have + * to do it manually. Given the trouble here, and + * the lack of BIO support for cleaning up, I don't + * care about linux/bio.h's comment about this helper. + */ + bio_for_each_segment_all(bv, bio, i) + __free_page(bv->bv_page); + } + bio_put(bio); + } r1bio_pool_free(r1_bio, data); return NULL; }