[libvirt] [PATCHv2 0/8] another round of audit improvements
This supercedes the unreviewed v1: https://www.redhat.com/archives/libvir-list/2011-March/msg00257.html The more I kept working on this, the more things I found that needed fixing. As it is, it's too late for me tonight, so patch 8 is currently untested, and patch 9/8 is needed to audit the hotplug devices used in 'virsh attach-interface'. But earlier patches are in good shape, so I want to start the review. Perhaps this series should be re-arranged a bit; let me know if you want the final version to see patches in any different order (although due to some of my renames, it will take me longer to do rebasing that shuffles patch order). Eric Blake (8): audit: tweak audit messages to match conventions audit: split cgroup audit types to allow more information audit: also audit cgroup controller path audit: audit use of /dev/vhost-net audit: rename remaining qemu audit functions cgroup: allow fine-tuning of device ACL permissions audit: also audit cgroup ACL permissions qemu: support vhost in attach-interface src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 9 +- src/qemu/qemu_audit.c | 263 ++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_audit.h | 83 +++++++++------ src/qemu/qemu_cgroup.c | 57 +++++----- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_command.h | 5 + src/qemu/qemu_driver.c | 44 ++++---- src/qemu/qemu_hotplug.c | 94 ++++++++++++----- src/qemu/qemu_migration.c | 14 ++-- src/qemu/qemu_process.c | 6 +- src/util/cgroup.c | 63 ++++++++---- src/util/cgroup.h | 31 +++++- 13 files changed, 466 insertions(+), 214 deletions(-) -- 1.7.4
* src/qemu/qemu_audit.c (qemuDomainHostdevAudit): Avoid use of "type", which has a pre-defined meaning. (qemuDomainCgroupAudit): Likewise, as well as "item". --- v2: no real change src/qemu/qemu_audit.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index b1948c8..0f954c0 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -159,7 +159,7 @@ qemuDomainHostdevAudit(virDomainObjPtr vm, } VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=dev reason=%s %s uuid=%s type=%s %s", + "resrc=dev reason=%s %s uuid=%s bus=%s %s", reason, vmname, uuidstr, virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type), device); @@ -200,14 +200,14 @@ void qemuDomainCgroupAudit(virDomainObjPtr vm, return; } if (name && - !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "type", + !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "category", name))) { VIR_WARN0("OOM while encoding audit message"); goto cleanup; } VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s item=%s%s%s", + "resrc=cgroup reason=%s %s uuid=%s class=%s%s%s", reason, vmname, uuidstr, item, detail ? " " : "", detail ? detail : ""); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:43PM -0700, Eric Blake wrote:
* src/qemu/qemu_audit.c (qemuDomainHostdevAudit): Avoid use of "type", which has a pre-defined meaning. (qemuDomainCgroupAudit): Likewise, as well as "item". ---
v2: no real change
src/qemu/qemu_audit.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Device names can be manipulated, so it is better to also log the major/minor device number corresponding to the cgroup ACL changes that libvirt made. This required some refactoring of the relatively new qemu cgroup audit code. Also, qemuSetupChardevCgroup was only auditing on failure, not success. * src/qemu/qemu_audit.h (qemuDomainCgroupAudit): Delete. (qemuAuditCgroup, qemuAuditCgroupMajor, qemuAuditCgroupPath): New prototypes. * src/qemu/qemu_audit.c (qemuDomainCgroupAudit): Rename... (qemuAuditCgroup): ...and drop a parameter. (qemuAuditCgroupMajor, qemuAuditCgroupPath): New functions, to allow listing device major/minor in audit. (qemuAuditGetRdev): New helper function. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust callers. * src/qemu/qemu_cgroup.c (qemuSetupDiskPathAllow) (qemuSetupHostUsbDeviceCgroup, qemuSetupCgroup) (qemuTeardownDiskPathDeny): Likewise. (qemuSetupChardevCgroup): Likewise, fixing missing audit. --- v2: fix a logic bug I noticed, choose nicer function names src/qemu/qemu_audit.c | 126 ++++++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_audit.h | 22 ++++++-- src/qemu/qemu_cgroup.c | 29 ++++------- src/qemu/qemu_driver.c | 10 +--- 4 files changed, 135 insertions(+), 52 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 0f954c0..56b0b74 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -23,12 +23,43 @@ #include <config.h> +#include <sys/stat.h> +#include <sys/types.h> + #include "qemu_audit.h" #include "virtaudit.h" #include "uuid.h" #include "logging.h" #include "memory.h" +/* Return rdev=nn:mm in hex for block and character devices, rdev=? + * for other file types or stat failure, or NULL on allocation + * failure. */ +#if defined major && defined minor +static char * +qemuAuditGetRdev(const char *path) +{ + char *ret; + struct stat sb; + + if (stat(path, &sb) == 0 && + (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode))) { + int maj = major(sb.st_rdev); + int min = minor(sb.st_rdev); + virAsprintf(&ret, "rdev=%02X:%02X", maj, min); + } else { + ret = strdup("rdev=?"); + } + return ret; +} +#else +static char * +qemuAuditGetRdev(const char *path ATTRIBUTE_UNUSED) +{ + return strdup("rdev=?"); +} +#endif + void qemuDomainDiskAudit(virDomainObjPtr vm, virDomainDiskDefPtr oldDef, virDomainDiskDefPtr newDef, @@ -106,7 +137,7 @@ void qemuDomainNetAudit(virDomainObjPtr vm, * qemuDomainHostdevAudit: * @vm: domain making a change in pass-through host device * @hostdev: device being attached or removed - * @reason: one of "start, "attach", or "detach" + * @reason: one of "start", "attach", or "detach" * @success: true if the device passthrough operation succeeded * * Log an audit message about an attempted device passthrough change. @@ -172,51 +203,104 @@ cleanup: /** - * qemuDomainCgroupAudit: + * qemuAuditCgroup: * @vm: domain making the cgroups ACL change * @cgroup: cgroup that manages the devices * @reason: either "allow" or "deny" - * @item: one of "all", "path", or "major" - * @name: NULL for @item of "all", device path for @item of "path", and - * string describing major device type for @item of "major" + * @extra: additional details, in the form "all", + * "major category=xyz maj=nn", or "path path=xyz dev=nn:mm" (the + * latter two are generated by qemuAuditCgroupMajor and + * qemuAuditCgroupPath). * @success: true if the cgroup operation succeeded * * Log an audit message about an attempted cgroup device ACL change. */ -void qemuDomainCgroupAudit(virDomainObjPtr vm, - virCgroupPtr cgroup ATTRIBUTE_UNUSED, - const char *reason, - const char *item, - const char *name, - bool success) +void +qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, + const char *reason, const char *extra, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; - char *detail = NULL; virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { VIR_WARN0("OOM while encoding audit message"); return; } - if (name && - !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "category", - name))) { + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=cgroup reason=%s %s uuid=%s class=%s", + reason, vmname, uuidstr, extra); + + VIR_FREE(vmname); +} + +/** + * qemuAuditCgroupMajor: + * @vm: domain making the cgroups ACL change + * @cgroup: cgroup that manages the devices + * @reason: either "allow" or "deny" + * @maj: the major number of the device category + * @name: a textual name for that device category, alphabetic only + * @success: true if the cgroup operation succeeded + * + * Log an audit message about an attempted cgroup device ACL change. + */ +void +qemuAuditCgroupMajor(virDomainObjPtr vm, virCgroupPtr cgroup, + const char *reason, int maj, const char *name, + bool success) +{ + char *extra; + + if (virAsprintf(&extra, "major category=%s maj=%02X", name, maj) < 0) { + VIR_WARN0("OOM while encoding audit message"); + return; + } + + qemuAuditCgroup(vm, cgroup, reason, extra, success); + + VIR_FREE(extra); +} + +/** + * qemuAuditCgroupPath: + * @vm: domain making the cgroups ACL change + * @cgroup: cgroup that manages the devices + * @reason: either "allow" or "deny" + * @path: the device being adjusted + * @rc: > 0 if not a device, 0 if success, < 0 if failure + * + * Log an audit message about an attempted cgroup device ACL change to + * a specific device. + */ +void +qemuAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr cgroup, + const char *reason, const char *path, int rc) +{ + char *detail; + char *rdev; + char *extra; + + /* Nothing to audit for regular files. */ + if (rc > 0) + return; + + if (!(detail = virAuditEncode("path", path)) || + !(rdev = qemuAuditGetRdev(path)) || + virAsprintf(&extra, "path path=%s %s", path, rdev) < 0) { VIR_WARN0("OOM while encoding audit message"); goto cleanup; } - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s%s%s", - reason, vmname, uuidstr, - item, detail ? " " : "", detail ? detail : ""); + qemuAuditCgroup(vm, cgroup, reason, extra, rc == 0); cleanup: - VIR_FREE(vmname); + VIR_FREE(extra); VIR_FREE(detail); + VIR_FREE(rdev); } - /** * qemuDomainResourceAudit: * @vm: domain making an integer resource change diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 247edde..53855e2 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -43,12 +43,22 @@ void qemuDomainHostdevAudit(virDomainObjPtr vm, virDomainHostdevDefPtr def, const char *reason, bool success); -void qemuDomainCgroupAudit(virDomainObjPtr vm, - virCgroupPtr group, - const char *reason, - const char *item, - const char *name, - bool success); +void qemuAuditCgroup(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + const char *extra, + bool success); +void qemuAuditCgroupMajor(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + int maj, + const char *name, + bool success); +void qemuAuditCgroupPath(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + const char *path, + int rc); void qemuDomainMemoryAudit(virDomainObjPtr vm, unsigned long long oldmem, unsigned long long newmem, diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e71d3fa..ebf9ad5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -67,9 +67,7 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ rc = virCgroupAllowDevicePath(data->cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", path, - rc == 0); + qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -110,9 +108,7 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ rc = virCgroupDenyDevicePath(data->cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "deny", "path", path, - rc == 0); + qemuAuditCgroupPath(data->vm, data->cgroup, "deny", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -155,9 +151,8 @@ qemuSetupChardevCgroup(virDomainDefPtr def, VIR_DEBUG("Process path '%s' for disk", dev->source.data.file.path); rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path); - if (rc < 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", - dev->source.data.file.path, rc == 0); + qemuAuditCgroupPath(data->vm, data->cgroup, "allow", + dev->source.data.file.path, rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -178,9 +173,7 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); rc = virCgroupAllowDevicePath(data->cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", path, - rc == 0); + qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s"), @@ -216,7 +209,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { qemuCgroupData data = { vm, cgroup }; rc = virCgroupDenyAllDevices(cgroup); - qemuDomainCgroupAudit(vm, cgroup, "deny", "all", NULL, rc == 0); + qemuAuditCgroup(vm, cgroup, "deny", "all", rc == 0); if (rc != 0) { if (rc == -EPERM) { VIR_WARN0("Group devices ACL is not accessible, disabling whitelisting"); @@ -234,7 +227,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, } rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); - qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "pty", rc == 0); + qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, + "pty", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/pts/ devices")); @@ -247,8 +241,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, driver->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); - qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "sound", - rc == 0); + qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, + "sound", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/snd/ devices")); @@ -259,8 +253,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, for (i = 0; deviceACL[i] != NULL ; i++) { rc = virCgroupAllowDevicePath(cgroup, deviceACL[i]); - qemuDomainCgroupAudit(vm, cgroup, "allow", "path", - deviceACL[i], rc == 0); + qemuAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], rc); if (rc < 0 && rc != -ENOENT) { virReportSystemError(-rc, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f7cbad..0f7b5ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1964,8 +1964,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, goto endjob; } rc = virCgroupAllowDevicePath(cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "allow", "path", path, rc == 0); + qemuAuditCgroupPath(vm, cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -2015,8 +2014,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "deny", "path", path, rc == 0); + qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s %d", path, vm->def->name, rc); @@ -2048,9 +2046,7 @@ endjob: if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); - if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "deny", "path", path, - rc == 0); + qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s: %d", path, vm->def->name, rc); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:44PM -0700, Eric Blake wrote:
Device names can be manipulated, so it is better to also log the major/minor device number corresponding to the cgroup ACL changes that libvirt made. This required some refactoring of the relatively new qemu cgroup audit code.
Also, qemuSetupChardevCgroup was only auditing on failure, not success.
+/* Return rdev=nn:mm in hex for block and character devices, rdev=? + * for other file types or stat failure, or NULL on allocation + * failure. */ +#if defined major && defined minor +static char * +qemuAuditGetRdev(const char *path) +{ + char *ret; + struct stat sb; + + if (stat(path, &sb) == 0 && + (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode))) { + int maj = major(sb.st_rdev); + int min = minor(sb.st_rdev); + virAsprintf(&ret, "rdev=%02X:%02X", maj, min); + } else { + ret = strdup("rdev=?"); + } + return ret; +} +#else +static char * +qemuAuditGetRdev(const char *path ATTRIBUTE_UNUSED) +{ + return strdup("rdev=?"); +} +#endif
Rather than have the two strdup("rdev=?") calls, I reckon it would be better to just return NULL. Then the caller can just check for NULL itself & fallback to a static "rdev=?". In fact, perhaps this should just do virAsprintf(&ret, "%02X:%02X", maj, min); And...
+void +qemuAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr cgroup, + const char *reason, const char *path, int rc) +{ + char *detail; + char *rdev; + char *extra; + + /* Nothing to audit for regular files. */ + if (rc > 0) + return; + + if (!(detail = virAuditEncode("path", path)) || + !(rdev = qemuAuditGetRdev(path)) || + virAsprintf(&extra, "path path=%s %s", path, rdev) < 0) {
...here do virAsprintf(&extra, "path path=%s rdev=%s", path, VIR_AUDIT_STR(rdev)) < 0) { ACK, to the rest of the patch though. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/09/2011 08:07 AM, Daniel P. Berrange wrote:
Rather than have the two strdup("rdev=?") calls, I reckon it would be better to just return NULL. Then the caller can just check for NULL itself & fallback to a static "rdev=?".
In fact, perhaps this should just do
virAsprintf(&ret, "%02X:%02X", maj, min);
And...
+ if (!(detail = virAuditEncode("path", path)) || + !(rdev = qemuAuditGetRdev(path)) || + virAsprintf(&extra, "path path=%s %s", path, rdev) < 0) {
...here do
virAsprintf(&extra, "path path=%s rdev=%s", path, VIR_AUDIT_STR(rdev)) < 0) {
Yeah, that's nicer. Will do.
ACK, to the rest of the patch though.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Although the cgroup device ACL controller path can be worked out by researching the code, it is more efficient to include that information directly in the audit message. * src/util/cgroup.h (virCgroupPathOfController): New prototype. * src/util/cgroup.c (virCgroupPathOfController): Export. * src/libvirt_private.syms: Likewise. * src/qemu/qemu_audit.c (qemuAuditCgroup): Use it. --- v2: rebase onto other changes src/libvirt_private.syms | 1 + src/qemu/qemu_audit.c | 19 ++++++++++++++++--- src/util/cgroup.c | 8 ++++---- src/util/cgroup.h | 5 +++++ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index efcf3c5..c0da78e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -79,6 +79,7 @@ virCgroupKill; virCgroupKillRecursive; virCgroupKillPainfully; virCgroupMounted; +virCgroupPathOfController; virCgroupRemove; virCgroupSetBlkioWeight; virCgroupSetCpuShares; diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 56b0b74..08eb431 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -216,11 +216,13 @@ cleanup: * Log an audit message about an attempted cgroup device ACL change. */ void -qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, +qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, const char *reason, const char *extra, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; + char *controller = NULL; + char *detail; virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { @@ -228,11 +230,22 @@ qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, return; } + virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, + NULL, &controller); + + if (!(detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller)))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s", - reason, vmname, uuidstr, extra); + "resrc=cgroup reason=%s %s uuid=%s %s class=%s", + reason, vmname, uuidstr, detail, extra); +cleanup: VIR_FREE(vmname); + VIR_FREE(controller); + VIR_FREE(detail); } /** diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 8551acd..46358ab 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -254,10 +254,10 @@ static int virCgroupDetect(virCgroupPtr group) #endif -static int virCgroupPathOfController(virCgroupPtr group, - int controller, - const char *key, - char **path) +int virCgroupPathOfController(virCgroupPtr group, + int controller, + const char *key, + char **path) { if (controller == -1) { int i; diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d468cb3..b3c5f27 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -40,6 +40,11 @@ int virCgroupForDomain(virCgroupPtr driver, virCgroupPtr *group, int create); +int virCgroupPathOfController(virCgroupPtr group, + int controller, + const char *key, + char **path); + int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:45PM -0700, Eric Blake wrote:
Although the cgroup device ACL controller path can be worked out by researching the code, it is more efficient to include that information directly in the audit message.
* src/util/cgroup.h (virCgroupPathOfController): New prototype. * src/util/cgroup.c (virCgroupPathOfController): Export. * src/libvirt_private.syms: Likewise. * src/qemu/qemu_audit.c (qemuAuditCgroup): Use it. ---
v2: rebase onto other changes
src/libvirt_private.syms | 1 + src/qemu/qemu_audit.c | 19 ++++++++++++++++--- src/util/cgroup.c | 8 ++++---- src/util/cgroup.h | 5 +++++ 4 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index efcf3c5..c0da78e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -79,6 +79,7 @@ virCgroupKill; virCgroupKillRecursive; virCgroupKillPainfully; virCgroupMounted; +virCgroupPathOfController; virCgroupRemove; virCgroupSetBlkioWeight; virCgroupSetCpuShares; diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 56b0b74..08eb431 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -216,11 +216,13 @@ cleanup: * Log an audit message about an attempted cgroup device ACL change. */ void -qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, +qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup, const char *reason, const char *extra, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; + char *controller = NULL; + char *detail;
virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { @@ -228,11 +230,22 @@ qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, return; }
+ virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, + NULL, &controller); + + if (!(detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller)))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s", - reason, vmname, uuidstr, extra); + "resrc=cgroup reason=%s %s uuid=%s %s class=%s", + reason, vmname, uuidstr, detail, extra);
I think perhaps we should make a better effort to output the audit event if creating 'detail' fails. eg remove the goto cleanup and do 'detail ? detail : "cgroup=?"' here Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/09/2011 08:10 AM, Daniel P. Berrange wrote:
+ virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, + NULL, &controller); + + if (!(detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller)))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s", - reason, vmname, uuidstr, extra); + "resrc=cgroup reason=%s %s uuid=%s %s class=%s", + reason, vmname, uuidstr, detail, extra);
I think perhaps we should make a better effort to output the audit event if creating 'detail' fails. eg remove the goto cleanup and do 'detail ? detail : "cgroup=?"' here
Sure. The VIR_AUDIT call is then likely to fail for the same OOM reason, but we might as well try it rather than giving up right away, since the cgroup mount point is not the central detail of the audit message. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Passing the vhost net device fd to qemu is worth an audit point, since it is a kernel-managed device. This patch points out that qemu still can't hot-plug and hot-unplug vhost-net interfaces. * src/qemu/qemu_audit.h (qemuAuditNetVhost): New prototype. * src/qemu/qemu_audit.c (qemuAuditNetVhost): New function. * src/qemu/qemu_command.c (qemuOpenVhostNet): Add audit point and new parameter. (qemuBuildCommandLine): Adjust caller. --- v2: new patch; still missing an audit point for where /dev/net/tun is opened, and the name should probably be qemuAuditNetDevice (since it is feasible to open just /dev/net/tun and not /dev/vhost-net when the xml asks for that). Perhaps should be shuffled to live after patch 8/8. src/qemu/qemu_audit.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_audit.h | 5 +++++ src/qemu/qemu_command.c | 8 +++++--- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 08eb431..1965a20 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -132,6 +132,46 @@ void qemuDomainNetAudit(virDomainObjPtr vm, VIR_FREE(vmname); } +/** + * qemuAuditNetVhost: + * @vm: domain receiving a vhost-net device + * @def: details of network device being attached or removed + * @device: device being attached + * @reason: one of "start", "attach", or "detach" + * @success: true if the device passthrough operation succeeded + * + * Log an audit message about an attempted device passthrough change. + */ +void +qemuAuditNetVhost(virDomainDefPtr vmDef, + virDomainNetDefPtr netDef, const char *device, + const char *reason, bool success) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char macstr[VIR_MAC_STRING_BUFLEN]; + char *vmname; + char *devname; + char *rdev; + + virUUIDFormat(vmDef->uuid, uuidstr); + virFormatMacAddr(netDef->mac, macstr); + if (!(vmname = virAuditEncode("vm", vmDef->name)) || + !(devname = virAuditEncode("path", device)) || + !(rdev = qemuAuditGetRdev(device))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=net reason=%s %s uuid=%s net='%s' %s %s", + reason, vmname, uuidstr, + macstr, devname, rdev); + +cleanup: + VIR_FREE(vmname); + VIR_FREE(devname); + VIR_FREE(rdev); +} /** * qemuDomainHostdevAudit: diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 53855e2..9f08362 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -39,6 +39,11 @@ void qemuDomainNetAudit(virDomainObjPtr vm, virDomainNetDefPtr newDef, const char *reason, bool success); +void qemuAuditNetVhost(virDomainDefPtr vmDef, + virDomainNetDefPtr netDef, + const char *device, + const char *reason, + bool success); void qemuDomainHostdevAudit(virDomainObjPtr vm, virDomainHostdevDefPtr def, const char *reason, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 198a4e2..d5f5a70 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -35,6 +35,7 @@ #include "uuid.h" #include "c-ctype.h" #include "domain_nwfilter.h" +#include "qemu_audit.h" #include <sys/utsname.h> #include <sys/stat.h> @@ -304,11 +305,11 @@ cleanup: static int -qemuOpenVhostNet(virDomainNetDefPtr net, +qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd) { - *vhostfd = -1; /* assume we won't use vhost */ /* If the config says explicitly to not use vhost, return now */ @@ -343,6 +344,7 @@ qemuOpenVhostNet(virDomainNetDefPtr net, } *vhostfd = open("/dev/vhost-net", O_RDWR); + qemuAuditNetVhost(def, net, "/dev/vhost-net", "start", *vhostfd >= 0); /* If the config says explicitly to use vhost and we couldn't open it, * report an error. @@ -3495,7 +3497,7 @@ qemuBuildCommandLine(virConnectPtr conn, network device */ int vhostfd; - if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) goto error; if (vhostfd >= 0) { virCommandTransferFD(cmd, vhostfd); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:46PM -0700, Eric Blake wrote:
Passing the vhost net device fd to qemu is worth an audit point, since it is a kernel-managed device.
This patch points out that qemu still can't hot-plug and hot-unplug vhost-net interfaces.
* src/qemu/qemu_audit.h (qemuAuditNetVhost): New prototype. * src/qemu/qemu_audit.c (qemuAuditNetVhost): New function. * src/qemu/qemu_command.c (qemuOpenVhostNet): Add audit point and new parameter. (qemuBuildCommandLine): Adjust caller. ---
v2: new patch; still missing an audit point for where /dev/net/tun is opened, and the name should probably be qemuAuditNetDevice (since it is feasible to open just /dev/net/tun and not /dev/vhost-net when the xml asks for that). Perhaps should be shuffled to live after patch 8/8.
There are several devices for networking - With type=bridge or type=network, /dev/net/tun is opened to get a FD for a tap device instance - With type=direct, /dev/tap%d is opened to get an FD for a macvtap device instance In both cases, /dev/vhost-net can *also* be opened. The tun/macvtap device provides the connectivity to the host NIC. The vhost-net device provides kernel acceleration for the QEMU NIC backend So I think we need to be auditing all of these devices, if we're going todo any of them
+void +qemuAuditNetVhost(virDomainDefPtr vmDef, + virDomainNetDefPtr netDef, const char *device, + const char *reason, bool success) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char macstr[VIR_MAC_STRING_BUFLEN]; + char *vmname; + char *devname; + char *rdev; + + virUUIDFormat(vmDef->uuid, uuidstr); + virFormatMacAddr(netDef->mac, macstr); + if (!(vmname = virAuditEncode("vm", vmDef->name)) || + !(devname = virAuditEncode("path", device)) || + !(rdev = qemuAuditGetRdev(device))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=net reason=%s %s uuid=%s net='%s' %s %s", + reason, vmname, uuidstr, + macstr, devname, rdev);
A similar thought here about rdev as per the earlier patch in the series -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/09/2011 08:19 AM, Daniel P. Berrange wrote:
There are several devices for networking
- With type=bridge or type=network, /dev/net/tun is opened to get a FD for a tap device instance
- With type=direct, /dev/tap%d is opened to get an FD for a macvtap device instance
In both cases, /dev/vhost-net can *also* be opened. The tun/macvtap device provides the connectivity to the host NIC. The vhost-net device provides kernel acceleration for the QEMU NIC backend
So I think we need to be auditing all of these devices, if we're going todo any of them
Agreed, and did that in v3: https://www.redhat.com/archives/libvir-list/2011-March/msg00416.html
+ if (!(vmname = virAuditEncode("vm", vmDef->name)) || + !(devname = virAuditEncode("path", device)) || + !(rdev = qemuAuditGetRdev(device))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=net reason=%s %s uuid=%s net='%s' %s %s", + reason, vmname, uuidstr, + macstr, devname, rdev);
A similar thought here about rdev as per the earlier patch in the series
Yep, that got fixed while rebasing on the earlier part of the series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_audit.h: The pattern qemuDomainXXXAudit is inconsistent; prefer qemuAuditXXX instead. * src/qemu/qemu_audit.c: Reflect the renames. * src/qemu/qemu_driver.c: Likewise. * src/qemu/qemu_hotplug.c: Likewise. * src/qemu/qemu_process.c: Likewise. * src/qemu/qemu_migration.c: Likewise. --- v2: new patch src/qemu/qemu_audit.c | 84 +++++++++++++++++++++------------------------ src/qemu/qemu_audit.h | 58 ++++++++++++++++-------------- src/qemu/qemu_driver.c | 25 ++++++------- src/qemu/qemu_hotplug.c | 40 +++++++++++----------- src/qemu/qemu_migration.c | 14 ++++---- src/qemu/qemu_process.c | 6 ++-- 6 files changed, 112 insertions(+), 115 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 1965a20..705cab7 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -60,11 +60,10 @@ qemuAuditGetRdev(const char *path ATTRIBUTE_UNUSED) } #endif -void qemuDomainDiskAudit(virDomainObjPtr vm, - virDomainDiskDefPtr oldDef, - virDomainDiskDefPtr newDef, - const char *reason, - bool success) +void +qemuAuditDisk(virDomainObjPtr vm, + virDomainDiskDefPtr oldDef, virDomainDiskDefPtr newDef, + const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; @@ -102,11 +101,10 @@ cleanup: } -void qemuDomainNetAudit(virDomainObjPtr vm, - virDomainNetDefPtr oldDef, - virDomainNetDefPtr newDef, - const char *reason, - bool success) +void +qemuAuditNet(virDomainObjPtr vm, + virDomainNetDefPtr oldDef, virDomainNetDefPtr newDef, + const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char newMacstr[VIR_MAC_STRING_BUFLEN]; @@ -174,7 +172,7 @@ cleanup: } /** - * qemuDomainHostdevAudit: + * qemuAuditHostdev: * @vm: domain making a change in pass-through host device * @hostdev: device being attached or removed * @reason: one of "start", "attach", or "detach" @@ -183,10 +181,8 @@ cleanup: * Log an audit message about an attempted device passthrough change. */ void -qemuDomainHostdevAudit(virDomainObjPtr vm, - virDomainHostdevDefPtr hostdev, - const char *reason, - bool success) +qemuAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, + const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; @@ -355,7 +351,7 @@ cleanup: } /** - * qemuDomainResourceAudit: + * qemuAuditResource: * @vm: domain making an integer resource change * @resource: name of the resource: "mem" or "vcpu" * @oldval: the old value of the resource @@ -366,12 +362,9 @@ cleanup: * Log an audit message about an attempted resource change. */ static void -qemuDomainResourceAudit(virDomainObjPtr vm, - const char *resource, - unsigned long long oldval, - unsigned long long newval, - const char *reason, - bool success) +qemuAuditResource(virDomainObjPtr vm, const char *resource, + unsigned long long oldval, unsigned long long newval, + const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; @@ -391,26 +384,24 @@ qemuDomainResourceAudit(virDomainObjPtr vm, } void -qemuDomainMemoryAudit(virDomainObjPtr vm, - unsigned long long oldmem, unsigned long long newmem, - const char *reason, bool success) +qemuAuditMemory(virDomainObjPtr vm, + unsigned long long oldmem, unsigned long long newmem, + const char *reason, bool success) { - return qemuDomainResourceAudit(vm, "mem", oldmem, newmem, reason, success); + return qemuAuditResource(vm, "mem", oldmem, newmem, reason, success); } void -qemuDomainVcpuAudit(virDomainObjPtr vm, - unsigned int oldvcpu, unsigned int newvcpu, - const char *reason, bool success) +qemuAuditVcpu(virDomainObjPtr vm, + unsigned int oldvcpu, unsigned int newvcpu, + const char *reason, bool success) { - return qemuDomainResourceAudit(vm, "vcpu", oldvcpu, newvcpu, reason, - success); + return qemuAuditResource(vm, "vcpu", oldvcpu, newvcpu, reason, success); } -static void qemuDomainLifecycleAudit(virDomainObjPtr vm, - const char *op, - const char *reason, - bool success) +static void +qemuAuditLifecycle(virDomainObjPtr vm, const char *op, + const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; @@ -429,39 +420,42 @@ static void qemuDomainLifecycleAudit(virDomainObjPtr vm, } -void qemuDomainStartAudit(virDomainObjPtr vm, const char *reason, bool success) +void +qemuAuditDomainStart(virDomainObjPtr vm, const char *reason, bool success) { int i; for (i = 0 ; i < vm->def->ndisks ; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; if (disk->src) /* Skips CDROM without media initially inserted */ - qemuDomainDiskAudit(vm, NULL, disk, "start", true); + qemuAuditDisk(vm, NULL, disk, "start", true); } for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - qemuDomainNetAudit(vm, NULL, net, "start", true); + qemuAuditNet(vm, NULL, net, "start", true); } for (i = 0 ; i < vm->def->nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i]; - qemuDomainHostdevAudit(vm, hostdev, "start", true); + qemuAuditHostdev(vm, hostdev, "start", true); } - qemuDomainMemoryAudit(vm, 0, vm->def->mem.cur_balloon, "start", true); - qemuDomainVcpuAudit(vm, 0, vm->def->vcpus, "start", true); + qemuAuditMemory(vm, 0, vm->def->mem.cur_balloon, "start", true); + qemuAuditVcpu(vm, 0, vm->def->vcpus, "start", true); - qemuDomainLifecycleAudit(vm, "start", reason, success); + qemuAuditLifecycle(vm, "start", reason, success); } -void qemuDomainStopAudit(virDomainObjPtr vm, const char *reason) +void +qemuAuditDomainStop(virDomainObjPtr vm, const char *reason) { - qemuDomainLifecycleAudit(vm, "stop", reason, true); + qemuAuditLifecycle(vm, "stop", reason, true); } -void qemuDomainSecurityLabelAudit(virDomainObjPtr vm, bool success) +void +qemuAuditSecurityLabel(virDomainObjPtr vm, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 9f08362..84874a2 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -27,27 +27,30 @@ # include "domain_conf.h" # include "cgroup.h" -void qemuDomainStartAudit(virDomainObjPtr vm, const char *reason, bool success); -void qemuDomainStopAudit(virDomainObjPtr vm, const char *reason); -void qemuDomainDiskAudit(virDomainObjPtr vm, - virDomainDiskDefPtr oldDef, - virDomainDiskDefPtr newDef, - const char *reason, - bool success); -void qemuDomainNetAudit(virDomainObjPtr vm, - virDomainNetDefPtr oldDef, - virDomainNetDefPtr newDef, - const char *reason, - bool success); +void qemuAuditDomainStart(virDomainObjPtr vm, + const char *reason, + bool success); +void qemuAuditDomainStop(virDomainObjPtr vm, + const char *reason); +void qemuAuditDisk(virDomainObjPtr vm, + virDomainDiskDefPtr oldDef, + virDomainDiskDefPtr newDef, + const char *reason, + bool success); +void qemuAuditNet(virDomainObjPtr vm, + virDomainNetDefPtr oldDef, + virDomainNetDefPtr newDef, + const char *reason, + bool success); void qemuAuditNetVhost(virDomainDefPtr vmDef, virDomainNetDefPtr netDef, const char *device, const char *reason, bool success); -void qemuDomainHostdevAudit(virDomainObjPtr vm, - virDomainHostdevDefPtr def, - const char *reason, - bool success); +void qemuAuditHostdev(virDomainObjPtr vm, + virDomainHostdevDefPtr def, + const char *reason, + bool success); void qemuAuditCgroup(virDomainObjPtr vm, virCgroupPtr group, const char *reason, @@ -64,16 +67,17 @@ void qemuAuditCgroupPath(virDomainObjPtr vm, const char *reason, const char *path, int rc); -void qemuDomainMemoryAudit(virDomainObjPtr vm, - unsigned long long oldmem, - unsigned long long newmem, - const char *reason, - bool success); -void qemuDomainVcpuAudit(virDomainObjPtr vm, - unsigned int oldvcpu, - unsigned int newvcpu, - const char *reason, - bool success); -void qemuDomainSecurityLabelAudit(virDomainObjPtr vm, bool success); +void qemuAuditMemory(virDomainObjPtr vm, + unsigned long long oldmem, + unsigned long long newmem, + const char *reason, + bool success); +void qemuAuditVcpu(virDomainObjPtr vm, + unsigned int oldvcpu, + unsigned int newvcpu, + const char *reason, + bool success); +void qemuAuditSecurityLabel(virDomainObjPtr vm, + bool success); #endif /* __QEMU_AUDIT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f7b5ec..1981cdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1266,7 +1266,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuProcessStart(conn, driver, vm, NULL, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1, NULL, VIR_VM_OP_CREATE) < 0) { - qemuDomainStartAudit(vm, "booted", false); + qemuAuditDomainStart(vm, "booted", false); if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); @@ -1277,7 +1277,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_BOOTED); - qemuDomainStartAudit(vm, "booted", true); + qemuAuditDomainStart(vm, "booted", true); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = vm->def->id; @@ -1492,7 +1492,7 @@ static int qemudDomainDestroy(virDomainPtr dom) { event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - qemuDomainStopAudit(vm, "destroyed"); + qemuAuditDomainStop(vm, "destroyed"); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) @@ -1604,8 +1604,7 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { qemuDomainObjEnterMonitor(vm); r = qemuMonitorSetBalloon(priv->mon, newmem); qemuDomainObjExitMonitor(vm); - qemuDomainMemoryAudit(vm, vm->def->mem.cur_balloon, newmem, "update", - r == 1); + qemuAuditMemory(vm, vm->def->mem.cur_balloon, newmem, "update", r == 1); if (r < 0) goto endjob; @@ -2024,7 +2023,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, /* Shut it down */ qemuProcessStop(driver, vm, 0); - qemuDomainStopAudit(vm, "saved"); + qemuAuditDomainStop(vm, "saved"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); @@ -2420,7 +2419,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, endjob: if ((ret == 0) && (flags & VIR_DUMP_CRASH)) { qemuProcessStop(driver, vm, 0); - qemuDomainStopAudit(vm, "crashed"); + qemuAuditDomainStop(vm, "crashed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); @@ -2552,7 +2551,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) cleanup: qemuDomainObjExitMonitor(vm); - qemuDomainVcpuAudit(vm, oldvcpus, nvcpus, "update", rc == 1); + qemuAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1); return ret; unsupported: @@ -3330,14 +3329,14 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, *read_pid = -1; if (ret < 0) { - qemuDomainStartAudit(vm, "restored", false); + qemuAuditDomainStart(vm, "restored", false); goto out; } event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); - qemuDomainStartAudit(vm, "restored", true); + qemuAuditDomainStart(vm, "restored", true); if (event) qemuDomainEventQueue(driver, event); @@ -3675,7 +3674,7 @@ static int qemudDomainObjStart(virConnectPtr conn, ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, VIR_VM_OP_CREATE); - qemuDomainStartAudit(vm, "booted", ret >= 0); + qemuAuditDomainStart(vm, "booted", ret >= 0); if (ret >= 0) { virDomainEventPtr event = virDomainEventNewFromObj(vm, @@ -6358,7 +6357,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, false, -1, NULL, VIR_VM_OP_CREATE); - qemuDomainStartAudit(vm, "from-snapshot", rc >= 0); + qemuAuditDomainStart(vm, "from-snapshot", rc >= 0); if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) goto endjob; if (rc < 0) @@ -6391,7 +6390,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainObjIsActive(vm)) { qemuProcessStop(driver, vm, 0); - qemuDomainStopAudit(vm, "from-snapshot"); + qemuAuditDomainStop(vm, "from-snapshot"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9991031..7895062 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -108,7 +108,7 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, origdisk, disk, "update", ret >= 0); + qemuAuditDisk(vm, origdisk, disk, "update", ret >= 0); if (ret < 0) goto error; @@ -203,7 +203,7 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, NULL, disk, "attach", ret >= 0); + qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); if (ret < 0) goto error; @@ -435,7 +435,7 @@ int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, NULL, disk, "attach", ret >= 0); + qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); if (ret < 0) goto error; @@ -518,7 +518,7 @@ int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, NULL, disk, "attach", ret >= 0); + qemuAuditDisk(vm, NULL, disk, "attach", ret >= 0); if (ret < 0) goto error; @@ -653,13 +653,13 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorAddNetdev(priv->mon, netstr) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, NULL, net, "attach", false); + qemuAuditNet(vm, NULL, net, "attach", false); goto try_tapfd_close; } } else { if (qemuMonitorAddHostNetwork(priv->mon, netstr) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, NULL, net, "attach", false); + qemuAuditNet(vm, NULL, net, "attach", false); goto try_tapfd_close; } } @@ -685,14 +685,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, NULL, net, "attach", false); + qemuAuditNet(vm, NULL, net, "attach", false); goto try_remove; } } else { if (qemuMonitorAddPCINetwork(priv->mon, nicstr, &guestAddr) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, NULL, net, "attach", false); + qemuAuditNet(vm, NULL, net, "attach", false); goto try_remove; } net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -700,7 +700,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, NULL, net, "attach", true); + qemuAuditNet(vm, NULL, net, "attach", true); ret = 0; @@ -842,7 +842,7 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, hostdev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; memcpy(&hostdev->info.addr.pci, &guestAddr, sizeof(guestAddr)); } - qemuDomainHostdevAudit(vm, hostdev, "attach", ret == 0); + qemuAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) goto error; @@ -919,7 +919,7 @@ int qemuDomainAttachHostUsbDevice(struct qemud_driver *driver, hostdev->source.subsys.u.usb.bus, hostdev->source.subsys.u.usb.device); qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainHostdevAudit(vm, hostdev, "attach", ret == 0); + qemuAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) goto error; @@ -1194,7 +1194,7 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, detach, NULL, "detach", ret >= 0); + qemuAuditDisk(vm, detach, NULL, "detach", ret >= 0); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) @@ -1277,7 +1277,7 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainDiskAudit(vm, detach, NULL, "detach", ret >= 0); + qemuAuditDisk(vm, detach, NULL, "detach", ret >= 0); virDomainDiskRemove(vm->def, i); @@ -1486,14 +1486,14 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitor(vm); - qemuDomainNetAudit(vm, detach, NULL, "detach", false); + qemuAuditNet(vm, detach, NULL, "detach", false); goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, detach, NULL, "detach", false); + qemuAuditNet(vm, detach, NULL, "detach", false); goto cleanup; } } @@ -1502,19 +1502,19 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver, qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, detach, NULL, "detach", false); + qemuAuditNet(vm, detach, NULL, "detach", false); goto cleanup; } } else { if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, detach, NULL, "detach", false); + qemuAuditNet(vm, detach, NULL, "detach", false); goto cleanup; } } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainNetAudit(vm, detach, NULL, "detach", true); + qemuAuditNet(vm, detach, NULL, "detach", true); if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) @@ -1615,7 +1615,7 @@ int qemuDomainDetachHostPciDevice(struct qemud_driver *driver, ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci); } qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainHostdevAudit(vm, detach, "detach", ret == 0); + qemuAuditHostdev(vm, detach, "detach", ret == 0); if (ret < 0) return -1; @@ -1714,7 +1714,7 @@ int qemuDomainDetachHostUsbDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); qemuDomainObjExitMonitorWithDriver(driver, vm); - qemuDomainHostdevAudit(vm, detach, "detach", ret == 0); + qemuAuditHostdev(vm, detach, "detach", ret == 0); if (ret < 0) return -1; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 822cb18..8ce6e17 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -324,7 +324,7 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, -1, NULL, VIR_VM_OP_MIGRATE_IN_START); VIR_FREE(migrateFrom); if (internalret < 0) { - qemuDomainStartAudit(vm, "migrated", false); + qemuAuditDomainStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart * should have already done that. */ @@ -338,7 +338,7 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, if (virFDStreamConnectUNIX(st, unixfile, false) < 0) { - qemuDomainStartAudit(vm, "migrated", false); + qemuAuditDomainStart(vm, "migrated", false); qemuProcessStop(driver, vm, 0); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) @@ -351,7 +351,7 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, goto endjob; } - qemuDomainStartAudit(vm, "migrated", true); + qemuAuditDomainStart(vm, "migrated", true); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, @@ -532,7 +532,7 @@ qemuMigrationPrepareDirect(struct qemud_driver *driver, snprintf (migrateFrom, sizeof (migrateFrom), "tcp:0.0.0.0:%d", this_port); if (qemuProcessStart(dconn, driver, vm, migrateFrom, true, -1, NULL, VIR_VM_OP_MIGRATE_IN_START) < 0) { - qemuDomainStartAudit(vm, "migrated", false); + qemuAuditDomainStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart * should have already done that. */ @@ -544,7 +544,7 @@ qemuMigrationPrepareDirect(struct qemud_driver *driver, goto endjob; } - qemuDomainStartAudit(vm, "migrated", true); + qemuAuditDomainStart(vm, "migrated", true); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_MIGRATED); @@ -1089,7 +1089,7 @@ int qemuMigrationPerform(struct qemud_driver *driver, /* Clean up the source domain. */ qemuProcessStop(driver, vm, 1); - qemuDomainStopAudit(vm, "migrated"); + qemuAuditDomainStop(vm, "migrated"); resume = 0; event = virDomainEventNewFromObj(vm, @@ -1268,7 +1268,7 @@ qemuMigrationFinish(struct qemud_driver *driver, } } else { qemuProcessStop(driver, vm, 1); - qemuDomainStopAudit(vm, "failed"); + qemuAuditDomainStop(vm, "failed"); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ee1b4c4..d3b0691 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -131,7 +131,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); qemuProcessStop(driver, vm, 0); - qemuDomainStopAudit(vm, hasError ? "failed" : "shutdown"); + qemuAuditDomainStop(vm, hasError ? "failed" : "shutdown"); if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); @@ -1935,10 +1935,10 @@ int qemuProcessStart(virConnectPtr conn, then generate a security label for isolation */ VIR_DEBUG0("Generating domain security label (if required)"); if (virSecurityManagerGenLabel(driver->securityManager, vm) < 0) { - qemuDomainSecurityLabelAudit(vm, false); + qemuAuditSecurityLabel(vm, false); goto cleanup; } - qemuDomainSecurityLabelAudit(vm, true); + qemuAuditSecurityLabel(vm, true); VIR_DEBUG0("Generating setting domain security labels (if required)"); if (virSecurityManagerSetAllLabel(driver->securityManager, -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:47PM -0700, Eric Blake wrote:
* src/qemu/qemu_audit.h: The pattern qemuDomainXXXAudit is inconsistent; prefer qemuAuditXXX instead. * src/qemu/qemu_audit.c: Reflect the renames. * src/qemu/qemu_driver.c: Likewise. * src/qemu/qemu_hotplug.c: Likewise. * src/qemu/qemu_process.c: Likewise. * src/qemu/qemu_migration.c: Likewise. ---
v2: new patch
src/qemu/qemu_audit.c | 84 +++++++++++++++++++++------------------------ src/qemu/qemu_audit.h | 58 ++++++++++++++++-------------- src/qemu/qemu_driver.c | 25 ++++++------- src/qemu/qemu_hotplug.c | 40 +++++++++++----------- src/qemu/qemu_migration.c | 14 ++++---- src/qemu/qemu_process.c | 6 ++-- 6 files changed, 112 insertions(+), 115 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Adding audit points showed that we were granting too much privilege to qemu; it should not need any mknod rights to recreate any devices other than ptys. On the other hand, lxc should have all device privileges. The solution is adding a flag parameter. This also lets us restrict write access to read-only disks. * src/util/cgroup.h (virCgroup*Device*): Adjust prototypes. * src/util/cgroup.c (virCgroupAllowDevice) (virCgroupAllowDeviceMajor, virCgroupAllowDevicePath) (virCgroupDenyDevice, virCgroupDenyDeviceMajor) (virCgroupDenyDevicePath): Add parameter. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Update clients. * src/lxc/lxc_controller.c (lxcSetContainerResources): Likewise. * src/qemu/qemu_cgroup.c: Likewise. (qemuSetupDiskPathAllo): Also, honor read-only disks. --- v2: new patch src/lxc/lxc_controller.c | 9 +++++-- src/qemu/qemu_cgroup.c | 27 +++++++++++++--------- src/qemu/qemu_driver.c | 9 +++++-- src/util/cgroup.c | 55 +++++++++++++++++++++++++++++++++------------ src/util/cgroup.h | 26 ++++++++++++++++----- 5 files changed, 88 insertions(+), 38 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index d2b113c..296b302 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1,5 +1,6 @@ /* - * Copyright (C) 2010 Red Hat, Inc. Copyright IBM Corp. 2008 + * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright IBM Corp. 2008 * * lxc_controller.c: linux container process controller * @@ -168,7 +169,8 @@ static int lxcSetContainerResources(virDomainDefPtr def) rc = virCgroupAllowDevice(cgroup, dev->type, dev->major, - dev->minor); + dev->minor, + VIR_CGROUP_DEVICE_RWM); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %c:%d:%d for domain %s"), @@ -177,7 +179,8 @@ static int lxcSetContainerResources(virDomainDefPtr def) } } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, + VIR_CGROUP_DEVICE_RWM); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow PYT devices for domain %s"), diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ebf9ad5..83063a9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -56,7 +56,7 @@ int qemuCgroupControllerActive(struct qemud_driver *driver, } static int -qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, +qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, const char *path, size_t depth ATTRIBUTE_UNUSED, void *opaque) @@ -65,8 +65,9 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, int rc; VIR_DEBUG("Process path %s for disk", path); - /* XXX RO vs RW */ - rc = virCgroupAllowDevicePath(data->cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path, + (disk->readonly ? VIR_CGROUP_DEVICE_READ + : VIR_CGROUP_DEVICE_RW)); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ @@ -106,8 +107,8 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, int rc; VIR_DEBUG("Process path %s for disk", path); - /* XXX RO vs RW */ - rc = virCgroupDenyDevicePath(data->cgroup, path); + rc = virCgroupDenyDevicePath(data->cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(data->vm, data->cgroup, "deny", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ @@ -150,7 +151,8 @@ qemuSetupChardevCgroup(virDomainDefPtr def, VIR_DEBUG("Process path '%s' for disk", dev->source.data.file.path); - rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path); + rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", dev->source.data.file.path, rc); if (rc < 0) { @@ -172,7 +174,8 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, int rc; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupAllowDevicePath(data->cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, @@ -226,7 +229,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, goto cleanup; } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, "pty", rc == 0); if (rc != 0) { @@ -240,7 +244,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, ((vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && driver->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { - rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, "sound", rc == 0); if (rc != 0) { @@ -251,8 +256,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, } for (i = 0; deviceACL[i] != NULL ; i++) { - rc = virCgroupAllowDevicePath(cgroup, - deviceACL[i]); + rc = virCgroupAllowDevicePath(cgroup, deviceACL[i], + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], rc); if (rc < 0 && rc != -ENOENT) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1981cdf..7b4edc5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1962,7 +1962,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, vm->def->name); goto endjob; } - rc = virCgroupAllowDevicePath(cgroup, path); + rc = virCgroupAllowDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(vm, cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, @@ -2012,7 +2013,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, VIR_WARN("failed to restore save state label on %s", path); if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path); + rc = virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s %d", @@ -2044,7 +2046,8 @@ endjob: } if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path); + rc = virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s: %d", diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 46358ab..9986e53 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1081,7 +1081,7 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) /** * virCgroupDenyAllDevices: * - * @group: The cgroup to deny devices for + * @group: The cgroup to deny all permissions, for all devices * * Returns: 0 on success */ @@ -1100,15 +1100,20 @@ int virCgroupDenyAllDevices(virCgroupPtr group) * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device * @minor: The minor number of the device + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Returns: 0 on success */ -int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor) +int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i rwm", type, major, minor) == -1) { + if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1129,15 +1134,20 @@ out: * @group: The cgroup to allow an entire device major type for * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device type + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Returns: 0 on success */ -int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) +int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* rwm", type, major) == -1) { + if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1157,6 +1167,7 @@ int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) * * @group: The cgroup to allow the device for * @path: the device to allow + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Queries the type of device and its major/minor number, and * adds that to the cgroup ACL @@ -1165,7 +1176,7 @@ int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) * negative errno value on failure */ #if defined(major) && defined(minor) -int virCgroupAllowDevicePath(virCgroupPtr group, const char *path) +int virCgroupAllowDevicePath(virCgroupPtr group, const char *path, int perms) { struct stat sb; @@ -1178,11 +1189,13 @@ int virCgroupAllowDevicePath(virCgroupPtr group, const char *path) return virCgroupAllowDevice(group, S_ISCHR(sb.st_mode) ? 'c' : 'b', major(sb.st_rdev), - minor(sb.st_rdev)); + minor(sb.st_rdev), + perms); } #else int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED) + const char *path ATTRIBUTE_UNUSED, + int perms ATTRIBUTE_UNUSED) { return -ENOSYS; } @@ -1196,15 +1209,20 @@ int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device * @minor: The minor number of the device + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny * * Returns: 0 on success */ -int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor) +int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i rwm", type, major, minor) == -1) { + if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1225,15 +1243,20 @@ out: * @group: The cgroup to deny an entire device major type for * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device type + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny * * Returns: 0 on success */ -int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major) +int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* rwm", type, major) == -1) { + if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1249,7 +1272,7 @@ int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major) } #if defined(major) && defined(minor) -int virCgroupDenyDevicePath(virCgroupPtr group, const char *path) +int virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms) { struct stat sb; @@ -1262,11 +1285,13 @@ int virCgroupDenyDevicePath(virCgroupPtr group, const char *path) return virCgroupDenyDevice(group, S_ISCHR(sb.st_mode) ? 'c' : 'b', major(sb.st_rdev), - minor(sb.st_rdev)); + minor(sb.st_rdev), + perms); } #else int virCgroupDenyDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED) + const char *path ATTRIBUTE_UNUSED, + int perms ATTRIBUTE_UNUSED) { return -ENOSYS; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index b3c5f27..16ffb46 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -60,27 +60,41 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb); int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb); int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb); +enum { + VIR_CGROUP_DEVICE_READ = 1, + VIR_CGROUP_DEVICE_WRITE = 2, + VIR_CGROUP_DEVICE_MKNOD = 4, + VIR_CGROUP_DEVICE_RW = VIR_CGROUP_DEVICE_READ | VIR_CGROUP_DEVICE_WRITE, + VIR_CGROUP_DEVICE_RWM = VIR_CGROUP_DEVICE_RW | VIR_CGROUP_DEVICE_MKNOD, +}; + int virCgroupDenyAllDevices(virCgroupPtr group); int virCgroupAllowDevice(virCgroupPtr group, char type, int major, - int minor); + int minor, + int perms); int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, - int major); + int major, + int perms); int virCgroupAllowDevicePath(virCgroupPtr group, - const char *path); + const char *path, + int perms); int virCgroupDenyDevice(virCgroupPtr group, char type, int major, - int minor); + int minor, + int perms); int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, - int major); + int major, + int perms); int virCgroupDenyDevicePath(virCgroupPtr group, - const char *path); + const char *path, + int perms); int virCgroupSetCpuShares(virCgroupPtr group, unsigned long long shares); int virCgroupGetCpuShares(virCgroupPtr group, unsigned long long *shares); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:48PM -0700, Eric Blake wrote:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ebf9ad5..83063a9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -226,7 +229,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, goto cleanup; }
- rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, "pty", rc == 0); if (rc != 0) {
I'm not entirely sure that we actually need RWM here, rather than just RW. QEMU doesn't do any mknod in /dev/pts/XXX. The entries in that filesystem just magically appear from the kernel when you open /dev/ptmx.
@@ -240,7 +244,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, ((vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && driver->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { - rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, "sound", rc == 0); if (rc != 0) {
Almost certain we don't need RWM for sound devices, just RW Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/09/2011 08:24 AM, Daniel P. Berrange wrote:
On Tue, Mar 08, 2011 at 10:13:48PM -0700, Eric Blake wrote:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ebf9ad5..83063a9 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -226,7 +229,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, goto cleanup; }
- rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, "pty", rc == 0); if (rc != 0) {
I'm not entirely sure that we actually need RWM here, rather than just RW. QEMU doesn't do any mknod in /dev/pts/XXX. The entries in that filesystem just magically appear from the kernel when you open /dev/ptmx.
I tested with just RW instead of RWM, and was still able to do 'virsh console' with no change in behavior, so I went with this change. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_audit.h (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add parameter. * src/qemu/qemu_audit.c (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add 'acl=rwm' to cgroup audit entries. * src/qemu/qemu_cgroup.c: Update clients. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Likewise. --- v2: new patch; perhaps patch should be floated before patch 2, and then this patch squashed into patch 2, so that I'm only touching qemuAuditCgroupPath once? src/qemu/qemu_audit.c | 12 ++++++++---- src/qemu/qemu_audit.h | 2 ++ src/qemu/qemu_cgroup.c | 15 ++++++++------- src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 705cab7..ea4f22b 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -291,6 +291,7 @@ cleanup: * @reason: either "allow" or "deny" * @maj: the major number of the device category * @name: a textual name for that device category, alphabetic only + * @perms: string containing "r", "w", and/or "m" as appropriate * @success: true if the cgroup operation succeeded * * Log an audit message about an attempted cgroup device ACL change. @@ -298,11 +299,12 @@ cleanup: void qemuAuditCgroupMajor(virDomainObjPtr vm, virCgroupPtr cgroup, const char *reason, int maj, const char *name, - bool success) + const char *perms, bool success) { char *extra; - if (virAsprintf(&extra, "major category=%s maj=%02X", name, maj) < 0) { + if (virAsprintf(&extra, "major category=%s maj=%02X acl=%s", + name, maj, perms) < 0) { VIR_WARN0("OOM while encoding audit message"); return; } @@ -318,6 +320,7 @@ qemuAuditCgroupMajor(virDomainObjPtr vm, virCgroupPtr cgroup, * @cgroup: cgroup that manages the devices * @reason: either "allow" or "deny" * @path: the device being adjusted + * @perms: string containing "r", "w", and/or "m" as appropriate * @rc: > 0 if not a device, 0 if success, < 0 if failure * * Log an audit message about an attempted cgroup device ACL change to @@ -325,7 +328,8 @@ qemuAuditCgroupMajor(virDomainObjPtr vm, virCgroupPtr cgroup, */ void qemuAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr cgroup, - const char *reason, const char *path, int rc) + const char *reason, const char *path, const char *perms, + int rc) { char *detail; char *rdev; @@ -337,7 +341,7 @@ qemuAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr cgroup, if (!(detail = virAuditEncode("path", path)) || !(rdev = qemuAuditGetRdev(path)) || - virAsprintf(&extra, "path path=%s %s", path, rdev) < 0) { + virAsprintf(&extra, "path path=%s %s acl=%s", path, rdev, perms) < 0) { VIR_WARN0("OOM while encoding audit message"); goto cleanup; } diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 84874a2..a9300d3 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -61,11 +61,13 @@ void qemuAuditCgroupMajor(virDomainObjPtr vm, const char *reason, int maj, const char *name, + const char *perms, bool success); void qemuAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr group, const char *reason, const char *path, + const char *perms, int rc); void qemuAuditMemory(virDomainObjPtr vm, unsigned long long oldmem, diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 83063a9..f65445c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -68,7 +68,8 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, rc = virCgroupAllowDevicePath(data->cgroup, path, (disk->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW)); - qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); + qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, + disk->readonly ? "r" : "rw", rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -109,7 +110,7 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); rc = virCgroupDenyDevicePath(data->cgroup, path, VIR_CGROUP_DEVICE_RWM); - qemuAuditCgroupPath(data->vm, data->cgroup, "deny", path, rc); + qemuAuditCgroupPath(data->vm, data->cgroup, "deny", path, "rwm", rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -154,7 +155,7 @@ qemuSetupChardevCgroup(virDomainDefPtr def, rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path, VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", - dev->source.data.file.path, rc); + dev->source.data.file.path, "rw", rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -176,7 +177,7 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); rc = virCgroupAllowDevicePath(data->cgroup, path, VIR_CGROUP_DEVICE_RW); - qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); + qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, "rw", rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s"), @@ -232,7 +233,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR, VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, - "pty", rc == 0); + "pty", "rwm", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/pts/ devices")); @@ -247,7 +248,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR, VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, - "sound", rc == 0); + "sound", "rwm", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/snd/ devices")); @@ -258,7 +259,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, for (i = 0; deviceACL[i] != NULL ; i++) { rc = virCgroupAllowDevicePath(cgroup, deviceACL[i], VIR_CGROUP_DEVICE_RW); - qemuAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], rc); + qemuAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], "rw", rc); if (rc < 0 && rc != -ENOENT) { virReportSystemError(-rc, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7b4edc5..ca2b61d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1964,7 +1964,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, } rc = virCgroupAllowDevicePath(cgroup, path, VIR_CGROUP_DEVICE_RW); - qemuAuditCgroupPath(vm, cgroup, "allow", path, rc); + qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -2015,7 +2015,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path, VIR_CGROUP_DEVICE_RWM); - qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); + qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s %d", path, vm->def->name, rc); @@ -2048,7 +2048,7 @@ endjob: if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path, VIR_CGROUP_DEVICE_RWM); - qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); + qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s: %d", path, vm->def->name, rc); -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:49PM -0700, Eric Blake wrote:
* src/qemu/qemu_audit.h (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add parameter. * src/qemu/qemu_audit.c (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add 'acl=rwm' to cgroup audit entries. * src/qemu/qemu_cgroup.c: Update clients. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Likewise. ---
v2: new patch; perhaps patch should be floated before patch 2, and then this patch squashed into patch 2, so that I'm only touching qemuAuditCgroupPath once?
I don't think it hugely matters.
src/qemu/qemu_audit.c | 12 ++++++++---- src/qemu/qemu_audit.h | 2 ++ src/qemu/qemu_cgroup.c | 15 ++++++++------- src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 21 insertions(+), 14 deletions(-)
ACK, unless it needs some changes based on my two comments to the previous patch about certain RWM vs RW usage. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/09/2011 08:26 AM, Daniel P. Berrange wrote:
On Tue, Mar 08, 2011 at 10:13:49PM -0700, Eric Blake wrote:
* src/qemu/qemu_audit.h (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add parameter. * src/qemu/qemu_audit.c (qemuAuditCgroupMajor) (qemuAuditCgroupPath): Add 'acl=rwm' to cgroup audit entries. * src/qemu/qemu_cgroup.c: Update clients. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Likewise. ---
v2: new patch; perhaps patch should be floated before patch 2, and then this patch squashed into patch 2, so that I'm only touching qemuAuditCgroupPath once?
I don't think it hugely matters.
src/qemu/qemu_audit.c | 12 ++++++++---- src/qemu/qemu_audit.h | 2 ++ src/qemu/qemu_cgroup.c | 15 ++++++++------- src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 21 insertions(+), 14 deletions(-)
ACK, unless it needs some changes based on my two comments to the previous patch about certain RWM vs RW usage.
It needed a tweak ("rw" vs. "rwm"). I've applied patches 1-3 and 5-7, and will be doing a PATCHv3 for patch 4, 8, and other followups for auditing network devices after I finish more testing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Honor vhost designations, similar to qemu_command code paths. * src/qemu/qemu_command.h (qemuOpenVhostNet): New prototype. * src/qemu/qemu_command.c (qemuOpenVhostNet): Export. --- v2: new patch; as yet untested, and needs patch 9/8 to do audit hooks for the opened devices src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 5 ++++ src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5f5a70..af36866 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -304,7 +304,7 @@ cleanup: } -static int +int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virBitmapPtr qemuCaps, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1902472..4c0713c 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -129,6 +129,11 @@ int qemuPhysIfaceConnect(virConnectPtr conn, const unsigned char *vmuuid, enum virVMOperationType vmop); +int qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, + virBitmapPtr qemuCaps, + int *vhostfd); + int qemudCanonicalizeMachine(struct qemud_driver *driver, virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7895062..9fbd004 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -552,6 +552,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; char *tapfd_name = NULL; int tapfd = -1; + char *vhostfd_name = NULL; + int vhostfd = -1; char *nicstr = NULL; char *netstr = NULL; int ret = -1; @@ -592,6 +594,24 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || + net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + if (qemuOpenVhostNet(vm->def, net, qemuCaps, &vhostfd) < 0) + goto cleanup; + + if (vhostfd >= 0 && + priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' cannot be attached: " + "qemu is not using a unix socket monitor"), + virDomainNetTypeToString(net->type)); + goto cleanup; + } + } + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) goto no_memory; @@ -636,15 +656,32 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } } - /* FIXME - need to support vhost-net here (5th arg) */ + if (vhostfd != -1) { + if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) + goto no_memory; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuMonitorSendFileHandle(priv->mon, vhostfd_name, vhostfd) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto try_tapfd_close; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, ',', - -1, tapfd_name, 0))) + -1, tapfd_name, vhostfd_name))) goto try_tapfd_close; } else { if (!(netstr = qemuBuildHostNetStr(net, ' ', - vlan, tapfd_name, 0))) + vlan, tapfd_name, vhostfd_name))) goto try_tapfd_close; } @@ -666,6 +703,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjExitMonitorWithDriver(driver, vm); VIR_FORCE_CLOSE(tapfd); + VIR_FORCE_CLOSE(vhostfd); if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -720,6 +758,8 @@ cleanup: VIR_FREE(netstr); VIR_FREE(tapfd_name); VIR_FORCE_CLOSE(tapfd); + VIR_FREE(vhostfd_name); + VIR_FORCE_CLOSE(vhostfd); return ret; @@ -759,10 +799,14 @@ try_tapfd_close: if (!virDomainObjIsActive(vm)) goto cleanup; - if (tapfd_name) { + if (tapfd_name || vhostfd_name) { qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0) + if (tapfd_name && + qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0) VIR_WARN("Failed to close tapfd with '%s'", tapfd_name); + if (vhostfd_name && + qemuMonitorCloseFileHandle(priv->mon, vhostfd_name) < 0) + VIR_WARN("Failed to close vhostfd with '%s'", vhostfd_name); qemuDomainObjExitMonitorWithDriver(driver, vm); } -- 1.7.4
On Tue, Mar 08, 2011 at 10:13:50PM -0700, Eric Blake wrote:
* src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Honor vhost designations, similar to qemu_command code paths. * src/qemu/qemu_command.h (qemuOpenVhostNet): New prototype. * src/qemu/qemu_command.c (qemuOpenVhostNet): Export. ---
v2: new patch; as yet untested, and needs patch 9/8 to do audit hooks for the opened devices
I think it looks basically sane, but yeah needs to be tested Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Eric Blake