[PATCH v2] selinux label: restore all labels when some labels fail to set

[Jin Yan]

On 2020/11/13 2:54, Michal Privoznik wrote:
> On 11/12/20 3:07 PM, Jin Yan wrote:
>> When migration fails, qemuMigrationDstPrepareAny will call qemuProcessStop
>> to restore labels only after all labels are successfully set. If some labels
>> fail to set, the labels that have been set will not be restored.
>>
>> Signed-off-by: Jin Yan <jinyan12 at huawei.com>
>> ---
>>    src/qemu/qemu_security.c | 12 ++++++++++--
>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 3bda96272c..0cb90c840a 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -51,16 +51,24 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>>                                          incomingPath,
>>                                          priv->chardevStdioLogd,
>>                                          migrated) < 0)
>> -        goto cleanup;
>> +        goto restorelabel;
>>    
>>        if (virSecurityManagerTransactionCommit(driver->securityManager,
>>                                                pid, priv->rememberOwner) < 0)
>> -        goto cleanup;
>> +        goto restorelabel;
>>    
>>        ret = 0;
>> +
>>     cleanup:
>>        virSecurityManagerTransactionAbort(driver->securityManager);
>>        return ret;
>> +
>> + restorelabel:
>> +    virSecurityManagerRestoreAllLabel(driver->securityManager,
>> +                                      vm->def,
>> +                                      migrated,
>> +                                      priv->chardevStdioLogd);
>> +    goto cleanup;
>>    }
>>    
>>    
>>
> I don't think this is correct. Firstly, this restores labels for ALL
> paths, and not just the failed ones (which messes up seclabel
> remembering and its refcounting), but more importantly:
>
> 1) rollback within one secdriver is handled in .transactionCommit
> callback, well virSecurity*TransactionRun() actually,
>
> 2) rollback for other secdrivers after one failed is handled in
> virSecurityStackSetAllLabel().
>
>
> Is this not working properly? What version do you run?

Hi Michal,
I found this problem while performing migration, based on
     libvirt version: 6.2.0
     SELinux mode: permissive

Steps:
1. start a vm configured with pipe-type serial port.
     <serial type='pipe'>
       <source path='/tmp/test_pipe'/>
       <target type='system-serial' port='1'>
         <model name='pl011'/>
       </target>
     </serial>
2. migrate vm to Dst-side where no '/tmp/test_pipe' exits.
3. migration failed in Dst-side qemuProcessLaunch, and the path's label that
has been set is not restored ('/var/lib/libvirt/qemu/nvram/XXX.fd').

I have no idea why 2)rollback you mentioned didn't work.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201113/de7fd28c/attachment-0001.htm>