[libvirt] [PATCH 5/7] Update QEMU driver to support multiple security drivers
Michal Privoznik
mprivozn at redhat.com
Tue Jun 5 15:01:01 UTC 2012
On 21.05.2012 15:39, Marcelo Cerri wrote:
> ---
> src/driver.h | 8 ++-
> src/qemu/qemu_conf.c | 33 ++++++++
> src/qemu/qemu_conf.h | 1 +
> src/qemu/qemu_driver.c | 196 ++++++++++++++++++++++++++++++++++++++---------
> src/qemu/qemu_process.c | 53 +++++++++----
> 5 files changed, 236 insertions(+), 55 deletions(-)
Changes to driver.h looks suspicious :) ...
>
> diff --git a/src/driver.h b/src/driver.h
> index 03d249b..ca4927f 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -305,11 +305,14 @@ typedef int
> int maplen);
> typedef int
> (*virDrvDomainGetMaxVcpus) (virDomainPtr domain);
> -
> typedef int
> - (*virDrvDomainGetSecurityLabel) (virDomainPtr domain,
> + (*virDrvDomainGetSecurityLabel) (virDomainPtr domain,
> virSecurityLabelPtr seclabel);
> typedef int
> + (*virDrvDomainGetSecurityLabelList) (virDomainPtr domain,
> + virSecurityLabelPtr seclabels,
> + int nseclabels);
> +typedef int
> (*virDrvNodeGetSecurityModel) (virConnectPtr conn,
> virSecurityModelPtr secmodel);
> typedef int
> @@ -911,6 +914,7 @@ struct _virDriver {
> virDrvDomainGetVcpus domainGetVcpus;
> virDrvDomainGetMaxVcpus domainGetMaxVcpus;
> virDrvDomainGetSecurityLabel domainGetSecurityLabel;
> + virDrvDomainGetSecurityLabelList domainGetSecurityLabelList;
> virDrvNodeGetSecurityModel nodeGetSecurityModel;
> virDrvDomainGetXMLDesc domainGetXMLDesc;
> virDrvConnectDomainXMLFromNative domainXMLFromNative;
... but reasonable after all. Although it may be better to save
formatting cleanups for another patch - leaving us with more easier to
understand git bisects in the future.
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 88a04bc..5cc2071 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -202,6 +202,39 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
> }
> }
>
> + p = virConfGetValue (conf, "additional_security_drivers");
> + CHECK_TYPE ("additional_security_driver", VIR_CONF_STRING);
> + if (p && p->str) {
> + char *it, *tok;
> + size_t len;
> +
> + for (len = 1, it = p->str; *it; it++)
> + len++;
> + if (VIR_ALLOC_N(driver->additionalSecurityDriverNames, len + 1) < 0) {
> + virReportOOMError();
> + virConfFree(conf);
> + return -1;
> + }
I'd say drop this ^^ ...
> +
> + i = 0;
> + tok = it = p->str;
> + while (1) {
> + if (*it == ',' || *it == '\0') {
> + driver->additionalSecurityDriverNames[i] = strndup(tok, it - tok);
... and expand this array only if needed.
> + if (driver->additionalSecurityDriverNames == NULL) {
> + virReportOOMError();
> + virConfFree(conf);
> + return -1;
> + }
> + tok = it + 1;
> + i++;
> + }
> + if (*it == '\0')
> + break;
> + it++;
> + }
> + }
> +
> p = virConfGetValue (conf, "security_default_confined");
> CHECK_TYPE ("security_default_confined", VIR_CONF_LONG);
> if (p) driver->securityDefaultConfined = p->l;
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 482e6d3..a5867b6 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -117,6 +117,7 @@ struct qemud_driver {
> virDomainEventStatePtr domainEventState;
>
> char *securityDriverName;
> + char **additionalSecurityDriverNames;
> bool securityDefaultConfined;
> bool securityRequireConfined;
> virSecurityManagerPtr securityManager;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index efbc421..39d9eee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -214,36 +214,84 @@ qemuAutostartDomains(struct qemud_driver *driver)
> static int
> qemuSecurityInit(struct qemud_driver *driver)
> {
> - virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName,
> - QEMU_DRIVER_NAME,
> - driver->allowDiskFormatProbing,
> - driver->securityDefaultConfined,
> - driver->securityRequireConfined);
> -
> + char **names;
> + virSecurityManagerPtr mgr, nested, stack;
> +
> + /* Create primary driver */
> + mgr = virSecurityManagerNew(driver->securityDriverName,
> + QEMU_DRIVER_NAME,
> + driver->allowDiskFormatProbing,
> + driver->securityDefaultConfined,
> + driver->securityRequireConfined);
> if (!mgr)
> goto error;
>
> - if (driver->privileged) {
> - virSecurityManagerPtr dac = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> - driver->user,
> - driver->group,
> - driver->allowDiskFormatProbing,
> - driver->securityDefaultConfined,
> - driver->securityRequireConfined,
> - driver->dynamicOwnership);
> - if (!dac)
> + /* If a DAC driver is required or additional drivers are provived, a stack
> + * driver should be create to group them all */
> + if (driver->privileged || driver->additionalSecurityDriverNames) {
> + stack = virSecurityManagerNewStack(mgr);
> + if (!stack)
> goto error;
> + mgr = stack;
> + }
> +
> + /* Loop through additional driver names and add a secudary driver to each
> + * one */
> + if (driver->additionalSecurityDriverNames) {
> + names = driver->additionalSecurityDriverNames;
> + while (names && *names) {
> + if (STREQ("dac", *names)) {
> + /* A DAC driver has specic parameters */
> + nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> + driver->user,
> + driver->group,
> + driver->allowDiskFormatProbing,
> + driver->securityDefaultConfined,
> + driver->securityRequireConfined,
> + driver->dynamicOwnership);
> + } else {
> + nested = virSecurityManagerNew(*names,
> + QEMU_DRIVER_NAME,
> + driver->allowDiskFormatProbing,
> + driver->securityDefaultConfined,
> + driver->securityRequireConfined);
> + }
> + if (nested == NULL)
> + goto error;
> + if (virSecurityManagerStackAddNested(stack, nested))
> + goto error;
> + names++;
> + }
> + }
>
> - if (!(driver->securityManager = virSecurityManagerNewStack(mgr,
> - dac))) {
> -
> - virSecurityManagerFree(dac);
> - goto error;
> + if (driver->privileged) {
> + /* When a DAC driver is required, check if there is already one in the
> + * additional drivers */
> + names = driver->additionalSecurityDriverNames;
> + while (names && *names) {
> + if (STREQ("dac", *names)) {
> + break;
> + }
> + names++;
> + }
> + /* If there isn't a DAC driver, create a new one and add it to the stack
> + * manager */
> + if (names == NULL || *names == NULL) {
> + nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
> + driver->user,
> + driver->group,
> + driver->allowDiskFormatProbing,
> + driver->securityDefaultConfined,
> + driver->securityRequireConfined,
> + driver->dynamicOwnership);
> + if (nested == NULL)
> + goto error;
> + if (virSecurityManagerStackAddNested(stack, nested))
> + goto error;
> }
> - } else {
> - driver->securityManager = mgr;
> }
>
> + driver->securityManager = mgr;
> return 0;
>
> error:
> @@ -257,7 +305,11 @@ static virCapsPtr
> qemuCreateCapabilities(virCapsPtr oldcaps,
> struct qemud_driver *driver)
> {
> + size_t i;
> virCapsPtr caps;
> + virSecurityManagerPtr *sec_managers = NULL;
> + /* Security driver data */
> + const char *doi, *model;
>
> /* Basic host arch / guest machine capabilities */
> if (!(caps = qemuCapsInit(oldcaps))) {
> @@ -282,26 +334,38 @@ qemuCreateCapabilities(virCapsPtr oldcaps,
> goto err_exit;
> }
>
> - /* Security driver data */
> - const char *doi, *model;
> + /* access sec drivers and create a sec model to each one */
> + sec_managers = virSecurityManagerGetNested(driver->securityManager);
> + if (sec_managers == NULL) {
> + goto err_exit;
> + }
> +
> + /* calculate length */
> + for (i = 0; sec_managers[i]; i++)
> + ;
> + caps->host.nsecModels = i;
>
> - doi = virSecurityManagerGetDOI(driver->securityManager);
> - model = virSecurityManagerGetModel(driver->securityManager);
> - if (STRNEQ(model, "none")) {
> - if (!(caps->host.secModel.model = strdup(model)))
> + if (VIR_ALLOC_N(caps->host.secModels, caps->host.nsecModels) < 0)
> + goto no_memory;
> +
> + for (i = 0; sec_managers[i]; i++) {
> + doi = virSecurityManagerGetDOI(sec_managers[i]);
> + model = virSecurityManagerGetModel(sec_managers[i]);
> + if (!(caps->host.secModels[i].model = strdup(model)))
> goto no_memory;
> - if (!(caps->host.secModel.doi = strdup(doi)))
> + if (!(caps->host.secModels[i].doi = strdup(doi)))
> goto no_memory;
> + VIR_DEBUG("Initialized caps for security driver \"%s\" with "
> + "DOI \"%s\"", model, doi);
> }
> -
> - VIR_DEBUG("Initialized caps for security driver \"%s\" with "
> - "DOI \"%s\"", model, doi);
> + VIR_FREE(sec_managers);
>
> return caps;
>
> no_memory:
> virReportOOMError();
> err_exit:
> + VIR_FREE(sec_managers);
> virCapabilitiesFree(caps);
> return NULL;
> }
> @@ -3958,6 +4022,63 @@ cleanup:
> return ret;
> }
>
> +static int qemudDomainGetSecurityLabelList(virDomainPtr dom,
> + virSecurityLabelPtr seclabel,
> + int nseclabels)
> +{
> + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> + virDomainObjPtr vm;
> + int i, ret = -1;
> +
> + qemuDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +
Do we really want to hold driver locked over the whole API?
> + memset(seclabel, 0, sizeof(*seclabel));
> +
> + if (!vm) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virUUIDFormat(dom->uuid, uuidstr);
> + qemuReportError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching uuid '%s'"), uuidstr);
> + goto cleanup;
> + }
> +
> + if (!virDomainVirtTypeToString(vm->def->virtType)) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown virt type in domain definition '%d'"),
> + vm->def->virtType);
> + goto cleanup;
> + }
> +
> + /*
> + * Check the comment in qemudDomainGetSecurityLabel function.
> + */
> + if (virDomainObjIsActive(vm)) {
> + virSecurityManagerPtr* mgrs = virSecurityManagerGetNested(
> + driver->securityManager);
> + if (!mgrs)
> + goto cleanup;
> +
> + for (i = 0; mgrs[i] && i < nseclabels; i++) {
> + if (virSecurityManagerGetProcessLabel(mgrs[i], vm->def, vm->pid,
> + seclabel) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Failed to get security label"));
> + VIR_FREE(mgrs);
> + goto cleanup;
> + }
> + }
> + VIR_FREE(mgrs);
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> + return ret;
> +}
> static int qemudNodeGetSecurityModel(virConnectPtr conn,
> virSecurityModelPtr secmodel)
> {
> @@ -3968,12 +4089,12 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn,
> qemuDriverLock(driver);
> memset(secmodel, 0, sizeof(*secmodel));
>
> - /* NULL indicates no driver, which we treat as
> - * success, but simply return no data in *secmodel */
> - if (driver->caps->host.secModel.model == NULL)
> + /* We treat no driver as success, but simply return no data in *secmodel */
> + if (driver->caps->host.nsecModels == 0 ||
> + driver->caps->host.secModels[0].model == NULL)
> goto cleanup;
>
> - p = driver->caps->host.secModel.model;
> + p = driver->caps->host.secModels[0].model;
> if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) {
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
> _("security model string exceeds max %d bytes"),
> @@ -3983,7 +4104,7 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn,
> }
> strcpy(secmodel->model, p);
>
> - p = driver->caps->host.secModel.doi;
> + p = driver->caps->host.secModels[0].doi;
> if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) {
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
> _("security DOI string exceeds max %d bytes"),
> @@ -13004,6 +13125,7 @@ static virDriver qemuDriver = {
> .domainGetVcpus = qemudDomainGetVcpus, /* 0.4.4 */
> .domainGetMaxVcpus = qemudDomainGetMaxVcpus, /* 0.4.4 */
> .domainGetSecurityLabel = qemudDomainGetSecurityLabel, /* 0.6.1 */
> + .domainGetSecurityLabelList = qemudDomainGetSecurityLabelList, /* ? */
> .nodeGetSecurityModel = qemudNodeGetSecurityModel, /* 0.6.1 */
> .domainGetXMLDesc = qemuDomainGetXMLDesc, /* 0.2.0 */
> .domainXMLFromNative = qemuDomainXMLFromNative, /* 0.6.4 */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 58ba5bf..59c7e0d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3979,12 +3979,12 @@ void qemuProcessStop(struct qemud_driver *driver,
> virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
>
> /* Clear out dynamically assigned labels */
> - if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> - if (!vm->def->seclabel.baselabel)
> - VIR_FREE(vm->def->seclabel.model);
> - VIR_FREE(vm->def->seclabel.label);
> + for (i = 0; i < vm->def->nseclabels; i++) {
> + if (vm->def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> + VIR_FREE(vm->def->seclabels[i]->label);
> + }
> + VIR_FREE(vm->def->seclabels[i]->imagelabel);
> }
> - VIR_FREE(vm->def->seclabel.imagelabel);
>
> virDomainDefClearDeviceAliases(vm->def);
> if (!priv->persistentAddrs) {
> @@ -4088,6 +4088,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> virDomainChrSourceDefPtr monConfig,
> bool monJSON)
> {
> + size_t i;
> char ebuf[1024];
> int logfile = -1;
> char *timestamp;
> @@ -4095,6 +4096,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> bool running = true;
> virDomainPausedReason reason;
> virSecurityLabelPtr seclabel = NULL;
> + virSecurityLabelDefPtr seclabeldef = NULL;
> + virSecurityManagerPtr* sec_managers;
> + const char *model;
>
> VIR_DEBUG("Beginning VM attach process");
>
> @@ -4127,17 +4131,35 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> goto no_memory;
>
> VIR_DEBUG("Detect security driver config");
> - vm->def->seclabel.type = VIR_DOMAIN_SECLABEL_STATIC;
> - if (VIR_ALLOC(seclabel) < 0)
> - goto no_memory;
> - if (virSecurityManagerGetProcessLabel(driver->securityManager,
> - vm->def, vm->pid, seclabel) < 0)
> + sec_managers = virSecurityManagerGetNested(driver->securityManager);
> + if (sec_managers == NULL) {
> goto cleanup;
> - if (driver->caps->host.secModel.model &&
> - !(vm->def->seclabel.model = strdup(driver->caps->host.secModel.model)))
> - goto no_memory;
> - if (!(vm->def->seclabel.label = strdup(seclabel->label)))
> - goto no_memory;
> + }
> +
> + for (i = 0; sec_managers[i]; i++) {
> + model = virSecurityManagerGetModel(sec_managers[i]);
> + seclabeldef = virDomainDefGetSecurityLabelDef(vm->def, model);
> + if (seclabeldef == NULL) {
> + goto cleanup;
> + }
> + seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC;
> + if (VIR_ALLOC(seclabel) < 0)
> + goto no_memory;
> + if (virSecurityManagerGetProcessLabel(driver->securityManager,
> + vm->def, vm->pid, seclabel) < 0)
> + goto cleanup;
> +
> + //if (driver->caps->host.secModel.model &&
> + // !(seclabeldef.model = strdup(driver->caps->host.secModel.model)))
> + // goto no_memory;
> + if (!(seclabeldef->model = strdup(model)))
> + goto no_memory;
> +
> + if (!(seclabeldef->label = strdup(seclabel->label)))
> + goto no_memory;
> + VIR_FREE(seclabel);
> + seclabel = NULL;
> + }
>
> VIR_DEBUG("Creating domain log file");
> if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0)
> @@ -4256,7 +4278,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> goto cleanup;
>
> VIR_FORCE_CLOSE(logfile);
> - VIR_FREE(seclabel);
>
> return 0;
>
More information about the libvir-list
mailing list