Olaf Hering
2012-Dec-05 10:01 UTC
[PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
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
Jan Beulich
2012-Dec-05 10:21 UTC
Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
>>> 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
Olaf Hering
2012-Dec-06 16:23 UTC
Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
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
Jan Beulich
2012-Dec-06 17:02 UTC
Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
>>> 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
Olaf Hering
2012-Dec-06 19:04 UTC
Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
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
Konrad Rzeszutek Wilk
2012-Dec-07 19:46 UTC
Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend_changed calls
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 > > >