[PATCH 0/9] Fix image labels lost after migration with shared fs
When migrating using shared fs, the dst host will remember the image labels again before launching the VM and the src host will remove the image labels after migration. It leads to that the image labels lost after migration. Patch 7-9 are the real fix patches. Peng Liang (9): security: add virSecurityUpdateTimestampIfexists security: add virSecurityManagerUpdateImageLabel security: implement domainUpdateSecurityImageLabel for stack security: implement domainUpdateSecurityImageLabel for DAC qemu: add qemuSecurityUpdateImageLabel security: rename virSecurityDACSetImageLabelInternal to virSecurityDACSetImageLabelSingle migration: don't remember image labels when migrating with shared fs migration: don't remove image labels after migration migration: update image labels in dst after migration src/libvirt_private.syms | 1 + src/qemu/qemu_migration.c | 12 +++++++ src/qemu/qemu_process.c | 5 ++- src/qemu/qemu_security.c | 10 ++++++ src/qemu/qemu_security.h | 5 +++ src/security/security_dac.c | 58 +++++++++++++++++++++++++-------- src/security/security_driver.h | 5 +++ src/security/security_manager.c | 29 +++++++++++++++++ src/security/security_manager.h | 5 +++ src/security/security_stack.c | 20 ++++++++++++ src/security/security_util.c | 32 ++++++++++++++++++ src/security/security_util.h | 4 +++ 12 files changed, 172 insertions(+), 14 deletions(-) -- 2.31.1
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_util.c | 32 ++++++++++++++++++++++++++++++++ src/security/security_util.h | 4 ++++ 2 files changed, 36 insertions(+) diff --git a/src/security/security_util.c b/src/security/security_util.c index 26a7861e2935..1b5ba2b92b09 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -227,6 +227,38 @@ virSecurityAddTimestamp(const char *name, } +/** + * virSecurityUpdateTimestampIfexists: + * @name: security driver name + * @path: file name + * + * Update timestamp of @path for given security driver (@name) if the timestamp + * of @path exists. + * + * Returns: 0 on success, + * 1 if timestamp of @path doesn't exist, + * -1 otherwise. + */ +int +virSecurityUpdateTimestampIfexists(const char *name, + const char *path) +{ + g_autofree char *timestamp_name = NULL; + g_autofree char *timestamp_value = NULL; + g_autofree char *old_value = NULL; + + if (!(timestamp_value = virSecurityGetTimestamp()) || + !(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + if (virFileGetXAttrQuiet(path, timestamp_name, &old_value) < 0) { + return 1; + } + + return virFileSetXAttr(path, timestamp_name, timestamp_value); +} + + static int virSecurityRemoveTimestamp(const char *name, const char *path) diff --git a/src/security/security_util.h b/src/security/security_util.h index 7af6f009e2ca..b66541fd92c5 100644 --- a/src/security/security_util.h +++ b/src/security/security_util.h @@ -33,5 +33,9 @@ virSecurityMoveRememberedLabel(const char *name, const char *src, const char *dst); +int +virSecurityUpdateTimestampIfexists(const char *name, + const char *path); + bool virSecurityXATTRNamespaceDefined(void); -- 2.31.1
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 29 +++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ 4 files changed, 40 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 090ac80691cb..50c4d0fd000a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1707,6 +1707,7 @@ virSecurityManagerStackAddNested; virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; virSecurityManagerTransactionStart; +virSecurityManagerUpdateImageLabel; virSecurityManagerVerify; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 07f8def3d3c6..82ef0ddd9801 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -123,6 +123,10 @@ typedef int (*virSecurityDomainMoveImageMetadata) (virSecurityManager *mgr, pid_t pid, virStorageSource *src, virStorageSource *dst); +typedef int (*virSecurityDomainUpdateImageLabel) (virSecurityManager *mgr, + virDomainDef *def, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags); typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManager *mgr, virDomainDef *def, virDomainMemoryDef *mem); @@ -180,6 +184,7 @@ struct _virSecurityDriver { virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; virSecurityDomainMoveImageMetadata domainMoveImageMetadata; + virSecurityDomainUpdateImageLabel domainUpdateSecurityImageLabel; virSecurityDomainSetMemoryLabel domainSetSecurityMemoryLabel; virSecurityDomainRestoreMemoryLabel domainRestoreSecurityMemoryLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9906c1691d0f..b580704d3abf 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -476,6 +476,35 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, } +/** + * virSecurityManagerUpdateImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' + * + * Update security label from @src according to @flags. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + if (mgr->drv->domainUpdateSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainUpdateSecurityImageLabel(mgr, vm, src, flags); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + + int virSecurityManagerSetDaemonSocketLabel(virSecurityManager *mgr, virDomainDef *vm) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 57047ccb137d..00bbb255538f 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -175,6 +175,11 @@ int virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, pid_t pid, virStorageSource *src, virStorageSource *dst); +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags); int virSecurityManagerSetMemoryLabel(virSecurityManager *mgr, virDomainDef *vm, -- 2.31.1
On 8/23/21 4:41 AM, Peng Liang wrote:
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 29 +++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ 4 files changed, 40 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9906c1691d0f..b580704d3abf 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -476,6 +476,35 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, }
+/** + * virSecurityManagerUpdateImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' + * + * Update security label from @src according to @flags. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + if (mgr->drv->domainUpdateSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainUpdateSecurityImageLabel(mgr, vm, src, flags); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + +
Is there a reason why this needs to be inside virSecurityManager? We already have virSecurityMoveRememberedLabel() that lives outside of it, in security_util.c and conceptually this function belongs there. Michal
On 9/9/2021 7:01 PM, Michal Prívozník wrote:
On 8/23/21 4:41 AM, Peng Liang wrote:
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 29 +++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ 4 files changed, 40 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9906c1691d0f..b580704d3abf 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -476,6 +476,35 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, }
+/** + * virSecurityManagerUpdateImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' + * + * Update security label from @src according to @flags. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + if (mgr->drv->domainUpdateSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainUpdateSecurityImageLabel(mgr, vm, src, flags); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + +
Is there a reason why this needs to be inside virSecurityManager? We already have virSecurityMoveRememberedLabel() that lives outside of it, in security_util.c and conceptually this function belongs there.
Michal
.
Maybe all security managers' labels need to be updated during migration, so I add it here. Thanks, Peng
On 9/9/21 1:45 PM, Peng Liang wrote:
On 9/9/2021 7:01 PM, Michal Prívozník wrote:
On 8/23/21 4:41 AM, Peng Liang wrote:
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 29 +++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ 4 files changed, 40 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9906c1691d0f..b580704d3abf 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -476,6 +476,35 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, }
+/** + * virSecurityManagerUpdateImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' + * + * Update security label from @src according to @flags. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + if (mgr->drv->domainUpdateSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainUpdateSecurityImageLabel(mgr, vm, src, flags); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + +
Is there a reason why this needs to be inside virSecurityManager? We already have virSecurityMoveRememberedLabel() that lives outside of it, in security_util.c and conceptually this function belongs there.
Michal
.
Maybe all security managers' labels need to be updated during migration, so I add it here.
Ah, you are correct. The timestamp XATTR is specific to secdriver so DAC and SELinux have their own timestamps. So your approach is in fact correct. For your v2 can you please also implement SELinux? I think it's going to be 1:1 copy of DAC code. Michal
On 9/9/2021 10:47 PM, Michal Prívozník wrote:
On 9/9/21 1:45 PM, Peng Liang wrote:
On 9/9/2021 7:01 PM, Michal Prívozník wrote:
On 8/23/21 4:41 AM, Peng Liang wrote:
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 29 +++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ 4 files changed, 40 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9906c1691d0f..b580704d3abf 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -476,6 +476,35 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, }
+/** + * virSecurityManagerUpdateImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' + * + * Update security label from @src according to @flags. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + if (mgr->drv->domainUpdateSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainUpdateSecurityImageLabel(mgr, vm, src, flags); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + +
Is there a reason why this needs to be inside virSecurityManager? We already have virSecurityMoveRememberedLabel() that lives outside of it, in security_util.c and conceptually this function belongs there.
Michal
.
Maybe all security managers' labels need to be updated during migration, so I add it here.
Ah, you are correct. The timestamp XATTR is specific to secdriver so DAC and SELinux have their own timestamps. So your approach is in fact correct. For your v2 can you please also implement SELinux? I think it's going to be 1:1 copy of DAC code.
Michal
.
OK，I'll add and test it in v2. Thanks for your reviewing! Peng
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_stack.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/security/security_stack.c b/src/security/security_stack.c index f7a9ed1e33a3..490238a92511 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -706,6 +706,25 @@ virSecurityStackMoveImageMetadata(virSecurityManager *mgr, return rc; } +static int +virSecurityStackUpdateImageLabel(virSecurityManager *mgr, + virDomainDef *vm, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) +{ + virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItem *item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerUpdateImageLabel(item->securityManager, + vm, src, flags) < 0) + rc = -1; + } + + return rc; +} + static int virSecurityStackSetMemoryLabel(virSecurityManager *mgr, virDomainDef *vm, @@ -984,6 +1003,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityImageLabel = virSecurityStackSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityStackRestoreImageLabel, .domainMoveImageMetadata = virSecurityStackMoveImageMetadata, + .domainUpdateSecurityImageLabel = virSecurityStackUpdateImageLabel, .domainSetSecurityMemoryLabel = virSecurityStackSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecurityStackRestoreMemoryLabel, -- 2.31.1
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_dac.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 04b9ecf02877..217fed203063 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1132,6 +1132,23 @@ virSecurityDACMoveImageMetadata(virSecurityManager *mgr, } +static int +virSecurityDACUpdateImageLabel(virSecurityManager *mgr G_GNUC_UNUSED, + virDomainDef *def G_GNUC_UNUSED, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) +{ + virStorageSource *n; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virSecurityUpdateTimestampIfexists(SECURITY_DAC_NAME, src->path) < 0) + return -1; + } + + return 0; +} + + static int virSecurityDACSetHostdevLabelHelper(const char *file, bool remember, @@ -2537,6 +2554,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityImageLabel = virSecurityDACSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreImageLabel, .domainMoveImageMetadata = virSecurityDACMoveImageMetadata, + .domainUpdateSecurityImageLabel = virSecurityDACUpdateImageLabel, .domainSetSecurityMemoryLabel = virSecurityDACSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecurityDACRestoreMemoryLabel, -- 2.31.1
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_security.c | 10 ++++++++++ src/qemu/qemu_security.h | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index e582a66071a1..eb211a9b0c4c 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -188,6 +188,16 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver, } +int +qemuSecurityUpdateImageLabel(virQEMUDriver *driver, + virDomainObj *vm, + virStorageSource *src) +{ + return virSecurityManagerUpdateImageLabel(driver->securityManager, vm->def, + src, 0); +} + + int qemuSecuritySetHostdevLabel(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 4c3d81e4b5e2..ac4acc583264 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -49,6 +49,11 @@ int qemuSecurityMoveImageMetadata(virQEMUDriver *driver, virStorageSource *src, virStorageSource *dst); +int +qemuSecurityUpdateImageLabel(virQEMUDriver *driver, + virDomainObj *vm, + virStorageSource *src); + int qemuSecuritySetHostdevLabel(virQEMUDriver *driver, virDomainObj *vm, virDomainHostdevDef *hostdev); -- 2.31.1
virSecurityDACSetImageLabelInt will be added in the next patch, so rename virSecurityDACSetImageLabelInternal to virSecurityDACSetImageLabelSingle to avoid confusion and keep consistent with virSecurityDACRestoreImageLabelInt and virSecurityDACRestoreImageLabelSingle. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_dac.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 217fed203063..b16552b2559e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -863,11 +863,11 @@ virSecurityDACRestoreFileLabel(virSecurityManager *mgr, static int -virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, - virDomainDef *def, - virStorageSource *src, - virStorageSource *parent, - bool isChainTop) +virSecurityDACSetImageLabelSingle(virSecurityManager *mgr, + virDomainDef *def, + virStorageSource *src, + virStorageSource *parent, + bool isChainTop) { virSecurityLabelDef *secdef; virSecurityDeviceLabelDef *disk_seclabel; @@ -949,7 +949,7 @@ virSecurityDACSetImageLabelRelative(virSecurityManager *mgr, for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP; - if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) + if (virSecurityDACSetImageLabelSingle(mgr, def, n, parent, isChainTop) < 0) return -1; if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) -- 2.31.1
When migrating with shared fs, the image labels has been remembered and the ownership of the image has been set in the src host. If the dst host remembers the ownership of the image again, the ownership of the image remembered in the src host (the origin ownership) will lost. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_dac.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b16552b2559e..bd342fd20312 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -867,7 +867,8 @@ virSecurityDACSetImageLabelSingle(virSecurityManager *mgr, virDomainDef *def, virStorageSource *src, virStorageSource *parent, - bool isChainTop) + bool isChainTop, + bool migrated) { virSecurityLabelDef *secdef; virSecurityDeviceLabelDef *disk_seclabel; @@ -931,7 +932,8 @@ virSecurityDACSetImageLabelSingle(virSecurityManager *mgr, * but the top layer, or read only image, or disk explicitly * marked as shared. */ - remember = isChainTop && !src->readonly && !src->shared; + remember = isChainTop && !src->readonly && !src->shared && + !(migrated && virFileIsSharedFS(src->path) > 0); return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember); } @@ -942,14 +944,15 @@ virSecurityDACSetImageLabelRelative(virSecurityManager *mgr, virDomainDef *def, virStorageSource *src, virStorageSource *parent, - virSecurityDomainImageLabelFlags flags) + virSecurityDomainImageLabelFlags flags, + bool migrated) { virStorageSource *n; for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP; - if (virSecurityDACSetImageLabelSingle(mgr, def, n, parent, isChainTop) < 0) + if (virSecurityDACSetImageLabelSingle(mgr, def, n, parent, isChainTop, migrated) < 0) return -1; if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) @@ -961,13 +964,23 @@ virSecurityDACSetImageLabelRelative(virSecurityManager *mgr, return 0; } +static int +virSecurityDACSetImageLabelInt(virSecurityManager *mgr, + virDomainDef *def, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags, + bool migrated) +{ + return virSecurityDACSetImageLabelRelative(mgr, def, src, src, flags, migrated); +} + static int virSecurityDACSetImageLabel(virSecurityManager *mgr, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags) { - return virSecurityDACSetImageLabelRelative(mgr, def, src, src, flags); + return virSecurityDACSetImageLabelInt(mgr, def, src, flags, false); } static int @@ -2116,7 +2129,7 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, virDomainDef *def, const char *incomingPath G_GNUC_UNUSED, bool chardevStdioLogd, - bool migrated G_GNUC_UNUSED) + bool migrated) { virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDef *secdef; @@ -2138,9 +2151,10 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, /* XXX fixme - we need to recursively label the entire tree :-( */ if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) continue; - if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src, - VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | - VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0) + if (virSecurityDACSetImageLabelInt(mgr, def, def->disks[i]->src, + VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | + VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP, + migrated) < 0) return -1; } -- 2.31.1
On 8/23/21 4:41 AM, Peng Liang wrote:
When migrating with shared fs, the image labels has been remembered and the ownership of the image has been set in the src host. If the dst host remembers the ownership of the image again, the ownership of the image remembered in the src host (the origin ownership) will lost.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_dac.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
I thought that refcounting should do the trick here. At least that was my intent when implementing this feature. I mean, the source sets seclabels and since the domain runs just once all refcounters are equal to 1. Then, during migration when the destination sets labels the refcounter is (temporarily) increased to 2, but only until the source calls restore (in which case the refcounter is decreased back to 1 again). Are you seeing different behaviour? BTW: what FS are you using to test this? Because I'm not aware of any shared FS that would support XATTRs. Michal
On 9/9/2021 7:01 PM, Michal Prívozník wrote:
On 8/23/21 4:41 AM, Peng Liang wrote:
When migrating with shared fs, the image labels has been remembered and the ownership of the image has been set in the src host. If the dst host remembers the ownership of the image again, the ownership of the image remembered in the src host (the origin ownership) will lost.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/security/security_dac.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
I thought that refcounting should do the trick here. At least that was my intent when implementing this feature. I mean, the source sets seclabels and since the domain runs just once all refcounters are equal to 1. Then, during migration when the destination sets labels the refcounter is (temporarily) increased to 2, but only until the source calls restore (in which case the refcounter is decreased back to 1 again).
Are you seeing different behaviour?
When the dst try to remember the labels (in virSecuritySetRememberedLabel), it will find that the timestamp is invalid, then it will remove all labels and set a new one instead of increasing the refcounter to 2. So I add virSecurityManagerUpdateImageLabel to update labels (currently, only update timestamp) during migration.
BTW: what FS are you using to test this? Because I'm not aware of any shared FS that would support XATTRs.
We are testing using ocfs2. Thanks, Peng
Michal
.
After migration, the image labels will be removed in the src host (on success) or the dst host (on failure). However, if we migrate using shared fs and remove image labels after migration in one host, the image labels will also lost in another host, which leads to that the ownership of the image will be restore to root:root instead of the origin ownership when shutting down the VM. Hence, don't remove image labels after migration with shared fs. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_process.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 77da9992f4e3..1eda7898c861 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8059,7 +8059,10 @@ void qemuProcessStop(virQEMUDriver *driver, qemuHostdevReAttachOneNVMeDisk(driver, vm->def->name, disk->mirror); } - qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); + if ((reason != VIR_DOMAIN_SHUTOFF_MIGRATED && + !(flags & VIR_QEMU_PROCESS_STOP_MIGRATED)) || + virFileIsSharedFS(disk->src->path) <= 0) + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); /* for now transient disks are forbidden with migration so they * can be handled here */ -- 2.31.1
Bacause the timestamp (the uptime of the host) is used to validate the remembered labels, it need to update after migration. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_migration.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b441d0226c8f..a5f7bd4add97 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5624,6 +5624,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainJobInfo *jobInfo = NULL; bool inPostCopy = false; bool doKill = true; + size_t i; VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -5831,6 +5832,17 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Guest is successfully running, so cancel previous auto destroy */ qemuProcessAutoDestroyRemove(driver, vm); + for (i = 0; i < vm->def->ndisks; i++) { + virStorageSource *src = vm->def->disks[i]->src; + + if (!virStorageSourceIsLocalStorage(src) || !src->path || + virFileIsSharedFS(src->path) < 0) + continue; + + if (qemuSecurityUpdateImageLabel(driver, vm, src) < 0) + VIR_WARN("Failed to update security label for %s", src->path); + } + endjob: if (!dom && !(flags & VIR_MIGRATE_OFFLINE) && -- 2.31.1
On 8/23/21 4:41 AM, Peng Liang wrote:
Bacause the timestamp (the uptime of the host) is used to validate the remembered labels, it need to update after migration.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_migration.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b441d0226c8f..a5f7bd4add97 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5624,6 +5624,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainJobInfo *jobInfo = NULL; bool inPostCopy = false; bool doKill = true; + size_t i;
VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -5831,6 +5832,17 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Guest is successfully running, so cancel previous auto destroy */ qemuProcessAutoDestroyRemove(driver, vm);
+ for (i = 0; i < vm->def->ndisks; i++) { + virStorageSource *src = vm->def->disks[i]->src; + + if (!virStorageSourceIsLocalStorage(src) || !src->path || + virFileIsSharedFS(src->path) < 0)
This last check is pretty much useless. virFileIsSharedFS() returns -1 only on failure. The way it is written completely ignores whether src->path is on a shared FS or not.
+ continue; + + if (qemuSecurityUpdateImageLabel(driver, vm, src) < 0) + VIR_WARN("Failed to update security label for %s", src->path); + } + endjob: if (!dom && !(flags & VIR_MIGRATE_OFFLINE) &&
Michal
On 9/9/2021 7:01 PM, Michal Prívozník wrote:
On 8/23/21 4:41 AM, Peng Liang wrote:
Bacause the timestamp (the uptime of the host) is used to validate the remembered labels, it need to update after migration.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_migration.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b441d0226c8f..a5f7bd4add97 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5624,6 +5624,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainJobInfo *jobInfo = NULL; bool inPostCopy = false; bool doKill = true; + size_t i;
VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -5831,6 +5832,17 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Guest is successfully running, so cancel previous auto destroy */ qemuProcessAutoDestroyRemove(driver, vm);
+ for (i = 0; i < vm->def->ndisks; i++) { + virStorageSource *src = vm->def->disks[i]->src; + + if (!virStorageSourceIsLocalStorage(src) || !src->path || + virFileIsSharedFS(src->path) < 0)
This last check is pretty much useless. virFileIsSharedFS() returns -1 only on failure. The way it is written completely ignores whether src->path is on a shared FS or not.
Oops, I'll fix it in the next version. Thanks, Peng
+ continue; + + if (qemuSecurityUpdateImageLabel(driver, vm, src) < 0) + VIR_WARN("Failed to update security label for %s", src->path); + } + endjob: if (!dom && !(flags & VIR_MIGRATE_OFFLINE) &&
Michal
.
Kindly ping. On 8/23/2021 10:41 AM, Peng Liang wrote:
When migrating using shared fs, the dst host will remember the image labels again before launching the VM and the src host will remove the image labels after migration. It leads to that the image labels lost after migration.
Patch 7-9 are the real fix patches.
Peng Liang (9): security: add virSecurityUpdateTimestampIfexists security: add virSecurityManagerUpdateImageLabel security: implement domainUpdateSecurityImageLabel for stack security: implement domainUpdateSecurityImageLabel for DAC qemu: add qemuSecurityUpdateImageLabel security: rename virSecurityDACSetImageLabelInternal to virSecurityDACSetImageLabelSingle migration: don't remember image labels when migrating with shared fs migration: don't remove image labels after migration migration: update image labels in dst after migration
src/libvirt_private.syms | 1 + src/qemu/qemu_migration.c | 12 +++++++ src/qemu/qemu_process.c | 5 ++- src/qemu/qemu_security.c | 10 ++++++ src/qemu/qemu_security.h | 5 +++ src/security/security_dac.c | 58 +++++++++++++++++++++++++-------- src/security/security_driver.h | 5 +++ src/security/security_manager.c | 29 +++++++++++++++++ src/security/security_manager.h | 5 +++ src/security/security_stack.c | 20 ++++++++++++ src/security/security_util.c | 32 ++++++++++++++++++ src/security/security_util.h | 4 +++ 12 files changed, 172 insertions(+), 14 deletions(-)
Thanks, Peng
participants (2)
-
Michal Prívozník -
Peng Liang