9:41 p.m.
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
9:41 p.m.
New subject: [PATCH 1/9] security: add virSecurityUpdateTimestampIfexists
Signed-off-by: Peng Liang <liangpeng10(a)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
9:41 p.m.
New subject: [PATCH 2/9] security: add virSecurityManagerUpdateImageLabel
Signed-off-by: Peng Liang <liangpeng10(a)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
6:01 a.m.
New subject: [PATCH 2/9] security: add virSecurityManagerUpdateImageLabel
On 8/23/21 4:41 AM, Peng Liang wrote:
Signed-off-by: Peng Liang <liangpeng10(a)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
6:45 a.m.
New subject: [PATCH 2/9] security: add virSecurityManagerUpdateImageLabel
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(a)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
9:47 a.m.
New subject: [PATCH 2/9] security: add virSecurityManagerUpdateImageLabel
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(a)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
8:42 p.m.
New subject: [PATCH 2/9] security: add virSecurityManagerUpdateImageLabel
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(a)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
9:41 p.m.
New subject: [PATCH 3/9] security: implement domainUpdateSecurityImageLabel for stack
Signed-off-by: Peng Liang <liangpeng10(a)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
9:41 p.m.
New subject: [PATCH 4/9] security: implement domainUpdateSecurityImageLabel for DAC
Signed-off-by: Peng Liang <liangpeng10(a)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
9:41 p.m.
New subject: [PATCH 5/9] qemu: add qemuSecurityUpdateImageLabel
Signed-off-by: Peng Liang <liangpeng10(a)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
9:41 p.m.
New subject: [PATCH 6/9] security: rename virSecurityDACSetImageLabelInternal to virSecurityDACSetImageLabelSingle
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(a)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
9:41 p.m.
New subject: [PATCH 7/9] migration: don't remember image labels when migrating with shared fs
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(a)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
6:01 a.m.
New subject: [PATCH 7/9] migration: don't remember image labels when migrating with shared fs
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(a)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
6:32 a.m.
New subject: [PATCH 7/9] migration: don't remember image labels when migrating with shared fs
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(a)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
.
9:41 p.m.
New subject: [PATCH 8/9] migration: don't remove image labels after migration
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(a)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
9:41 p.m.
New subject: [PATCH 9/9] migration: update image labels in dst after migration
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(a)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
6:01 a.m.
New subject: [PATCH 9/9] migration: update image labels in dst after migration
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(a)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
6:33 a.m.
New subject: [PATCH 9/9] migration: update image labels in dst after migration
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(a)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
.
2:42 a.m.
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
1205
days inactive
1223
days old
18 comments
2 participants
participants (2)
-
Michal Prívozník -
Peng Liang