* [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;
as well as URLs for NNTP newsgroup(s).