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@gmail.com>
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@redhat.com> 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;