[libvirt] [PATCH] storage: fix device detach regression with cgroup ACLs

https://bugzilla.redhat.com/show_bug.cgi?id=876828 Commit 38c4a9cc introduced a regression in hot unplugging of disks from qemu, where cgroup device ACLs were no longer being revoked (thankfully not a security hole: cgroup ACLs only prevent open() of the disk; so reverting the ACL prevents future abuse but doesn't stop abuse from an fd that was already opened before the ACL change). The actual regression is due to a latent bug. The hot unplug code was computing the set of files to do cgroup ACL revocation based on the XML passed in by the user, rather than based on the domain's details on which disk was being deleted. As long as the revoke path was always recomputing the backing chain, this didn't really matter; but now that we want to compute the chain exactly once and remember that computation, we need to hang on to the backing chain until after the revoke has happened. * src/qemu/qemu_hotplug.c (qemuDomainDetachPciDiskDevice): Transfer backing chain before deletion. --- The patch is deceptively small for how long it took me to figure out why we had a regression :) src/qemu/qemu_hotplug.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ae8381e..0bc2259 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1999,6 +1999,8 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, virDomainDiskRemove(vm->def, i); + dev->data.disk->backingChain = detach->backingChain; + detach->backingChain = NULL; virDomainDiskDefFree(detach); if (virSecurityManagerRestoreImageLabel(driver->securityManager, -- 1.7.1

On 11/27/12 15:04, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=876828
Commit 38c4a9cc introduced a regression in hot unplugging of disks from qemu, where cgroup device ACLs were no longer being revoked (thankfully not a security hole: cgroup ACLs only prevent open() of the disk; so reverting the ACL prevents future abuse but doesn't stop abuse from an fd that was already opened before the ACL change).
The actual regression is due to a latent bug. The hot unplug code was computing the set of files to do cgroup ACL revocation based on the XML passed in by the user, rather than based on the domain's details on which disk was being deleted. As long as the revoke path was always recomputing the backing chain, this didn't really matter; but now that we want to compute the chain exactly once and remember that computation, we need to hang on to the backing chain until after the revoke has happened.
* src/qemu/qemu_hotplug.c (qemuDomainDetachPciDiskDevice): Transfer backing chain before deletion. ---
The patch is deceptively small for how long it took me to figure out why we had a regression :)
src/qemu/qemu_hotplug.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
ACK. Peter

On 11/27/12 15:04, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=876828
Commit 38c4a9cc introduced a regression in hot unplugging of disks from qemu, where cgroup device ACLs were no longer being revoked (thankfully not a security hole: cgroup ACLs only prevent open() of the disk; so reverting the ACL prevents future abuse but doesn't stop abuse from an fd that was already opened before the ACL change).
src/qemu/qemu_hotplug.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
ACK.
Thanks; pushed. [And pardon me if this web-mail interface breaks threading; I'm still recovering from a failed hard drive on the machine where I normally send mail. Thank goodness that git is distributed, so I didn't lose everything I had been working on, but it was definitely an unplanned setback...]
participants (2)
-
Eric Blake
-
Peter Krempa