[libvirt] [PATCH RFC 0/3] Keep original file label
Just sending out early to make sure this time I am going in acceptable direction before digging into selinux. Hopefully, apparmor won't be any deal as I don't see anything that should be restored on domain shut off process. Michal Privoznik (3): security_dac: Remember owner prior chown() and restore on relabel security_manager: Introduce {Save,Load}Status security_dac: Implement {save,load}Status src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 1 + src/qemu/qemu_driver.c | 3 + src/security/security_dac.c | 465 ++++++++++++++++++++++++++++++++++----- src/security/security_driver.h | 12 + src/security/security_manager.c | 161 +++++++++++++- src/security/security_manager.h | 2 + tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 3 +- 10 files changed, 590 insertions(+), 64 deletions(-) -- 1.8.1.4
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong. However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner. In order to not pollute domain XML, this info is kept in driver's private data in a hash table with path being key and pair <oldLabel, refcount> being value. --- src/security/security_dac.c | 351 ++++++++++++++++++++++++++++++++++------- src/security/security_driver.h | 3 + 2 files changed, 296 insertions(+), 58 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..4b8f0a2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -42,8 +42,121 @@ struct _virSecurityDACData { uid_t user; gid_t group; bool dynamicOwnership; + virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ }; +struct _virOldLabel { + char *owner; + int refCount; +}; + +static void virOldLabelFree(virOldLabelPtr label) +{ + if (!label) + return; + + VIR_FREE(label->owner); + VIR_FREE(label); +} + +static void +hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + virOldLabelFree(payload); +} + +/** + * virSecurityDACRememberLabel: + * @priv: private DAC driver data + * @path: path which is about to be relabelled + * + * Store the original DAC label of @path. + * Returns: number of references of @path, or -1 on error + */ +static int +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, + const char *path) +{ + struct stat sb; + virOldLabelPtr oldLabel = NULL; + char *user = NULL, *group = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + + if (oldLabel) { + /* just increment ref counter */ + oldLabel->refCount++; + goto cleanup; + } + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat %s"), path); + goto cleanup; + } + + user = virGetUserName(sb.st_uid); + group = virGetGroupName(sb.st_gid); + if ((!user && (virAsprintf(&user, "+%ld", (long) sb.st_uid) < 0)) || + (!group && (virAsprintf(&group, "+%ld", (long) sb.st_gid) < 0)) || + (VIR_ALLOC(oldLabel) < 0) || + (virAsprintf(&oldLabel->owner, "%s:%s", user, group) < 0)) { + virReportOOMError(); + VIR_FREE(oldLabel); + goto cleanup; + } + + oldLabel->refCount = 1; + if (virHashUpdateEntry(priv->oldOwners, path, oldLabel) < 0) { + virOldLabelFree(oldLabel); + oldLabel = NULL; + } + +cleanup: + VIR_FREE(user); + VIR_FREE(group); + return oldLabel ? oldLabel->refCount : -1; +} + +static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); + +/** + * virSecurityDACGetRememberedLabel: + * @priv: private DAC driver data + * @path: path which we want to restore label on + * @user: original owner of @path + * @group: original owner of @path + * + * Decrements reference counter on @path. If this was the last + * reference, we need to restore the original label, in which + * case @user and @group are updated. + * Returns: the number of references of @path + * 0 if we need to restore the label + * -1 on error + */ +static int +virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t *user, + gid_t *group) +{ + int ret = -1; + virOldLabelPtr oldLabel = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + if (!oldLabel) + goto cleanup; + + ret = --oldLabel->refCount; + + if (!ret) { + ret = parseIds(oldLabel->owner, user, group); + virHashRemoveEntry(priv->oldOwners, path); + } + +cleanup: + return ret; +} + void virSecurityDACSetUser(virSecurityManagerPtr mgr, uid_t user) { @@ -242,14 +355,20 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) } static int -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACOpen(virSecurityManagerPtr mgr) { - return 0; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + priv->oldOwners = virHashCreate(0, hashDataFree); + return priv->oldOwners ? 0 : -1; } static int -virSecurityDACClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACClose(virSecurityManagerPtr mgr) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + virHashFree(priv->oldOwners); return 0; } @@ -306,7 +425,9 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) } static int -virSecurityDACRestoreSecurityFileLabel(const char *path) +virSecurityDACRestoreSecurityFileLabel(const char *path, + uid_t user, + gid_t group) { struct stat buf; int rc = -1; @@ -323,8 +444,7 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) if (stat(newpath, &buf) != 0) goto err; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + rc = virSecurityDACSetOwnership(newpath, user, group); err: VIR_FREE(newpath); @@ -345,7 +465,8 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, path) < 0)) return -1; return virSecurityDACSetOwnership(path, user, group); @@ -382,26 +503,14 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk, int migrated) { + int ret; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0; - if (!priv->dynamicOwnership) - return 0; - - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) - return 0; - - /* Don't restore labels on readoly/shared disks, because - * other VMs may still be accessing these - * Alternatively we could iterate over all running - * domains and try to figure out if it is in use, but - * this would not work for clustered filesystems, since - * we can't see running VMs using the file on other nodes - * Safest bet is thus to skip the restore step. - */ - if (disk->readonly || disk->shared) - return 0; - - if (!disk->src) + if (!priv->dynamicOwnership || + (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) || + !disk->src) return 0; /* If we have a shared FS & doing migrated, we must not @@ -411,7 +520,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, */ if (migrated) { int rc = virStorageFileIsSharedFS(disk->src); - if (rc < 0) + if (rc > 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", @@ -420,7 +529,15 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreSecurityFileLabel(disk->src); + ret = virSecurityDACGetRememberedLabel(priv, disk->src, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping image label restore on %s " + "because the image is still in use", disk->src); + return 0; + } + return virSecurityDACRestoreSecurityFileLabel(disk->src, user, group); } @@ -445,7 +562,8 @@ virSecurityDACSetSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, file) < 0)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -464,7 +582,8 @@ virSecurityDACSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group)) + if (virSecurityDACGetIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, file) < 0)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -536,18 +655,46 @@ done: static int virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(opaque); + uid_t user = 0; + gid_t group = 0; + int ret; + + ret = virSecurityDACGetRememberedLabel(priv, file, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the PCI device is still in use", file); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(file, user, group); } static int virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(opaque); + uid_t user = 0; + gid_t group = 0; + int ret; + + ret = virSecurityDACGetRememberedLabel(priv, file, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the USB device is still in use", file); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(file, user, group); } @@ -630,6 +777,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: + if (virSecurityDACRememberLabel(priv, dev->data.file.path) < 0) + goto cleanup; ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); break; @@ -637,16 +786,22 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { virReportOOMError(); - goto done; + goto cleanup; } if (virFileExists(in) && virFileExists(out)) { + if ((virSecurityDACRememberLabel(priv, in) < 0) || + (virSecurityDACRememberLabel(priv, out) < 0)) + goto cleanup; if ((virSecurityDACSetOwnership(in, user, group) < 0) || (virSecurityDACSetOwnership(out, user, group) < 0)) { - goto done; + goto cleanup; } - } else if (virSecurityDACSetOwnership(dev->data.file.path, - user, group) < 0) { - goto done; + } else { + if (virSecurityDACRememberLabel(priv, dev->data.file.path) < 0) + goto cleanup; + if (virSecurityDACSetOwnership(dev->data.file.path, + user, group) < 0) + goto cleanup; } ret = 0; break; @@ -656,38 +811,85 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, break; } -done: +cleanup: VIR_FREE(in); VIR_FREE(out); return ret; } static int -virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrSourceDefPtr dev) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); char *in = NULL, *out = NULL; int ret = -1; + uid_t user = 0; + gid_t group = 0; switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path); + ret = virSecurityDACGetRememberedLabel(priv, dev->data.file.path, + &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", + dev->data.file.path); + return 0; + } + + ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path, + user, group); break; case VIR_DOMAIN_CHR_TYPE_PIPE: if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) || (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) { virReportOOMError(); - goto done; + goto cleanup; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || - (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { - goto done; + ret = virSecurityDACGetRememberedLabel(priv, in, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", in); + goto cleanup; + } + ret = virSecurityDACGetRememberedLabel(priv, out, &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", out); + goto cleanup; + } + if ((virSecurityDACRestoreSecurityFileLabel(out, user, group) < 0) || + (virSecurityDACRestoreSecurityFileLabel(in, user, group) < 0)) { + ret = -1; + goto cleanup; + } + } else { + ret = virSecurityDACGetRememberedLabel(priv, dev->data.file.path, + &user, &group); + if (ret < 0) + goto cleanup; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the chardev device is still in use", + dev->data.file.path); + goto cleanup; + } + + if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path, + user, group) < 0) { + ret = -1; + goto cleanup; } - } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) { - goto done; } ret = 0; break; @@ -697,7 +899,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, break; } -done: +cleanup: VIR_FREE(in); VIR_FREE(out); return ret; @@ -723,6 +925,8 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int i; int rc = 0; + uid_t user = 0; + gid_t group = 0; if (!priv->dynamicOwnership) return 0; @@ -752,13 +956,29 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) rc = -1; - if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) - rc = -1; + if (def->os.kernel) { + i = virSecurityDACGetRememberedLabel(priv, def->os.kernel, &user, &group); + if (i < 0) + rc = -1; + else if (i > 0) + VIR_DEBUG("Skipping security label restore on %s " + "because the kernel image is still in use", def->os.kernel); + else if (virSecurityDACRestoreSecurityFileLabel(def->os.kernel, + user, group) < 0) + rc = -1; + } - if (def->os.initrd && - virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0) - rc = -1; + if (def->os.initrd) { + i = virSecurityDACGetRememberedLabel(priv, def->os.initrd, &user, &group); + if (i < 0) + rc = -1; + else if (i > 0) + VIR_DEBUG("Skipping security label restore on %s " + "because the initrd image is still in use", def->os.initrd); + else if (virSecurityDACRestoreSecurityFileLabel(def->os.initrd, + user, group) < 0) + rc = -1; + } return rc; } @@ -815,11 +1035,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.kernel) < 0) || + (virSecurityDACSetOwnership(def->os.kernel, user, group) < 0))) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(def->os.initrd, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.initrd) < 0) || + (virSecurityDACSetOwnership(def->os.initrd, user, group) < 0))) return -1; return 0; @@ -835,7 +1057,8 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, savefile) < 0)) return -1; return virSecurityDACSetOwnership(savefile, user, group); @@ -848,11 +1071,23 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, const char *savefile) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0; + int ret; if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreSecurityFileLabel(savefile); + ret = virSecurityDACGetRememberedLabel(priv, savefile, &user, &group); + if (ret < 0) + return -1; + else if (ret > 0) { + VIR_DEBUG("Skipping security label restore on %s " + "because the saved state file is still in use", savefile); + return 0; + } + + return virSecurityDACRestoreSecurityFileLabel(savefile, user, group); } diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cc401e1..d54f754 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -40,6 +40,9 @@ typedef enum { typedef struct _virSecurityDriver virSecurityDriver; typedef virSecurityDriver *virSecurityDriverPtr; +typedef struct _virOldLabel virOldLabel; +typedef virOldLabel *virOldLabelPtr; + typedef virSecurityDriverStatus (*virSecurityDriverProbe) (const char *virtDriver); typedef int (*virSecurityDriverOpen) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverClose) (virSecurityManagerPtr mgr); -- 1.8.1.4
On 02/26/2013 09:08 AM, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Definitely important for a read-only file shared by more than one domain.
In order to not pollute domain XML, this info is kept in driver's private data in a hash table with path being key and pair <oldLabel, refcount> being value.
Makes sense. Have you looked at what it would take to use ACLs to grant access to qemu without having to do a full-blown chown? That would also need to use the hash table to undo the ACL at the end of the day, and we would need to fall back to chown() on file systems where ACL doesn't work, but it certainly sounds like that would be sharing some of the work in this patch.
--- src/security/security_dac.c | 351 ++++++++++++++++++++++++++++++++++------- src/security/security_driver.h | 3 + 2 files changed, 296 insertions(+), 58 deletions(-)
Couple of questions below:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..4b8f0a2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -42,8 +42,121 @@ struct _virSecurityDACData { uid_t user; gid_t group; bool dynamicOwnership; + virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ };
+struct _virOldLabel { + char *owner;
Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.
+ int refCount; +}; + +static void virOldLabelFree(virOldLabelPtr label)
Line break after void.
+/** + * virSecurityDACRememberLabel: + * @priv: private DAC driver data + * @path: path which is about to be relabelled + * + * Store the original DAC label of @path. + * Returns: number of references of @path, or -1 on error + */ +static int +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, + const char *path) +{ + struct stat sb; + virOldLabelPtr oldLabel = NULL; + char *user = NULL, *group = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + + if (oldLabel) { + /* just increment ref counter */ + oldLabel->refCount++;
Is this threadsafe, or do we need to use virAtomic?
+ goto cleanup; + } + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat %s"), path); + goto cleanup; + } + + user = virGetUserName(sb.st_uid); + group = virGetGroupName(sb.st_gid); + if ((!user && (virAsprintf(&user, "+%ld", (long) sb.st_uid) < 0)) || + (!group && (virAsprintf(&group, "+%ld", (long) sb.st_gid) < 0)) ||
Given Philipp's recent series, it's probably better to mix %u and (unsigned), rather than %ld and (long) (that is, we've already asserted that uid_t fits within an int, and that unsigned is more likely than signed to represent all users except for the sentinel of -1, and -1 is not going to appear as stat() results). Again, if your struct has a uid_t and gid_t instead of a char*, then you don't have to convert to string here.
+ (VIR_ALLOC(oldLabel) < 0) || + (virAsprintf(&oldLabel->owner, "%s:%s", user, group) < 0)) {
Worse, you are printing into two temporaries only to create yet another malloc'd string. If you do keep a char*, why not just: virAsprintf(&oldLabel->owner, "+%u:+%u", (unsigned) sb.st_uid, (unsigned) sb.st_gid);
+ +static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr);
Any reason you can't topologically sort that function to be first, and avoid a forward declaration?
+ +/** + * virSecurityDACGetRememberedLabel: + * @priv: private DAC driver data + * @path: path which we want to restore label on + * @user: original owner of @path + * @group: original owner of @path + * + * Decrements reference counter on @path. If this was the last + * reference, we need to restore the original label, in which + * case @user and @group are updated. + * Returns: the number of references of @path + * 0 if we need to restore the label + * -1 on error + */ +static int +virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t *user, + gid_t *group) +{ + int ret = -1; + virOldLabelPtr oldLabel = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + if (!oldLabel) + goto cleanup; + + ret = --oldLabel->refCount;
Thread safety again. virAtomic?
+ + if (!ret) { + ret = parseIds(oldLabel->owner, user, group);
If the hash value struct has uid_t and gid_t fields, then no parsing is needed here.
static int -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACOpen(virSecurityManagerPtr mgr) { - return 0; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + priv->oldOwners = virHashCreate(0, hashDataFree); + return priv->oldOwners ? 0 : -1;
Caller doesn't report an error, so you need a virReportOOMError()
@@ -382,26 +503,14 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk, int migrated) { + int ret; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0;
I prefer -1 when initializing an unknown uid or gid; 0 has special meaning, and there are cases where we really do want to restore to uid 0.
@@ -411,7 +520,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, */ if (migrated) { int rc = virStorageFileIsSharedFS(disk->src); - if (rc < 0) + if (rc > 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared",
This seems like a spurious change.
@@ -752,13 +956,29 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) rc = -1;
- if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) - rc = -1; + if (def->os.kernel) { + i = virSecurityDACGetRememberedLabel(priv, def->os.kernel, &user, &group); + if (i < 0) + rc = -1;
Using 'i' for a non-iteration is unconventional; maybe 'rc' would be a better name.
@@ -815,11 +1035,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1;
if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.kernel) < 0) || + (virSecurityDACSetOwnership(def->os.kernel, user, group) < 0)))
Indentation looks off; and also redundant (): if (def->os.kernel && (virSecurityDACRememberLabel(priv, def->os.kernel) < 0 || virSecurityDACSetOwnership(def->os.kernel, user, group) < 0))
@@ -835,7 +1057,8 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, savefile) < 0))
Redundant () How is this information is preserved across a libvirtd restart? You'll need code that outputs private XML on the save side, then parses it back on the load side - is that later in the series? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 27.02.2013 01:23, Eric Blake wrote:
On 02/26/2013 09:08 AM, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Definitely important for a read-only file shared by more than one domain.
In order to not pollute domain XML, this info is kept in driver's private data in a hash table with path being key and pair <oldLabel, refcount> being value.
Makes sense.
Have you looked at what it would take to use ACLs to grant access to qemu without having to do a full-blown chown? That would also need to use the hash table to undo the ACL at the end of the day, and we would need to fall back to chown() on file systems where ACL doesn't work, but it certainly sounds like that would be sharing some of the work in this patch.
I haven't take look at that.
--- src/security/security_dac.c | 351 ++++++++++++++++++++++++++++++++++------- src/security/security_driver.h | 3 + 2 files changed, 296 insertions(+), 58 deletions(-)
Couple of questions below:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0b274b7..4b8f0a2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -42,8 +42,121 @@ struct _virSecurityDACData { uid_t user; gid_t group; bool dynamicOwnership; + virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ };
+struct _virOldLabel { + char *owner;
Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.
My idea is; store userName:groupName whenever possible; When one of them is not accessible, use +NNN instead. The rationale is - whenever a user or a gropu changes its ID, we will follow it. For instance, a file X has owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as A's ID is 2 not 1. That means I have to do parsing and all the virAsprintf magic. However, maybe this is not what we want and I should really remember just numeric values of IDs which has nice tradeoff - much simpler code.
+ int refCount; +}; + +static void virOldLabelFree(virOldLabelPtr label)
Line break after void.
+/** + * virSecurityDACRememberLabel: + * @priv: private DAC driver data + * @path: path which is about to be relabelled + * + * Store the original DAC label of @path. + * Returns: number of references of @path, or -1 on error + */ +static int +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, + const char *path) +{ + struct stat sb; + virOldLabelPtr oldLabel = NULL; + char *user = NULL, *group = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + + if (oldLabel) { + /* just increment ref counter */ + oldLabel->refCount++;
Is this threadsafe, or do we need to use virAtomic?
This in fact is thread safe since the security manager from layer just above locks itself before calling any security driver's method. So there's not locking visible here, as we rely on inherited sec driver's lock. But agreed that turning this into virAtomic does no harm. Moreover, we don't need to care for it if we ever drop the upper layer lock.
+ goto cleanup; + } + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("Unable to stat %s"), path); + goto cleanup; + } + + user = virGetUserName(sb.st_uid); + group = virGetGroupName(sb.st_gid); + if ((!user && (virAsprintf(&user, "+%ld", (long) sb.st_uid) < 0)) || + (!group && (virAsprintf(&group, "+%ld", (long) sb.st_gid) < 0)) ||
Given Philipp's recent series, it's probably better to mix %u and (unsigned), rather than %ld and (long) (that is, we've already asserted that uid_t fits within an int, and that unsigned is more likely than signed to represent all users except for the sentinel of -1, and -1 is not going to appear as stat() results).
Again, if your struct has a uid_t and gid_t instead of a char*, then you don't have to convert to string here.
+ (VIR_ALLOC(oldLabel) < 0) || + (virAsprintf(&oldLabel->owner, "%s:%s", user, group) < 0)) {
Worse, you are printing into two temporaries only to create yet another malloc'd string. If you do keep a char*, why not just:
virAsprintf(&oldLabel->owner, "+%u:+%u", (unsigned) sb.st_uid, (unsigned) sb.st_gid);
+ +static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr);
Any reason you can't topologically sort that function to be first, and avoid a forward declaration?
No. Will reorder.
+ +/** + * virSecurityDACGetRememberedLabel: + * @priv: private DAC driver data + * @path: path which we want to restore label on + * @user: original owner of @path + * @group: original owner of @path + * + * Decrements reference counter on @path. If this was the last + * reference, we need to restore the original label, in which + * case @user and @group are updated. + * Returns: the number of references of @path + * 0 if we need to restore the label + * -1 on error + */ +static int +virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t *user, + gid_t *group) +{ + int ret = -1; + virOldLabelPtr oldLabel = NULL; + + oldLabel = virHashLookup(priv->oldOwners, path); + if (!oldLabel) + goto cleanup; + + ret = --oldLabel->refCount;
Thread safety again. virAtomic?
+ + if (!ret) { + ret = parseIds(oldLabel->owner, user, group);
If the hash value struct has uid_t and gid_t fields, then no parsing is needed here.
static int -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACOpen(virSecurityManagerPtr mgr) { - return 0; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + priv->oldOwners = virHashCreate(0, hashDataFree); + return priv->oldOwners ? 0 : -1;
Caller doesn't report an error, so you need a virReportOOMError()
Aaah. Okay.
@@ -382,26 +503,14 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk, int migrated) { + int ret; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + uid_t user = 0; + gid_t group = 0;
I prefer -1 when initializing an unknown uid or gid; 0 has special meaning, and there are cases where we really do want to restore to uid 0.
Yeah, my intent was to keep things consistent with current libvirt behavior.
@@ -411,7 +520,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, */ if (migrated) { int rc = virStorageFileIsSharedFS(disk->src); - if (rc < 0) + if (rc > 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared",
This seems like a spurious change.
Yep. I don't know how it got there.
@@ -752,13 +956,29 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) rc = -1;
- if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) - rc = -1; + if (def->os.kernel) { + i = virSecurityDACGetRememberedLabel(priv, def->os.kernel, &user, &group); + if (i < 0) + rc = -1;
Using 'i' for a non-iteration is unconventional; maybe 'rc' would be a better name.
@@ -815,11 +1035,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1;
if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + ((virSecurityDACRememberLabel(priv, def->os.kernel) < 0) || + (virSecurityDACSetOwnership(def->os.kernel, user, group) < 0)))
Indentation looks off; and also redundant ():
if (def->os.kernel && (virSecurityDACRememberLabel(priv, def->os.kernel) < 0 || virSecurityDACSetOwnership(def->os.kernel, user, group) < 0))
@@ -835,7 +1057,8 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(def, priv, &user, &group) || + (virSecurityDACRememberLabel(priv, savefile) < 0))
Redundant ()
How is this information is preserved across a libvirtd restart? You'll need code that outputs private XML on the save side, then parses it back on the load side - is that later in the series?
Yes. That is what the next two patches do. Moreover, the design may look slightly like an overkill for now, but it is just for easier implementation for migrating the internal state of security driver. Michal
On 02/27/2013 02:25 AM, Michal Privoznik wrote:
Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.
My idea is; store userName:groupName whenever possible; When one of them is not accessible, use +NNN instead. The rationale is - whenever a user or a gropu changes its ID, we will follow it. For instance, a file X has owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as A's ID is 2 not 1. That means I have to do parsing and all the virAsprintf magic. However, maybe this is not what we want and I should really remember just numeric values of IDs which has nice tradeoff - much simpler code.
Migration and shared storage makes this problem so much tougher - the uid on shared storage is common, but the name is not necessarily common. I'd go for uid only; leave name lookups to each local machine connecting to shared storage, but don't store the name ourselves. Besides, admins tend not to change the name associated with a uid all that frequently (it's generally one-time setup). Dan has a point that you really need to involve the lock manager or use some persistent storage (extended attribute or additional file in the storage pool) alongside the file whose attributes we want to remember - if a file is on shared storage, then it is the responsibility of the last machine using the image to restore permissions, even if it was not the machine that first did a chmod(). Merely storing a hash in just a single libvirtd instance won't scale. Does our virlockd interface support attaching attributes to a file as part of locking it?
How is this information is preserved across a libvirtd restart? You'll need code that outputs private XML on the save side, then parses it back on the load side - is that later in the series?
Yes. That is what the next two patches do. Moreover, the design may look slightly like an overkill for now, but it is just for easier implementation for migrating the internal state of security driver.
I'm not quite sure I follow your plans for migration of security driver state. We migrate domains, not drivers - but you are storing persistent attributes on a driver, not per-domain state. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28.02.2013 02:22, Eric Blake wrote:
On 02/27/2013 02:25 AM, Michal Privoznik wrote:
Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.
My idea is; store userName:groupName whenever possible; When one of them is not accessible, use +NNN instead. The rationale is - whenever a user or a gropu changes its ID, we will follow it. For instance, a file X has owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as A's ID is 2 not 1. That means I have to do parsing and all the virAsprintf magic. However, maybe this is not what we want and I should really remember just numeric values of IDs which has nice tradeoff - much simpler code.
Migration and shared storage makes this problem so much tougher - the uid on shared storage is common, but the name is not necessarily common. I'd go for uid only; leave name lookups to each local machine connecting to shared storage, but don't store the name ourselves. Besides, admins tend not to change the name associated with a uid all that frequently (it's generally one-time setup).
Dan has a point that you really need to involve the lock manager or use some persistent storage (extended attribute or additional file in the storage pool) alongside the file whose attributes we want to remember - if a file is on shared storage, then it is the responsibility of the last machine using the image to restore permissions, even if it was not the machine that first did a chmod(). Merely storing a hash in just a single libvirtd instance won't scale. Does our virlockd interface support attaching attributes to a file as part of locking it?
Okay. You two has a point. I have not thought about that initially. Anyway, if I go with xattrs, would it be safe? I mean, I'd need to atomically get extended attribute for refCount. If there's none, get current owner of the file and store it - again - as an extended attribute among with refCount = 1. Any later libvirtd instance would then just atomically increment/decrement refCount. And here's where I see the trouble. In case of files protected by sanlock/virlockd we are safe - the relabeling is done after we obtain the lock. But what about files that are not protected by lock, e.g. <shared/> and <readonly/>? Unless we lock them for the time we do the xattr magic, we cannot guarantee atomicity, right? But this will extend need of usage of sanlock/virlockd to nearly all cases, which is something we might not want. Even if you have a couple of domains within a single host and you're sharing an ISO image you'd need the lock daemon. Would it suffice if my first draft does not deal with the explicit locking of files we don't lock now? We can assume some luck, right? :) Michal
On Fri, Mar 01, 2013 at 09:42:57AM +0100, Michal Privoznik wrote:
On 28.02.2013 02:22, Eric Blake wrote:
On 02/27/2013 02:25 AM, Michal Privoznik wrote:
Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.
My idea is; store userName:groupName whenever possible; When one of them is not accessible, use +NNN instead. The rationale is - whenever a user or a gropu changes its ID, we will follow it. For instance, a file X has owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as A's ID is 2 not 1. That means I have to do parsing and all the virAsprintf magic. However, maybe this is not what we want and I should really remember just numeric values of IDs which has nice tradeoff - much simpler code.
Migration and shared storage makes this problem so much tougher - the uid on shared storage is common, but the name is not necessarily common. I'd go for uid only; leave name lookups to each local machine connecting to shared storage, but don't store the name ourselves. Besides, admins tend not to change the name associated with a uid all that frequently (it's generally one-time setup).
Dan has a point that you really need to involve the lock manager or use some persistent storage (extended attribute or additional file in the storage pool) alongside the file whose attributes we want to remember - if a file is on shared storage, then it is the responsibility of the last machine using the image to restore permissions, even if it was not the machine that first did a chmod(). Merely storing a hash in just a single libvirtd instance won't scale. Does our virlockd interface support attaching attributes to a file as part of locking it?
Okay. You two has a point. I have not thought about that initially. Anyway, if I go with xattrs, would it be safe? I mean, I'd need to atomically get extended attribute for refCount. If there's none, get current owner of the file and store it - again - as an extended attribute among with refCount = 1. Any later libvirtd instance would then just atomically increment/decrement refCount. And here's where I see the trouble. In case of files protected by sanlock/virlockd we are safe - the relabeling is done after we obtain the lock. But what about files that are not protected by lock, e.g. <shared/> and <readonly/>? Unless we lock them for the time we do the xattr magic, we cannot guarantee atomicity, right? But this will extend need of usage of sanlock/virlockd to nearly all cases, which is something we might not want. Even if you have a couple of domains within a single host and you're sharing an ISO image you'd need the lock daemon.
Actually you can't rely on the existing sanlock/virtlockd support. While during VM startup, you are safe because the lock is held, for relabelling back to the original after shutdown no locks are protecting you. Basically you should think of the current lock support as protecting the *content* of files. We would need to use a separate lockspace to protect metadata (ie ownership/xtttr/etc). I would envisage this unconditionally using virtlockd, regardless of whether sanlock is used for protecting content. Regards, 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 Tue, Feb 26, 2013 at 05:23:18PM -0700, Eric Blake wrote:
On 02/26/2013 09:08 AM, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Definitely important for a read-only file shared by more than one domain.
In order to not pollute domain XML, this info is kept in driver's private data in a hash table with path being key and pair <oldLabel, refcount> being value.
Makes sense.
Have you looked at what it would take to use ACLs to grant access to qemu without having to do a full-blown chown? That would also need to use the hash table to undo the ACL at the end of the day, and we would need to fall back to chown() on file systems where ACL doesn't work, but it certainly sounds like that would be sharing some of the work in this patch.
Yep, independantly this patch we ought to make use of ACLs. It would remove a whole class of problems users experiance and make udev happier since it hates things chown'ing device nodes behind its back and has a tendancy to change them back at any moment. 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 Tue, Feb 26, 2013 at 05:08:40PM +0100, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Your patches don't deal with this scenario correctly I'm afraid. A shared file may be on NFS, so simply ref-counting inside libvirtd doesn't cut it. We need a ref count visible to all libvirtd instances that can see the file. My thought is that we ought to make use of an extended attribute for recording the ref count and original ownership. 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 27.02.2013 11:21, Daniel P. Berrange wrote:
On Tue, Feb 26, 2013 at 05:08:40PM +0100, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Your patches don't deal with this scenario correctly I'm afraid. A shared file may be on NFS, so simply ref-counting inside libvirtd doesn't cut it. We need a ref count visible to all libvirtd instances that can see the file. My thought is that we ought to make use of an extended attribute for recording the ref count and original ownership.
Daniel
Okay, but I think we should not deal with NFS at all. If a disk is shared libvirt should not event try to label it. And if so, then definitely not relabel it back. Michal
On Wed, Feb 27, 2013 at 11:30:31AM +0100, Michal Privoznik wrote:
On 27.02.2013 11:21, Daniel P. Berrange wrote:
On Tue, Feb 26, 2013 at 05:08:40PM +0100, Michal Privoznik wrote:
Currently, if we label a file to match qemu process DAC label, we do not store the original owner anywhere. So when relabeling back, the only option we have is to relabel to root:root which is obviously wrong.
However, bare remembering is not enough. We need to keep track of how many times we labeled a file so only the last restore chown()-s file back to the original owner.
Your patches don't deal with this scenario correctly I'm afraid. A shared file may be on NFS, so simply ref-counting inside libvirtd doesn't cut it. We need a ref count visible to all libvirtd instances that can see the file. My thought is that we ought to make use of an extended attribute for recording the ref count and original ownership.
Daniel
Okay, but I think we should not deal with NFS at all. If a disk is shared libvirt should not event try to label it. And if so, then definitely not relabel it back.
I completely disagree. We absolutely must correctly support shared filesystems, whether NFS, GFS or any number of FUSE cluster filesystems that are increasingly common. NFS is an exception in that its POSIX compliance is poor & root squash concept can fsck things up, but not all shared filesystems are so badly designed/limiting. 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 :|
In order to keep security driver's private data for future libvirtd restart, we must copy the behaviour of other drivers and create and load state XML file. This requires that the security driver is able to produce its internal state in XML form and parse it back then. These routines are called whenever security manager API is called, because the manager can't know if the driver's state has changed or not. The driver can decide to not update the XML in which case saveStatus method should return 0 and set @buf argument to NULL. The production of internal state XML can be enforced too. The saveStatus method obtains @force argument set to true in which case it should produce XML even though state hasn't change since last run. This can be handy during migration when transferring the internal state to the destination. The state XML file is kept in the state directory of parent hypervisor driver. --- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 1 + src/qemu/qemu_driver.c | 3 + src/security/security_driver.h | 9 +++ src/security/security_manager.c | 161 ++++++++++++++++++++++++++++++++++++++- src/security/security_manager.h | 2 + tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 3 +- 9 files changed, 180 insertions(+), 6 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 15aa334..fe50c2a 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1639,7 +1639,7 @@ int main(int argc, char *argv[]) ctrl->handshakeFd = handshakeFd; if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, - LXC_DRIVER_NAME, + LXC_DRIVER_NAME, NULL, false, false, false))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f136df2..9da2bd9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1387,6 +1387,7 @@ lxcSecurityInit(virLXCDriverPtr driver) VIR_INFO("lxcSecurityInit %s", driver->securityDriverName); virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, LXC_DRIVER_NAME, + driver->stateDir, false, driver->securityDefaultConfined, driver->securityRequireConfined); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e96915..3b28a84 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -320,6 +320,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) while (names && *names) { if (!(mgr = virSecurityManagerNew(*names, QEMU_DRIVER_NAME, + cfg->stateDir, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined))) @@ -337,6 +338,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) } else { if (!(mgr = virSecurityManagerNew(NULL, QEMU_DRIVER_NAME, + cfg->stateDir, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined))) @@ -348,6 +350,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) if (cfg->privileged) { if (!(mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME, + cfg->stateDir, cfg->user, cfg->group, cfg->allowDiskFormatProbing, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index d54f754..ee255a0 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -112,6 +112,12 @@ typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); +typedef int (*virSecurityDriverSaveStatus) (virSecurityManagerPtr mgr, + virBufferPtr *buf, + bool force); +typedef int (*virSecurityDriverLoadStatus) (virSecurityManagerPtr mgr, + xmlDocPtr xml); + struct _virSecurityDriver { size_t privateDataLen; const char *name; @@ -122,6 +128,9 @@ struct _virSecurityDriver { virSecurityDriverGetModel getModel; virSecurityDriverGetDOI getDOI; + virSecurityDriverSaveStatus saveStatus; + virSecurityDriverLoadStatus loadStatus; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainSetImageLabel domainSetSecurityImageLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c621366..521901a 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -42,6 +42,7 @@ struct _virSecurityManager { bool defaultConfined; bool requireConfined; const char *virtDriver; + char *stateDir; void *privateData; }; @@ -62,8 +63,109 @@ static int virSecurityManagerOnceInit(void) VIR_ONCE_GLOBAL_INIT(virSecurityManager); +static char * +virSecurityManagerGetStatusFile(const char *stateDir, + const char *drvName) +{ + char *ret = NULL; + + if (virAsprintf(&ret, "%s/%s.xml", stateDir, drvName) < 0) { + virReportOOMError(); + return NULL; + } + + return ret; +} + +static int +virSecurityManagerSaveStatusXML(virSecurityManagerPtr mgr, + const char *xml) +{ + int ret = -1; + char *statusFile = NULL; + + if (!(statusFile = virSecurityManagerGetStatusFile(mgr->stateDir, + mgr->drv->name))) + return ret; + + if (virFileMakePath(mgr->stateDir) < 0) { + virReportSystemError(errno, + _("cannot create status directory '%s'"), + mgr->stateDir); + goto cleanup; + } + + VIR_DEBUG("Saving state file %s for %s security driver", + statusFile, mgr->drv->name); + + ret = virXMLSaveFile(statusFile, NULL, NULL, xml); +cleanup: + VIR_FREE(statusFile); + return ret; +} + +static int +virSecurityManagerSaveStatus(virSecurityManagerPtr mgr) +{ + int ret = -1; + char *xml = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferPtr bufptr = &buf; + + if (!mgr->drv->saveStatus) + return 0; + + ret = mgr->drv->saveStatus(mgr, &bufptr, false); + + xml = virBufferContentAndReset(bufptr); + + if ((ret < 0) || !xml) + goto cleanup; + + if ((ret = virSecurityManagerSaveStatusXML(mgr, xml)) < 0) + goto cleanup; + + ret = 0; + +cleanup: + VIR_FREE(xml); + return ret; +} + +static int +virSecurityManagerLoadStatus(virSecurityManagerPtr mgr) +{ + int ret = -1; + char *statusFile = NULL; + xmlDocPtr xml; + + if (!mgr->drv->loadStatus) + return 0; + + if (!(statusFile = virSecurityManagerGetStatusFile(mgr->stateDir, + mgr->drv->name))) + return ret; + + if (!virFileExists(statusFile)) { + VIR_FREE(statusFile); + return 0; + } + + VIR_DEBUG("Loading state file %s for %s security driver", + statusFile, mgr->drv->name); + + if ((xml = virXMLParseFile(statusFile))) { + ret = mgr->drv->loadStatus(mgr, xml); + xmlFreeDoc(xml); + } + + VIR_FREE(statusFile); + return ret; +} + static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, const char *virtDriver, + const char *stateDir, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined) @@ -74,9 +176,9 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr if (virSecurityManagerInitialize() < 0) return NULL; - VIR_DEBUG("drv=%p (%s) virtDriver=%s allowDiskFormatProbing=%d " + VIR_DEBUG("drv=%p (%s) virtDriver=%s stateDir=%s allowDiskFormatProbing=%d " "defaultConfined=%d requireConfined=%d", - drv, drv->name, virtDriver, + drv, drv->name, virtDriver, stateDir, allowDiskFormatProbing, defaultConfined, requireConfined); @@ -97,11 +199,22 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr mgr->virtDriver = virtDriver; mgr->privateData = privateData; + if (stateDir && + (virAsprintf(&mgr->stateDir, "%s/security", stateDir) < 0)) { + virObjectUnref(mgr); + return NULL; + } + if (drv->open(mgr) < 0) { virObjectUnref(mgr); return NULL; } + if (virSecurityManagerLoadStatus(mgr) < 0) { + virObjectUnref(mgr); + return NULL; + } + return mgr; } @@ -110,6 +223,7 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary) virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, virSecurityManagerGetDriver(primary), + NULL, virSecurityManagerGetAllowDiskFormatProbing(primary), virSecurityManagerGetDefaultConfined(primary), virSecurityManagerGetRequireConfined(primary)); @@ -131,6 +245,7 @@ int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, } virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, + const char *stateDir, uid_t user, gid_t group, bool allowDiskFormatProbing, @@ -141,6 +256,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, + stateDir, allowDiskFormatProbing, defaultConfined, requireConfined); @@ -157,6 +273,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, + const char *stateDir, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined) @@ -187,6 +304,7 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, return virSecurityManagerNewDriver(drv, virtDriver, + stateDir, allowDiskFormatProbing, defaultConfined, requireConfined); @@ -225,6 +343,7 @@ static void virSecurityManagerDispose(void *obj) if (mgr->drv->close) mgr->drv->close(mgr); VIR_FREE(mgr->privateData); + VIR_FREE(mgr->stateDir); } const char * @@ -286,6 +405,8 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -301,6 +422,8 @@ int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityDaemonSocketLabel(mgr, vm); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -316,6 +439,8 @@ int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecuritySocketLabel(mgr, vm); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -331,6 +456,8 @@ int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainClearSecuritySocketLabel(mgr, vm); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -347,6 +474,8 @@ int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -364,6 +493,8 @@ int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev, vroot); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -381,6 +512,8 @@ int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityHostdevLabel(mgr, vm, dev, vroot); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -397,6 +530,8 @@ int virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -413,6 +548,8 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -471,6 +608,8 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, } cleanup: + if ((rc >= 0) && (virSecurityManagerSaveStatus(mgr) < 0)) + rc = -1; virObjectUnlock(mgr); VIR_FREE(sec_managers); return rc; @@ -484,6 +623,8 @@ int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainReserveSecurityLabel(mgr, vm, pid); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -499,6 +640,8 @@ int virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainReleaseSecurityLabel(mgr, vm); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -515,6 +658,8 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -531,6 +676,8 @@ int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -548,6 +695,8 @@ int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainGetSecurityProcessLabel(mgr, vm, pid, sec); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -563,6 +712,8 @@ int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityProcessLabel(mgr, vm); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -618,6 +769,8 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityImageFDLabel(mgr, vm, fd); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -634,6 +787,8 @@ int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityTapFDLabel(mgr, vm, fd); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } @@ -684,6 +839,8 @@ int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityHugepages(mgr, vm, path); + if (!ret) + ret = virSecurityManagerSaveStatus(mgr); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 711b354..bc87589 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -31,6 +31,7 @@ typedef virSecurityManager *virSecurityManagerPtr; virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, + const char *stateDir, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined); @@ -40,6 +41,7 @@ int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, virSecurityManagerPtr nested); virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, + const char *stateDir, uid_t user, gid_t group, bool allowDiskFormatProbing, diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index cd34b6b..f0eed63 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -17,7 +17,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) if (virThreadInitialize() < 0) return EXIT_FAILURE; - mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false); + mgr = virSecurityManagerNew(NULL, "QEMU", NULL, false, true, false); if (mgr == NULL) { fprintf(stderr, "Failed to start security driver"); return EXIT_FAILURE; diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 2454772..cc44e7d 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -314,7 +314,8 @@ mymain(void) { int ret = 0; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL, + false, true, false))) { virErrorPtr err = virGetLastError(); if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) return EXIT_AM_SKIP; diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index ba00010..a1f7ddb 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -271,7 +271,8 @@ mymain(void) int ret = 0; virSecurityManagerPtr mgr; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL, + false, true, false))) { virErrorPtr err = virGetLastError(); if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) return EXIT_AM_SKIP; -- 1.8.1.4
--- src/security/security_dac.c | 116 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8f0a2..f2d8c67 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -42,6 +42,7 @@ struct _virSecurityDACData { uid_t user; gid_t group; bool dynamicOwnership; + bool updated; /* has the state changed since last virSecurityDACSaveStatus? */ virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ }; @@ -65,6 +66,15 @@ hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) virOldLabelFree(payload); } +static void +hashDataToXML(void *payload, const void *name, void *data) +{ + virOldLabelPtr label = payload; + + virBufferAsprintf(data, "<label path='%s' owner='%s' refCount='%d'/>\n", + (const char *)name, label->owner, label->refCount); +} + /** * virSecurityDACRememberLabel: * @priv: private DAC driver data @@ -114,7 +124,7 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv, cleanup: VIR_FREE(user); VIR_FREE(group); - return oldLabel ? oldLabel->refCount : -1; + return oldLabel ? priv->updated = true, oldLabel->refCount : -1; } static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); @@ -147,6 +157,7 @@ virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, goto cleanup; ret = --oldLabel->refCount; + priv->updated = true; if (!ret) { ret = parseIds(oldLabel->owner, user, group); @@ -372,6 +383,106 @@ virSecurityDACClose(virSecurityManagerPtr mgr) return 0; } +static int +virSecurityDACSaveStatus(virSecurityManagerPtr mgr, + virBufferPtr *buf, + bool force) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + int ret = -1; + + if (!force && !priv->updated) { + *buf = NULL; + return 0; + } + + priv->updated = false; + + virBufferAddLit(*buf, "<securityDriver>\n"); + virBufferAddLit(*buf, " <oldLabel>\n"); + virBufferAdjustIndent(*buf, 4); + + ret = virHashForEach(priv->oldOwners, hashDataToXML, *buf); + + virBufferAdjustIndent(*buf, -4); + virBufferAddLit(*buf, " </oldLabel>\n"); + virBufferAddLit(*buf, "</securityDriver>\n"); + return ret; +} + +static int +virSecurityDACLoadStatus(virSecurityManagerPtr mgr, + xmlDocPtr doc) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + int i, n, ret = -1; + xmlNodePtr *nodes = NULL, root = xmlDocGetRootElement(doc); + xmlXPathContextPtr ctxt = NULL; + char *path = NULL,*owner = NULL, *refCountStr = NULL; + virOldLabelPtr oldLabel = NULL; + + if (!xmlStrEqual(root->name, BAD_CAST "securityDriver")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <securityDriver>"), + root->name); + return ret; + } + + if (!(ctxt = xmlXPathNewContext(doc))) { + virReportOOMError(); + return ret; + } + + ctxt->node = root; + + n = virXPathNodeSet("./oldLabel/label", ctxt, &nodes); + if (n < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + path = virXMLPropString(nodes[i], "path"); + owner = virXMLPropString(nodes[i], "owner"); + refCountStr = virXMLPropString(nodes[i], "refCount"); + + if (!path || !owner || !refCountStr) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Malformed security driver xml")); + goto cleanup; + } + if (VIR_ALLOC(oldLabel) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virStrToLong_i(refCountStr, NULL, 10, &oldLabel->refCount) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("Malformed refCount attribute: %s"), + refCountStr); + goto cleanup; + } + + oldLabel->owner = owner; + owner = NULL; + + if (virHashUpdateEntry(priv->oldOwners, path, oldLabel) < 0) + goto cleanup; + + path = NULL; + oldLabel = NULL; + VIR_FREE(refCountStr); + } + + ret = 0; +cleanup: + virOldLabelFree(oldLabel); + VIR_FREE(path); + VIR_FREE(owner); + VIR_FREE(refCountStr); + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + return ret; +} static const char * virSecurityDACGetModel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { @@ -1304,6 +1415,9 @@ virSecurityDriver virSecurityDriverDAC = { .open = virSecurityDACOpen, .close = virSecurityDACClose, + .saveStatus = virSecurityDACSaveStatus, + .loadStatus = virSecurityDACLoadStatus, + .getModel = virSecurityDACGetModel, .getDOI = virSecurityDACGetDOI, -- 1.8.1.4
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik