Xen devel - Dec 2012 - [PATCH] linux-2.6.18/blkback: do not leak mode property

"be->mode" is obtained from xenbus_read(), which does a kmalloc()
for
the message body. The short string is never released, so do it along
with freeing "be" itself, and make sure the string isn''t kept
when
backend_changed() doesn''t complete successfully (which made it
desirable to slightly re-structure that function, so that the error
cleanup can be done in one place).

Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/blkback/xenbus.c
+++ b/drivers/xen/blkback/xenbus.c
@@ -198,6 +198,7 @@ static int blkback_remove(struct xenbus_
 		be->blkif = NULL;
 	}
 
+	kfree(be->mode);
 	kfree(be);
 	dev->dev.driver_data = NULL;
 	write_unlock(&sysfs_read_lock);
@@ -278,6 +279,7 @@ static void backend_changed(struct xenbu
 		= container_of(watch, struct backend_info, backend_watch);
 	struct xenbus_device *dev = be->dev;
 	int cdrom = 0;
+	unsigned long handle;
 	char *device_type;
 
 	DPRINTK("");
@@ -295,12 +297,12 @@ static void backend_changed(struct xenbu
 		return;
 	}
 
-	if ((be->major || be->minor) &&
-	    ((be->major != major) || (be->minor != minor))) {
-		printk(KERN_WARNING
-		       "blkback: changing physical device (from %x:%x to "
-		       "%x:%x) not supported.\n", be->major, be->minor,
-		       major, minor);
+	if (be->major | be->minor) {
+		if (be->major != major || be->minor != minor)
+			printk(KERN_WARNING "blkback: "
+			       "changing physical device (from %x:%x to "
+			       "%x:%x) not supported.\n",
+			       be->major, be->minor, major, minor);
 		return;
 	}
 
@@ -318,31 +320,30 @@ static void backend_changed(struct xenbu
 		kfree(device_type);
 	}
 
-	if (be->major == 0 && be->minor == 0) {
-		/* Front end dir is a number, which is used as the handle. */
+	/* Front end dir is a number, which is used as the handle. */
+	handle = simple_strtoul(strrchr(dev->otherend, ''/'') + 1,
NULL, 0);
 
-		char *p = strrchr(dev->otherend, ''/'') + 1;
-		long handle = simple_strtoul(p, NULL, 0);
+	be->major = major;
+	be->minor = minor;
 
-		be->major = major;
-		be->minor = minor;
-
-		err = vbd_create(be->blkif, handle, major, minor,
-				 (NULL == strchr(be->mode, ''w'')), cdrom);
-		if (err) {
-			be->major = be->minor = 0;
-			xenbus_dev_fatal(dev, err, "creating vbd structure");
-			return;
-		}
+	err = vbd_create(be->blkif, handle, major, minor,
+			 (NULL == strchr(be->mode, ''w'')), cdrom);
 
+	if (err)
+		xenbus_dev_fatal(dev, err, "creating vbd structure");
+	else {
 		err = xenvbd_sysfs_addif(dev);
 		if (err) {
 			vbd_free(&be->blkif->vbd);
-			be->major = be->minor = 0;
 			xenbus_dev_fatal(dev, err, "creating sysfs entries");
-			return;
 		}
+	}
 
+	if (err) {
+		kfree(be->mode);
+		be->mode = NULL;
+		be->major = be->minor = 0;
+	} else {
 		/* We''re potentially connected now */
 		update_blkif_status(be->blkif);
 	}





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Thu, Dec 13, Jan Beulich wrote:
> +	if (be->major | be->minor) {
I think a single | works as intended, but || was meant?

Olaf

>>> On 13.12.12 at 16:09, Olaf Hering <olaf@aepfle.de> wrote:
> On Thu, Dec 13, Jan Beulich wrote:
> 
>> +	if (be->major | be->minor) {
> 
> I think a single | works as intended, but || was meant?
No, I indeed meant to use | (as producing better code even with
a compiler not knowing of that transformation).

Jan