Re: [PATCH RESEND 09/20] dac, selinux: skip setting/restoring label for absent PCI devices

[Laine Stump <laine@xxxxxxxxxx>]

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:
> If the underlying PCI device of a hostdev does not exist in the
> host (e.g. a SR-IOV VF that was removed while the domain was
> running), skip security label handling for it.
>
> This will avoid errors that happens during qemuProcessStop() time,
> where a VF that was being used by the domain is not present anymore.
> The restore label functions of both DAC and SELinux drivers will
> trigger errors in virPCIDeviceNew().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>

This is fine as far as it goes, but what about 
AppArmorSetSecurityHostdevLabel() (and also whatever it is that's going 
on in the get_files() function in virt-aa-helper.c). You likely don't 
have a setup to test it, but it seems pretty straightforward to 
extrapolate that if you should skip setting the selinux and DAC labels 
when a device doesn't exist, you can probably do the same thing for 
AppArmor.

Reviewed-by: Laine Stump <laine@xxxxxxxxxx>

but add another patch that fixes the problem for AppArmor too.

(Also, all the code repetition here makes me think that there must be a 
better way to do this and reduce all the boilerplate, but I think it 
would be better to just make these changes and then look at eliminating 
the boilerplate afterwards, rather than re-structuring the code (which 
could create regressions) while adding this new concept on top of it.

> ---
>   src/security/security_dac.c     | 14 ++++++++++++--
>   src/security/security_selinux.c | 14 ++++++++++++--
>   2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0085982bb1..a2528aeb2d 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1266,7 +1266,12 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>       }
>   
>       case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> -        g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
> +        g_autoptr(virPCIDevice) pci = NULL;
> +
> +        if (!virPCIDeviceExists(&pcisrc->addr))
> +            break;
> +
> +        pci = virPCIDeviceNew(&pcisrc->addr);
>   
>           if (!pci)
>               return -1;
> @@ -1422,7 +1427,12 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
>       }
>   
>       case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> -        g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
> +        g_autoptr(virPCIDevice) pci = NULL;
> +
> +        if (!virPCIDeviceExists(&pcisrc->addr))
> +            break;
> +
> +        pci = virPCIDeviceNew(&pcisrc->addr);
>   
>           if (!pci)
>               return -1;
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 99adf08a15..c018c0708a 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -2101,7 +2101,12 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>       }
>   
>       case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> -        g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
> +        g_autoptr(virPCIDevice) pci = NULL;
> +
> +        if (!virPCIDeviceExists(&pcisrc->addr))
> +            break;
> +
> +        pci = virPCIDeviceNew(&pcisrc->addr);
>   
>           if (!pci)
>               return -1;
> @@ -2329,7 +2334,12 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>       }
>   
>       case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> -        g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
> +        g_autoptr(virPCIDevice) pci = NULL;
> +
> +        if (!virPCIDeviceExists(&pcisrc->addr))
> +            break;
> +
> +        pci = virPCIDeviceNew(&pcisrc->addr);
>   
>           if (!pci)
>               return -1;