| Message ID | 20130722065515.GB14617@longonot.mountain (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On mon, 22 Jul 2013 09:55:15 +0300, Dan Carpenter wrote: > The alloc_rbio() frees "raid_map" and "bbio" on error, so there is a > potential double free bug in raid56_parity_write(). The > raid56_parity_write() and raid56_parity_recover() functions should still > free "raid_map" and "bbio" on error if other errors occur though, so I > have added some more calls to kfree(). > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> From the viewpoint of the readability, it is better to free raid_map and bbio in the caller, I think. But it is up to you. Reviewed-by: Miao Xie <miaox@cn.fujitsu.com> > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 0525e13..0db856c 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -1687,11 +1687,8 @@ int raid56_parity_write(struct btrfs_root *root, struct bio *bio, > struct blk_plug_cb *cb; > > rbio = alloc_rbio(root, bbio, raid_map, stripe_len); > - if (IS_ERR(rbio)) { > - kfree(raid_map); > - kfree(bbio); > + if (IS_ERR(rbio)) > return PTR_ERR(rbio); > - } > bio_list_add(&rbio->bio_list, bio); > rbio->bio_list_bytes = bio->bi_size; > > @@ -2041,9 +2038,8 @@ int raid56_parity_recover(struct btrfs_root *root, struct bio *bio, > int ret; > > rbio = alloc_rbio(root, bbio, raid_map, stripe_len); > - if (IS_ERR(rbio)) { > + if (IS_ERR(rbio)) > return PTR_ERR(rbio); > - } > > rbio->read_rebuild = 1; > bio_list_add(&rbio->bio_list, bio); > @@ -2052,6 +2048,8 @@ int raid56_parity_recover(struct btrfs_root *root, struct bio *bio, > rbio->faila = find_logical_bio_stripe(rbio, bio); > if (rbio->faila == -1) { > BUG(); > + kfree(raid_map); > + kfree(bbio); > kfree(rbio); > return -EIO; > } > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 22, 2013 at 04:47:00PM +0800, Miao Xie wrote: > On mon, 22 Jul 2013 09:55:15 +0300, Dan Carpenter wrote: > > The alloc_rbio() frees "raid_map" and "bbio" on error, so there is a > > potential double free bug in raid56_parity_write(). The > > raid56_parity_write() and raid56_parity_recover() functions should still > > free "raid_map" and "bbio" on error if other errors occur though, so I > > have added some more calls to kfree(). > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > >From the viewpoint of the readability, it is better to free raid_map and bbio > in the caller, I think. But it is up to you. Normally, yes, you are right, but in this case the code makes sense as is. We would have to free it in btrfs_map_bio() but we actually allocate it in __btrfs_map_block(). It would be just as weird as the current code. The current code is not ugly, it's just that complicated things require complicated code. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 0525e13..0db856c 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -1687,11 +1687,8 @@ int raid56_parity_write(struct btrfs_root *root, struct bio *bio, struct blk_plug_cb *cb; rbio = alloc_rbio(root, bbio, raid_map, stripe_len); - if (IS_ERR(rbio)) { - kfree(raid_map); - kfree(bbio); + if (IS_ERR(rbio)) return PTR_ERR(rbio); - } bio_list_add(&rbio->bio_list, bio); rbio->bio_list_bytes = bio->bi_size; @@ -2041,9 +2038,8 @@ int raid56_parity_recover(struct btrfs_root *root, struct bio *bio, int ret; rbio = alloc_rbio(root, bbio, raid_map, stripe_len); - if (IS_ERR(rbio)) { + if (IS_ERR(rbio)) return PTR_ERR(rbio); - } rbio->read_rebuild = 1; bio_list_add(&rbio->bio_list, bio); @@ -2052,6 +2048,8 @@ int raid56_parity_recover(struct btrfs_root *root, struct bio *bio, rbio->faila = find_logical_bio_stripe(rbio, bio); if (rbio->faila == -1) { BUG(); + kfree(raid_map); + kfree(bbio); kfree(rbio); return -EIO; }
The alloc_rbio() frees "raid_map" and "bbio" on error, so there is a potential double free bug in raid56_parity_write(). The raid56_parity_write() and raid56_parity_recover() functions should still free "raid_map" and "bbio" on error if other errors occur though, so I have added some more calls to kfree(). Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html