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
> 
> .
>