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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04/26/2012 03:05 AM, Stefan Hajnoczi wrote:
> On Thu, Apr 26, 2012 at 12:04 AM, Eric Blake <eblake@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>

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@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]