| Message ID | 517A6DD1.2080102@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Josh Durgin <josh.durgin@inktank.com> On 04/26/2013 05:06 AM, Alex Elder wrote: > When the snapshot context for an rbd device gets updated (or the > initial one is recorded) a a list of snapshot structures is created > to represent them, one entry per snapshot. Each entry includes a > dynamically-allocated copy of the snapshot name. > > Currently the name is allocated in rbd_snap_create(), as a duplicate > of the passed-in name. > > For format 1 images, the snapshot name provided is just a pointer to > an existing name. But for format 2 images, the passed-in name is > already dynamically allocated, and in the the process of duplicating > it here we are leaking the passed-in name. > > Fix this by dynamically allocating the name for format 1 snapshots > also, and then stop allocating a duplicate in rbd_snap_create(). > > Change rbd_dev_v1_snap_info() so none of its parameters is > side-effected unless it's going to return success. > > This is part of: > http://tracker.ceph.com/issues/4803 > > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 916741b..2b5ba50 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -3427,30 +3427,23 @@ static struct rbd_snap *rbd_snap_create(struct > rbd_device *rbd_dev, > u64 snap_features) > { > struct rbd_snap *snap; > - int ret; > > snap = kzalloc(sizeof (*snap), GFP_KERNEL); > if (!snap) > return ERR_PTR(-ENOMEM); > > - ret = -ENOMEM; > - snap->name = kstrdup(snap_name, GFP_KERNEL); > - if (!snap->name) > - goto err; > - > + snap->name = snap_name; > snap->id = snap_id; > snap->size = snap_size; > snap->features = snap_features; > > return snap; > - > -err: > - kfree(snap->name); > - kfree(snap); > - > - return ERR_PTR(ret); > } > > +/* > + * Returns a dynamically-allocated snapshot name if successful, or a > + * pointer-coded error otherwise. > + */ > static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which, > u64 *snap_size, u64 *snap_features) > { > @@ -3458,15 +3451,19 @@ static char *rbd_dev_v1_snap_info(struct > rbd_device *rbd_dev, u32 which, > > rbd_assert(which < rbd_dev->header.snapc->num_snaps); > > - *snap_size = rbd_dev->header.snap_sizes[which]; > - *snap_features = 0; /* No features for v1 */ > - > /* Skip over names until we find the one we are looking for */ > > snap_name = rbd_dev->header.snap_names; > while (which--) > snap_name += strlen(snap_name) + 1; > > + snap_name = kstrdup(snap_name, GFP_KERNEL); > + if (!snap_name); > + return ERR_PTR(-ENOMEM); > + > + *snap_size = rbd_dev->header.snap_sizes[which]; > + *snap_features = 0; /* No features for v1 */ > + > return snap_name; > } > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/29/2013 08:16 AM, Josh Durgin wrote: > Reviewed-by: Josh Durgin <josh.durgin@inktank.com> That was meant for v2 of this patch. > On 04/26/2013 05:06 AM, Alex Elder wrote: >> When the snapshot context for an rbd device gets updated (or the >> initial one is recorded) a a list of snapshot structures is created >> to represent them, one entry per snapshot. Each entry includes a >> dynamically-allocated copy of the snapshot name. >> >> Currently the name is allocated in rbd_snap_create(), as a duplicate >> of the passed-in name. >> >> For format 1 images, the snapshot name provided is just a pointer to >> an existing name. But for format 2 images, the passed-in name is >> already dynamically allocated, and in the the process of duplicating >> it here we are leaking the passed-in name. >> >> Fix this by dynamically allocating the name for format 1 snapshots >> also, and then stop allocating a duplicate in rbd_snap_create(). >> >> Change rbd_dev_v1_snap_info() so none of its parameters is >> side-effected unless it's going to return success. >> >> This is part of: >> http://tracker.ceph.com/issues/4803 >> >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" 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/drivers/block/rbd.c b/drivers/block/rbd.c index 916741b..2b5ba50 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -3427,30 +3427,23 @@ static struct rbd_snap *rbd_snap_create(struct rbd_device *rbd_dev, u64 snap_features) { struct rbd_snap *snap; - int ret; snap = kzalloc(sizeof (*snap), GFP_KERNEL); if (!snap) return ERR_PTR(-ENOMEM); - ret = -ENOMEM; - snap->name = kstrdup(snap_name, GFP_KERNEL); - if (!snap->name) - goto err; - + snap->name = snap_name; snap->id = snap_id; snap->size = snap_size; snap->features = snap_features; return snap; - -err: - kfree(snap->name); - kfree(snap); - - return ERR_PTR(ret); } +/* + * Returns a dynamically-allocated snapshot name if successful, or a + * pointer-coded error otherwise. + */ static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which, u64 *snap_size, u64 *snap_features)
When the snapshot context for an rbd device gets updated (or the initial one is recorded) a a list of snapshot structures is created to represent them, one entry per snapshot. Each entry includes a dynamically-allocated copy of the snapshot name. Currently the name is allocated in rbd_snap_create(), as a duplicate of the passed-in name. For format 1 images, the snapshot name provided is just a pointer to an existing name. But for format 2 images, the passed-in name is already dynamically allocated, and in the the process of duplicating it here we are leaking the passed-in name. Fix this by dynamically allocating the name for format 1 snapshots also, and then stop allocating a duplicate in rbd_snap_create(). Change rbd_dev_v1_snap_info() so none of its parameters is side-effected unless it's going to return success. This is part of: http://tracker.ceph.com/issues/4803 Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) { @@ -3458,15 +3451,19 @@ static char *rbd_dev_v1_snap_info(struct rbd_device *rbd_dev, u32 which, rbd_assert(which < rbd_dev->header.snapc->num_snaps); - *snap_size = rbd_dev->header.snap_sizes[which]; - *snap_features = 0; /* No features for v1 */ - /* Skip over names until we find the one we are looking for */ snap_name = rbd_dev->header.snap_names; while (which--) snap_name += strlen(snap_name) + 1; + snap_name = kstrdup(snap_name, GFP_KERNEL); + if (!snap_name); + return ERR_PTR(-ENOMEM); + + *snap_size = rbd_dev->header.snap_sizes[which]; + *snap_features = 0; /* No features for v1 */ + return snap_name; }