[libvirt] [PATCH] lock qemu_driver early in qemuGetSchedulerParametersFlags()
If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to qemuGetSchedulerParametersFlags() or *nparams is less than 1, we will unlock qemu_driver without locking it. It's very dangerous. We should lock qemu_driver after calling virCheckFlags(). --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 768e0f2..c6994cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + qemuDriverLock(driver); + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (vm == NULL) { -- 1.7.1
On 28.06.2011 09:58, Wen Congyang wrote:
If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to qemuGetSchedulerParametersFlags() or *nparams is less than 1, we will unlock qemu_driver without locking it. It's very dangerous.
We should lock qemu_driver after calling virCheckFlags().
--- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 768e0f2..c6994cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
+ qemuDriverLock(driver); + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; }
- qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (vm == NULL) {
ACK & nice catch. Michal
On 06/28/2011 04:09 AM, Michal Privoznik wrote:
On 28.06.2011 09:58, Wen Congyang wrote:
If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to qemuGetSchedulerParametersFlags() or *nparams is less than 1, we will unlock qemu_driver without locking it. It's very dangerous.
We should lock qemu_driver after calling virCheckFlags().
--- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 768e0f2..c6994cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
+ qemuDriverLock(driver); + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; }
An alternative would have been to replace the 'goto cleanup' with 'return -1'. In fact, with that alternative, we can error out without even spending time grabbing the lock. Then again, the error is unexpected (you can assume that most callers are compliant), and optimizing for the rare bad code won't have any impact to the execution speed of good code, so I'm happy with the patch as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 06/28/2011 10:50 PM, Eric Blake Write:
On 06/28/2011 04:09 AM, Michal Privoznik wrote:
On 28.06.2011 09:58, Wen Congyang wrote:
If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to qemuGetSchedulerParametersFlags() or *nparams is less than 1, we will unlock qemu_driver without locking it. It's very dangerous.
We should lock qemu_driver after calling virCheckFlags().
--- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 768e0f2..c6994cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
+ qemuDriverLock(driver); + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; }
An alternative would have been to replace the 'goto cleanup' with 'return -1'. In fact, with that alternative, we can error out without even spending time grabbing the lock. Then again, the error is unexpected (you can assume that most callers are compliant), and optimizing for the rare bad code won't have any impact to the execution speed of good code, so I'm happy with the patch as-is.
Yes, it is another way to fix this problem, and the error is unexpected. I do not modify it and pushed it. Thanks.
participants (3)
-
Eric Blake -
Michal Privoznik -
Wen Congyang