[libvirt] [PATCH] blockjob: fix block-stream bandwidth race

[Eric Blake]

On 04/26/2012 03:05 AM, Stefan Hajnoczi wrote:
> On Thu, Apr 26, 2012 at 12:04 AM, Eric Blake <eblake at redhat.com> wrote:
>> With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race
>> with non-zero bandwidth: there is a window between the block_stream
>> and block_job_set_speed monitor commands where an unlimited amount
>> of data was let through, defeating the point of a throttle.
>>
>> This race was first identified in commit a9d3495e, and libvirt was
>> able to reduce the size of the window for that race.  In the meantime,
>> the qemu developers decided to fix things properly; per this message:
>> https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html
>> the fix will be in qemu 1.1, and changes block-job-set-speed to use
>> a different parameter name, as well as adding a new optional parameter
>> to block-stream, which eliminates the race altogether.
>>
>> Since our documentation already mentioned that we can refuse a non-zero
>> bandwidth for some hypervisors, I think the best solution is to do
>> just that for RHEL 6.2 qemu, so that the race is obvious to the user.
>> Meanwhile the code must be fixed to honor actual qemu 1.1 naming.
>>
>> * src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value.
>> * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject
>> bandwidth during pull with too-old qemu.
>> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Support
>> difference between RHEL 6.2 and qemu 1.1 block pull.
>> * src/libvirt.c (virDomainBlockPull, virDomainBlockRebase):
>> Document this.
>> ---
>>  src/libvirt.c                |    6 +++-
>>  src/qemu/qemu_driver.c       |    8 ++++--
>>  src/qemu/qemu_monitor.h      |    3 +-
>>  src/qemu/qemu_monitor_json.c |   53 +++++++++++++++++++++++------------------
>>  4 files changed, 40 insertions(+), 30 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha at linux.vnet.ibm.com>

Thanks.  However, I found a bug:

>                          unsigned long bandwidth,
...
> +    bandwidth *= 1024ULL * 1024ULL; /* Scale MiB to bytes */

This can overflow, and silently wrap around.

v2 coming up.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120426/320cc25a/attachment-0001.sig>