lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 25 Nov 2015 12:40:34 -0500
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	Bob Liu <bob.liu@...cle.com>
Cc:	xen-devel@...ts.xen.org, linux-kernel@...r.kernel.org,
	roger.pau@...rix.com, felipe.franciosi@...rix.com, axboe@...com,
	avanzini.arianna@...il.com, rafal.mielniczuk@...rix.com,
	jonathan.davies@...rix.com, david.vrabel@...rix.com
Subject: Re: [PATCH v5 07/10] xen/blkback: pseudo support for multi hardware
 queues/rings

> @@ -113,19 +115,55 @@ static void xen_update_blkif_status(struct xen_blkif *blkif)
>  	}
>  	invalidate_inode_pages2(blkif->vbd.bdev->bd_inode->i_mapping);
>  
> -	blkif->ring.xenblkd = kthread_run(xen_blkif_schedule, &blkif->ring, "%s", name);
> -	if (IS_ERR(blkif->ring.xenblkd)) {
> -		err = PTR_ERR(blkif->ring.xenblkd);
> -		blkif->ring.xenblkd = NULL;
> -		xenbus_dev_error(blkif->be->dev, err, "start xenblkd");
> -		return;
> +	for (i = 0; i < blkif->nr_rings; i++) {
> +		ring = &blkif->rings[i];
> +		ring->xenblkd = kthread_run(xen_blkif_schedule, ring, "%s-%d", name, i);
> +		if (IS_ERR(ring->xenblkd)) {
> +			err = PTR_ERR(ring->xenblkd);
> +			ring->xenblkd = NULL;
> +			xenbus_dev_fatal(blkif->be->dev, err,
> +					"start %s-%d xenblkd", name, i);
> +			goto out;
> +		}
> +	}
> +	return;
> +
> +out:
> +	while (--i >= 0) {
> +		ring = &blkif->rings[i];
> +		kthread_stop(ring->xenblkd);

That won't work. Imagine us failing at the start of the loop above, 
so i==0. We get here and decrement and unsigned int by one, and loop
back to 0xffffffffff. Naturally 0xffff.. >= 0 so we will just continue
one going over the blkif->rings[0xffffff].. and BOOM!

This worked when 'i' was 'int', but now it is unsigned int.

Let me make it 'int' and then this works, or we can swap
the loop around and use 'i-1' to use the previous entry.

[Fixed it up in my tree]
>  	}
> +	return;
> +}
> +
.. snip..
> +static int connect_ring(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	unsigned int pers_grants;
> +	char protocol[64] = "";
> +	int err, i;
> +	char *xspath;
> +	size_t xspathsize;
> +	const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-NNN" */
> +
> +	pr_debug("%s %s\n", __func__, dev->otherend);
> +
> +	be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
> +	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
> +			    "%63s", protocol, NULL);
> +	if (err)
> +		strcpy(protocol, "unspecified, assuming default");
> +	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
> +		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
> +	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
> +		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
> +	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
> +		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
> +	else {
> +		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
> +		return -1;
> +	}
> +	err = xenbus_gather(XBT_NIL, dev->otherend,
> +			    "feature-persistent", "%u",
> +			    &pers_grants, NULL);
> +	if (err)
> +		pers_grants = 0;
> +
> +	be->blkif->vbd.feature_gnt_persistent = pers_grants;
> +	be->blkif->vbd.overflow_max_grants = 0;
> +
> +	pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
> +		 be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
> +		 pers_grants ? "persistent grants" : "");
> +
> +	if (be->blkif->nr_rings == 1)
> +		return read_per_ring_refs(&be->blkif->rings[0], dev->otherend);
> +	else {
> +		xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
> +		xspath = kmalloc(xspathsize, GFP_KERNEL);
> +		if (!xspath) {
> +			xenbus_dev_fatal(dev, -ENOMEM, "reading ring references");
> +			return -ENOMEM;
> +		}
> +
> +		for (i = 0; i < be->blkif->nr_rings; i++) {
> +			memset(xspath, 0, xspathsize);
> +			snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend, i);
> +			err = read_per_ring_refs(&be->blkif->rings[i], xspath);

Say nr_rings is 4 and this fails at the last one. That means
be->blkif->rings[0..2].pending_free has a bunch of pages and
also ring->blk_ring are set. We return out of this function
and end back in (frontend_changed):
 752                 err = connect_ring(be);
 753                 if (err)
 754                         break;

Great. So we have a memory leak until the device goes in
XenbusStateConnected (where we end up calling xen_blkif_disconnect
and free ring[0..2]..

But that may take a while if the guest is not nice. Perhaps we should
add in  frontend_changed(..) an call to xen_blkif_disconnect in case
we fail at 'connect_ring' to clear the memory faster. I will prep a
patch for that.

> +			if (err) {
> +				kfree(xspath);
> +				return err;
> +			}
> +		}
> +		kfree(xspath);
> +	}
> +	return 0;
>  }
>  
>  static const struct xenbus_device_id xen_blkbk_ids[] = {
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ