[PATCH] xen/blkback: prevent repeated backend_changed invocations

[PATCH] xen/blkback: prevent repeated backend_changed invocations

[Olaf Hering]

backend_changed might be called multiple times, which will leak
be->mode. Make sure it will be called only once. Remove some unneeded
checks. Also the be->mode string was leaked, release the memory on
device shutdown.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---

incorporate all comments from Jan.
fold the oneline change to xen_blkbk_remove into this change
now its compile tested.

 drivers/block/xen-blkback/xenbus.c | 69 ++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index f58434c..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;
@@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
 		be->blkif = NULL;
 	}
 
+	kfree(be->mode);
 	kfree(be);
 	dev_set_drvdata(&dev->dev, NULL);
 	return 0;
@@ -501,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)) {
@@ -520,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)) {
@@ -541,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

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Jan Beulich]

>>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> backend_changed might be called multiple times, which will leak
> be->mode. Make sure it will be called only once. Remove some unneeded
> checks. Also the be->mode string was leaked, release the memory on
> device shutdown.

So did I miss some discussion here? I haven't seen any
confirmation of this function indeed being supposed to be called
just once.

Also, as said previously, if indeed it is to be called just once,
removing the watch during/after the first invocation would seem
to be the more appropriate thing to do.

Jan

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Olaf Hering]

On Wed, Dec 12, Jan Beulich wrote:

> >>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So did I miss some discussion here? I haven't seen any
> confirmation of this function indeed being supposed to be called
> just once.
> 
> Also, as said previously, if indeed it is to be called just once,
> removing the watch during/after the first invocation would seem
> to be the more appropriate thing to do.

Does the API allow this, that the called function can disable the watch?

Olaf

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Jan Beulich]

>>> On 12.12.12 at 10:47, Olaf Hering <olaf@aepfle.de> wrote:
> On Wed, Dec 12, Jan Beulich wrote:
> 
>> >>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
>> > backend_changed might be called multiple times, which will leak
>> > be->mode. Make sure it will be called only once. Remove some unneeded
>> > checks. Also the be->mode string was leaked, release the memory on
>> > device shutdown.
>> 
>> So did I miss some discussion here? I haven't seen any
>> confirmation of this function indeed being supposed to be called
>> just once.
>> 
>> Also, as said previously, if indeed it is to be called just once,
>> removing the watch during/after the first invocation would seem
>> to be the more appropriate thing to do.
> 
> Does the API allow this, that the called function can disable the watch?

That is what would need looking into (and why I said "during/after").

Jan

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Ian Campbell]

On Wed, 2012-12-12 at 09:42 +0000, Jan Beulich wrote:
> >>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So did I miss some discussion here? I haven't seen any
> confirmation of this function indeed being supposed to be called
> just once.
> 
> Also, as said previously, if indeed it is to be called just once,
> removing the watch during/after the first invocation would seem
> to be the more appropriate thing to do.

The watch fires (often needlessly) when you first register it so in the
common case it is going to be called twice. Of course that first time
should abort early on so perhaps that's a moot point.

Ian.

Re: [Xen-devel] [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Olaf Hering]

On Wed, Dec 12, Ian Campbell wrote:

> On Wed, 2012-12-12 at 09:42 +0000, Jan Beulich wrote:
> > >>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> > > backend_changed might be called multiple times, which will leak
> > > be->mode. Make sure it will be called only once. Remove some unneeded
> > > checks. Also the be->mode string was leaked, release the memory on
> > > device shutdown.
> > 
> > So did I miss some discussion here? I haven't seen any
> > confirmation of this function indeed being supposed to be called
> > just once.
> > 
> > Also, as said previously, if indeed it is to be called just once,
> > removing the watch during/after the first invocation would seem
> > to be the more appropriate thing to do.
> 
> The watch fires (often needlessly) when you first register it so in the
> common case it is going to be called twice. Of course that first time
> should abort early on so perhaps that's a moot point.

The current code handles that, if a property does not exist the function
will exit early.

Olaf

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Jan Beulich]

>>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> backend_changed might be called multiple times, which will leak
> be->mode. Make sure it will be called only once. Remove some unneeded
> checks. Also the be->mode string was leaked, release the memory on
> device shutdown.

So I decided to make an attempt myself, retaining the current
behavior of allowing multiple calls, yet not having to sprinkle
around multiple kfree()-s for be->mode. Slightly re-structuring
the function made this pretty strait forward.

Jan

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> 
> incorporate all comments from Jan.
> fold the oneline change to xen_blkbk_remove into this change
> now its compile tested.
> 
>  drivers/block/xen-blkback/xenbus.c | 69 ++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index f58434c..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;
> @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>  		be->blkif = NULL;
>  	}
>  
> +	kfree(be->mode);
>  	kfree(be);
>  	dev_set_drvdata(&dev->dev, NULL);
>  	return 0;
> @@ -501,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)) {
> @@ -520,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)) {
> @@ -541,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

Re: [PATCH] xen/blkback: prevent repeated backend_changed invocations

[Konrad Rzeszutek Wilk]

On Thu, Dec 13, 2012 at 03:02:11PM +0000, Jan Beulich wrote:
> >>> On 11.12.12 at 21:50, Olaf Hering <olaf@aepfle.de> wrote:
> > backend_changed might be called multiple times, which will leak
> > be->mode. Make sure it will be called only once. Remove some unneeded
> > checks. Also the be->mode string was leaked, release the memory on
> > device shutdown.
> 
> So I decided to make an attempt myself, retaining the current
> behavior of allowing multiple calls, yet not having to sprinkle
> around multiple kfree()-s for be->mode. Slightly re-structuring
> the function made this pretty strait forward.

<nods> Would it be possible to post a patch?

> 
> Jan
> 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > ---
> > 
> > incorporate all comments from Jan.
> > fold the oneline change to xen_blkbk_remove into this change
> > now its compile tested.
> > 
> >  drivers/block/xen-blkback/xenbus.c | 69 ++++++++++++++++++--------------------
> >  1 file changed, 33 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkback/xenbus.c 
> > b/drivers/block/xen-blkback/xenbus.c
> > index f58434c..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;
> > @@ -366,6 +367,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
> >  		be->blkif = NULL;
> >  	}
> >  
> > +	kfree(be->mode);
> >  	kfree(be);
> >  	dev_set_drvdata(&dev->dev, NULL);
> >  	return 0;
> > @@ -501,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)) {
> > @@ -520,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)) {
> > @@ -541,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
> 
>