* [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls @ 2012-12-05 10:01 Olaf Hering 2012-12-05 10:21 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Olaf Hering @ 2012-12-05 10:01 UTC (permalink / raw) To: konrad.wilk; +Cc: linux-kernel, xen-devel, jbeulich, Olaf Hering backend_changed might be called multiple times, which will leak be->mode. free the previous value before storing the current mode value. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- !! Not compile tested !! drivers/block/xen-blkback/xenbus.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index a6585a4..8650b19 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -502,7 +502,7 @@ static void backend_changed(struct xenbus_watch *watch, = container_of(watch, struct backend_info, backend_watch); struct xenbus_device *dev = be->dev; int cdrom = 0; - char *device_type; + char *mode, *device_type; DPRINTK(""); @@ -528,13 +528,15 @@ static void backend_changed(struct xenbus_watch *watch, return; } - be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL); - if (IS_ERR(be->mode)) { - err = PTR_ERR(be->mode); + mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL); + kfree(be->mode); + if (IS_ERR(mode)) { + err = PTR_ERR(mode); be->mode = NULL; xenbus_dev_fatal(dev, err, "reading mode"); return; } + be->mode = mode; device_type = xenbus_read(XBT_NIL, dev->otherend, "device-type", NULL); if (!IS_ERR(device_type)) { @@ -555,7 +557,7 @@ static void backend_changed(struct xenbus_watch *watch, be->minor = minor; err = xen_vbd_create(be->blkif, handle, major, minor, - (NULL == strchr(be->mode, 'w')), cdrom); + (NULL == strchr(mode, 'w')), cdrom); if (err) { be->major = 0; be->minor = 0; -- 1.8.0.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls 2012-12-05 10:01 [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls Olaf Hering @ 2012-12-05 10:21 ` Jan Beulich 2012-12-06 16:23 ` Olaf Hering 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2012-12-05 10:21 UTC (permalink / raw) To: Olaf Hering, konrad.wilk; +Cc: xen-devel, linux-kernel >>> On 05.12.12 at 11:01, Olaf Hering <olaf@aepfle.de> wrote: > backend_changed might be called multiple times, which will leak > be->mode. free the previous value before storing the current mode value. As said before - this is one possible route to take. But did you consider at all the alternative of preventing the function from getting called more than once for a given device? As also said before, I think that would have other bad effects, and hence should be preferred (and would likely also result in a smaller patch). And _if_ this here is the route to go, I'd clearly see this and your earlier patch to be folded into just one (dealing with both leaks in one go). Jan > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > > !! Not compile tested !! > > drivers/block/xen-blkback/xenbus.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/xen-blkback/xenbus.c > b/drivers/block/xen-blkback/xenbus.c > index a6585a4..8650b19 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -502,7 +502,7 @@ static void backend_changed(struct xenbus_watch *watch, > = container_of(watch, struct backend_info, backend_watch); > struct xenbus_device *dev = be->dev; > int cdrom = 0; > - char *device_type; > + char *mode, *device_type; > > DPRINTK(""); > > @@ -528,13 +528,15 @@ static void backend_changed(struct xenbus_watch *watch, > return; > } > > - be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL); > - if (IS_ERR(be->mode)) { > - err = PTR_ERR(be->mode); > + mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL); > + kfree(be->mode); > + if (IS_ERR(mode)) { > + err = PTR_ERR(mode); > be->mode = NULL; > xenbus_dev_fatal(dev, err, "reading mode"); > return; > } > + be->mode = mode; > > device_type = xenbus_read(XBT_NIL, dev->otherend, "device-type", NULL); > if (!IS_ERR(device_type)) { > @@ -555,7 +557,7 @@ static void backend_changed(struct xenbus_watch *watch, > be->minor = minor; > > err = xen_vbd_create(be->blkif, handle, major, minor, > - (NULL == strchr(be->mode, 'w')), cdrom); > + (NULL == strchr(mode, 'w')), cdrom); > if (err) { > be->major = 0; > be->minor = 0; > -- > 1.8.0.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls 2012-12-05 10:21 ` Jan Beulich @ 2012-12-06 16:23 ` Olaf Hering 2012-12-06 17:02 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Olaf Hering @ 2012-12-06 16:23 UTC (permalink / raw) To: Jan Beulich; +Cc: konrad.wilk, xen-devel, linux-kernel On Wed, Dec 05, Jan Beulich wrote: > >>> On 05.12.12 at 11:01, Olaf Hering <olaf@aepfle.de> wrote: > > backend_changed might be called multiple times, which will leak > > be->mode. free the previous value before storing the current mode value. > > As said before - this is one possible route to take. But did you > consider at all the alternative of preventing the function from > getting called more than once for a given device? As also said > before, I think that would have other bad effects, and hence > should be preferred (and would likely also result in a smaller > patch). Maybe it could be done like this, adding a flag to the backend device and exit early if its called twice. Olaf diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index a6585a4..2822e73 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -28,6 +28,7 @@ struct backend_info { unsigned major; unsigned minor; char *mode; + unsigned alive; }; static struct kmem_cache *xen_blkif_cachep; @@ -506,6 +507,9 @@ static void backend_changed(struct xenbus_watch *watch, DPRINTK(""); + if (be->alive) + return; + err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x", &major, &minor); if (XENBUS_EXIST_ERR(err)) { @@ -548,8 +552,11 @@ static void backend_changed(struct xenbus_watch *watch, char *p = strrchr(dev->otherend, '/') + 1; long handle; err = strict_strtoul(p, 0, &handle); - if (err) + if (err) { + kfree(be->mode); + be->mode = NULL; return; + } be->major = major; be->minor = minor; @@ -560,6 +567,8 @@ static void backend_changed(struct xenbus_watch *watch, be->major = 0; be->minor = 0; xenbus_dev_fatal(dev, err, "creating vbd structure"); + kfree(be->mode); + be->mode = NULL; return; } @@ -569,10 +578,13 @@ static void backend_changed(struct xenbus_watch *watch, be->major = 0; be->minor = 0; xenbus_dev_fatal(dev, err, "creating sysfs entries"); + kfree(be->mode); + be->mode = NULL; return; } /* We're potentially connected now */ + be->alive = 1; xen_update_blkif_status(be->blkif); } } -- 1.8.0.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls 2012-12-06 16:23 ` Olaf Hering @ 2012-12-06 17:02 ` Jan Beulich 2012-12-06 19:04 ` Olaf Hering 2012-12-07 19:46 ` Konrad Rzeszutek Wilk 0 siblings, 2 replies; 6+ messages in thread From: Jan Beulich @ 2012-12-06 17:02 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel, konrad.wilk, linux-kernel >>> On 06.12.12 at 17:23, Olaf Hering <olaf@aepfle.de> wrote: > On Wed, Dec 05, Jan Beulich wrote: > >> >>> On 05.12.12 at 11:01, Olaf Hering <olaf@aepfle.de> wrote: >> > backend_changed might be called multiple times, which will leak >> > be->mode. free the previous value before storing the current mode value. >> >> As said before - this is one possible route to take. But did you >> consider at all the alternative of preventing the function from >> getting called more than once for a given device? As also said >> before, I think that would have other bad effects, and hence >> should be preferred (and would likely also result in a smaller >> patch). > > Maybe it could be done like this, adding a flag to the backend device > and exit early if its called twice. Maybe, but it looks odd to me. But then again I had hoped Konrad would have an opinion here... Also I don't see why you need to free be->mode now on all error paths - afaict it would still get freed when "be" gets freed (with your earlier patch). Jan > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -28,6 +28,7 @@ struct backend_info { > unsigned major; > unsigned minor; > char *mode; > + unsigned alive; > }; > > static struct kmem_cache *xen_blkif_cachep; > @@ -506,6 +507,9 @@ static void backend_changed(struct xenbus_watch *watch, > > DPRINTK(""); > > + if (be->alive) > + return; > + > err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x", > &major, &minor); > if (XENBUS_EXIST_ERR(err)) { > @@ -548,8 +552,11 @@ static void backend_changed(struct xenbus_watch *watch, > char *p = strrchr(dev->otherend, '/') + 1; > long handle; > err = strict_strtoul(p, 0, &handle); > - if (err) > + if (err) { > + kfree(be->mode); > + be->mode = NULL; > return; > + } > > be->major = major; > be->minor = minor; > @@ -560,6 +567,8 @@ static void backend_changed(struct xenbus_watch *watch, > be->major = 0; > be->minor = 0; > xenbus_dev_fatal(dev, err, "creating vbd structure"); > + kfree(be->mode); > + be->mode = NULL; > return; > } > > @@ -569,10 +578,13 @@ static void backend_changed(struct xenbus_watch > *watch, > be->major = 0; > be->minor = 0; > xenbus_dev_fatal(dev, err, "creating sysfs entries"); > + kfree(be->mode); > + be->mode = NULL; > return; > } > > /* We're potentially connected now */ > + be->alive = 1; > xen_update_blkif_status(be->blkif); > } > } > -- > 1.8.0.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls 2012-12-06 17:02 ` Jan Beulich @ 2012-12-06 19:04 ` Olaf Hering 2012-12-07 19:46 ` Konrad Rzeszutek Wilk 1 sibling, 0 replies; 6+ messages in thread From: Olaf Hering @ 2012-12-06 19:04 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, konrad.wilk, linux-kernel On Thu, Dec 06, Jan Beulich wrote: > >>> On 06.12.12 at 17:23, Olaf Hering <olaf@aepfle.de> wrote: > > On Wed, Dec 05, Jan Beulich wrote: > > > >> >>> On 05.12.12 at 11:01, Olaf Hering <olaf@aepfle.de> wrote: > >> > backend_changed might be called multiple times, which will leak > >> > be->mode. free the previous value before storing the current mode value. > >> > >> As said before - this is one possible route to take. But did you > >> consider at all the alternative of preventing the function from > >> getting called more than once for a given device? As also said > >> before, I think that would have other bad effects, and hence > >> should be preferred (and would likely also result in a smaller > >> patch). > > > > Maybe it could be done like this, adding a flag to the backend device > > and exit early if its called twice. > > Maybe, but it looks odd to me. But then again I had hoped Konrad > would have an opinion here... Looking at this some more, if backend_changed is supposed to be called exactly once then major/minor checks can be removed because they will be always zero, like this: drivers/block/xen-blkback/xenbus.c | 68 ++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index a6585a4..5ca77c3 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -28,6 +28,7 @@ struct backend_info { unsigned major; unsigned minor; char *mode; + unsigned alive; }; static struct kmem_cache *xen_blkif_cachep; @@ -502,10 +503,14 @@ static void backend_changed(struct xenbus_watch *watch, = container_of(watch, struct backend_info, backend_watch); struct xenbus_device *dev = be->dev; int cdrom = 0; - char *device_type; + char *device_type, *p; + long handle; DPRINTK(""); + if (be->alive) + return; + err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x", &major, &minor); if (XENBUS_EXIST_ERR(err)) { @@ -521,12 +526,7 @@ static void backend_changed(struct xenbus_watch *watch, return; } - if ((be->major || be->minor) && - ((be->major != major) || (be->minor != minor))) { - pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) not supported.\n", - be->major, be->minor, major, minor); - return; - } + be->alive = 1; be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL); if (IS_ERR(be->mode)) { @@ -542,39 +542,35 @@ static void backend_changed(struct xenbus_watch *watch, kfree(device_type); } - if (be->major == 0 && be->minor == 0) { - /* Front end dir is a number, which is used as the handle. */ - - char *p = strrchr(dev->otherend, '/') + 1; - long handle; - err = strict_strtoul(p, 0, &handle); - if (err) - return; - - be->major = major; - be->minor = minor; + /* Front end dir is a number, which is used as the handle. */ + p = strrchr(dev->otherend, '/') + 1; + err = strict_strtoul(p, 0, &handle); + if (err) + return; - err = xen_vbd_create(be->blkif, handle, major, minor, - (NULL == strchr(be->mode, 'w')), cdrom); - if (err) { - be->major = 0; - be->minor = 0; - xenbus_dev_fatal(dev, err, "creating vbd structure"); - return; - } + be->major = major; + be->minor = minor; - err = xenvbd_sysfs_addif(dev); - if (err) { - xen_vbd_free(&be->blkif->vbd); - be->major = 0; - be->minor = 0; - xenbus_dev_fatal(dev, err, "creating sysfs entries"); - return; - } + err = xen_vbd_create(be->blkif, handle, major, minor, + (NULL == strchr(be->mode, 'w')), cdrom); + if (err) { + be->major = 0; + be->minor = 0; + xenbus_dev_fatal(dev, err, "creating vbd structure"); + return; + } - /* We're potentially connected now */ - xen_update_blkif_status(be->blkif); + err = xenvbd_sysfs_addif(dev); + if (err) { + xen_vbd_free(&be->blkif->vbd); + be->major = 0; + be->minor = 0; + xenbus_dev_fatal(dev, err, "creating sysfs entries"); + return; } + + /* We're potentially connected now */ + xen_update_blkif_status(be->blkif); } -- 1.8.0.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls 2012-12-06 17:02 ` Jan Beulich 2012-12-06 19:04 ` Olaf Hering @ 2012-12-07 19:46 ` Konrad Rzeszutek Wilk 1 sibling, 0 replies; 6+ messages in thread From: Konrad Rzeszutek Wilk @ 2012-12-07 19:46 UTC (permalink / raw) To: Jan Beulich; +Cc: Olaf Hering, xen-devel, linux-kernel On Thu, Dec 06, 2012 at 05:02:50PM +0000, Jan Beulich wrote: > >>> On 06.12.12 at 17:23, Olaf Hering <olaf@aepfle.de> wrote: > > On Wed, Dec 05, Jan Beulich wrote: > > > >> >>> On 05.12.12 at 11:01, Olaf Hering <olaf@aepfle.de> wrote: > >> > backend_changed might be called multiple times, which will leak > >> > be->mode. free the previous value before storing the current mode value. > >> > >> As said before - this is one possible route to take. But did you > >> consider at all the alternative of preventing the function from > >> getting called more than once for a given device? As also said > >> before, I think that would have other bad effects, and hence > >> should be preferred (and would likely also result in a smaller > >> patch). > > > > Maybe it could be done like this, adding a flag to the backend device > > and exit early if its called twice. > > Maybe, but it looks odd to me. But then again I had hoped Konrad > would have an opinion here... Sorry - was lurking around and hadn't paid any attention to this thread. And it does not help that next week I am out :-) > > Also I don't see why you need to free be->mode now on all error > paths - afaict it would still get freed when "be" gets freed (with > your earlier patch). > > Jan > > > --- a/drivers/block/xen-blkback/xenbus.c > > +++ b/drivers/block/xen-blkback/xenbus.c > > @@ -28,6 +28,7 @@ struct backend_info { > > unsigned major; > > unsigned minor; > > char *mode; > > + unsigned alive; > > }; > > > > static struct kmem_cache *xen_blkif_cachep; > > @@ -506,6 +507,9 @@ static void backend_changed(struct xenbus_watch *watch, > > > > DPRINTK(""); > > > > + if (be->alive) > > + return; > > + > > err = xenbus_scanf(XBT_NIL, dev->nodename, "physical-device", "%x:%x", > > &major, &minor); > > if (XENBUS_EXIST_ERR(err)) { > > @@ -548,8 +552,11 @@ static void backend_changed(struct xenbus_watch *watch, > > char *p = strrchr(dev->otherend, '/') + 1; > > long handle; > > err = strict_strtoul(p, 0, &handle); > > - if (err) > > + if (err) { > > + kfree(be->mode); > > + be->mode = NULL; > > return; > > + } > > > > be->major = major; > > be->minor = minor; > > @@ -560,6 +567,8 @@ static void backend_changed(struct xenbus_watch *watch, > > be->major = 0; > > be->minor = 0; > > xenbus_dev_fatal(dev, err, "creating vbd structure"); > > + kfree(be->mode); > > + be->mode = NULL; > > return; > > } > > > > @@ -569,10 +578,13 @@ static void backend_changed(struct xenbus_watch > > *watch, > > be->major = 0; > > be->minor = 0; > > xenbus_dev_fatal(dev, err, "creating sysfs entries"); > > + kfree(be->mode); > > + be->mode = NULL; > > return; > > } > > > > /* We're potentially connected now */ > > + be->alive = 1; > > xen_update_blkif_status(be->blkif); > > } > > } > > -- > > 1.8.0.1 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-07 19:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-05 10:01 [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls Olaf Hering 2012-12-05 10:21 ` Jan Beulich 2012-12-06 16:23 ` Olaf Hering 2012-12-06 17:02 ` Jan Beulich 2012-12-06 19:04 ` Olaf Hering 2012-12-07 19:46 ` Konrad Rzeszutek Wilk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox