[PATCH 0/3] Fix crash when hot-unplugging a hostdev-interface
Peter Krempa (3): conf: Clear pointer to freed bitmap holding hostdev's 'origstates' conf: Unexport virDomainHostdevDefClear virDomainHostdevDefClear: Fix and shorten comment src/conf/domain_conf.c | 129 +++++++++++++++++++-------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 63 insertions(+), 68 deletions(-) -- 2.39.2
'virDomainHostdevDefClear' must clear the pointers too as it can be invoked multiple times on the same object e.g. inside qemuDomainRemoveHostDevice once via virDomainHostdevDefFree which skips freeing the object if it's used via <interface> and thus has a 'net' definition corresponding to it, and then subsequently via virDomainNetDefFree. Fix it by clearing the pointer along with freeing it. Fixes: d9e4075d4e9 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2182961 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9f49c6e62d..08527964d1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3384,7 +3384,7 @@ void virDomainHostdevDefClear(virDomainHostdevDef *def) VIR_FREE(def->source.subsys.u.scsi_host.wwpn); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - virBitmapFree(def->source.subsys.u.pci.origstates); + g_clear_pointer(&def->source.subsys.u.pci.origstates, virBitmapFree); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: -- 2.39.2
Move it before it's first usage and make it static. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 135 ++++++++++++++++++++------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 69 insertions(+), 68 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 08527964d1..7ad4ff26ab 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2589,6 +2589,75 @@ void virDomainFSDefFree(virDomainFSDef *def) g_free(def); } + +static void +virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSI *scsisrc) +{ + if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { + g_clear_pointer(&scsisrc->u.iscsi.src, virObjectUnref); + } else { + VIR_FREE(scsisrc->u.host.adapter); + g_clear_pointer(&scsisrc->u.host.src, virObjectUnref); + } +} + + +static void +virDomainHostdevDefClear(virDomainHostdevDef *def) +{ + if (!def) + return; + + /* Free all resources in the hostdevdef. Currently the only + * such resource is the virDomainDeviceInfo. + */ + + /* If there is a parentnet device object, it will handle freeing + * def->info. + */ + if (!def->parentnet) + virDomainDeviceInfoFree(def->info); + + virDomainNetTeamingInfoFree(def->teaming); + + switch (def->mode) { + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: + switch ((virDomainHostdevCapsType) def->source.caps.type) { + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: + VIR_FREE(def->source.caps.u.storage.block); + break; + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC: + VIR_FREE(def->source.caps.u.misc.chardev); + break; + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET: + VIR_FREE(def->source.caps.u.net.ifname); + virNetDevIPInfoClear(&def->source.caps.u.net.ip); + break; + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST: + break; + } + break; + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: + switch ((virDomainHostdevSubsysType) def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virDomainHostdevSubsysSCSIClear(&def->source.subsys.u.scsi); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: + VIR_FREE(def->source.subsys.u.scsi_host.wwpn); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + g_clear_pointer(&def->source.subsys.u.pci.origstates, virBitmapFree); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + break; + } + break; + } +} + + void virDomainActualNetDefFree(virDomainActualNetDef *def) { @@ -3329,72 +3398,6 @@ virDomainHostdevDefNew(void) } -static void -virDomainHostdevSubsysSCSIClear(virDomainHostdevSubsysSCSI *scsisrc) -{ - if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - g_clear_pointer(&scsisrc->u.iscsi.src, virObjectUnref); - } else { - VIR_FREE(scsisrc->u.host.adapter); - g_clear_pointer(&scsisrc->u.host.src, virObjectUnref); - } -} - - -void virDomainHostdevDefClear(virDomainHostdevDef *def) -{ - if (!def) - return; - - /* Free all resources in the hostdevdef. Currently the only - * such resource is the virDomainDeviceInfo. - */ - - /* If there is a parentnet device object, it will handle freeing - * def->info. - */ - if (!def->parentnet) - virDomainDeviceInfoFree(def->info); - - virDomainNetTeamingInfoFree(def->teaming); - - switch (def->mode) { - case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: - switch ((virDomainHostdevCapsType) def->source.caps.type) { - case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: - VIR_FREE(def->source.caps.u.storage.block); - break; - case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC: - VIR_FREE(def->source.caps.u.misc.chardev); - break; - case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET: - VIR_FREE(def->source.caps.u.net.ifname); - virNetDevIPInfoClear(&def->source.caps.u.net.ip); - break; - case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST: - break; - } - break; - case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: - switch ((virDomainHostdevSubsysType) def->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - virDomainHostdevSubsysSCSIClear(&def->source.subsys.u.scsi); - break; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: - VIR_FREE(def->source.subsys.u.scsi_host.wwpn); - break; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - g_clear_pointer(&def->source.subsys.u.pci.origstates, virBitmapFree); - break; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: - break; - } - break; - } -} - static virDomainTPMDef * virDomainTPMDefNew(virDomainXMLOption *xmlopt) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5a2c70f012..bd6b7a1a19 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3574,7 +3574,6 @@ void virDomainVideoDefFree(virDomainVideoDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVideoDef, virDomainVideoDefFree); void virDomainVideoDefClear(virDomainVideoDef *def); virDomainHostdevDef *virDomainHostdevDefNew(void); -void virDomainHostdevDefClear(virDomainHostdevDef *def); void virDomainHostdevDefFree(virDomainHostdevDef *def); void virDomainHubDefFree(virDomainHubDef *def); void virDomainRedirdevDefFree(virDomainRedirdevDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 87c3bab64f..2cd975a8a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -461,7 +461,6 @@ virDomainGraphicsVNCSharePolicyTypeFromString; virDomainGraphicsVNCSharePolicyTypeToString; virDomainHasNet; virDomainHostdevCapsTypeToString; -virDomainHostdevDefClear; virDomainHostdevDefFree; virDomainHostdevDefNew; virDomainHostdevFind; -- 2.39.2
On a Thursday in 2023, Peter Krempa wrote:
Move it before it's first usage and make it static.
*its Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 135 ++++++++++++++++++++------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 69 insertions(+), 68 deletions(-)
There's more stuff than device info to clear nowadays. Drop the misleading comment. Shorten the comment saying that device info is freed elsewhere when 'parentnet' is present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7ad4ff26ab..f8067be49a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2608,13 +2608,7 @@ virDomainHostdevDefClear(virDomainHostdevDef *def) if (!def) return; - /* Free all resources in the hostdevdef. Currently the only - * such resource is the virDomainDeviceInfo. - */ - - /* If there is a parentnet device object, it will handle freeing - * def->info. - */ + /* Device info is freed with 'parentnet' if present. */ if (!def->parentnet) virDomainDeviceInfoFree(def->info); -- 2.39.2
On a Thursday in 2023, Peter Krempa wrote:
There's more stuff than device info to clear nowadays. Drop the misleading comment. Shorten the comment saying that device info is freed elsewhere when 'parentnet' is present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7ad4ff26ab..f8067be49a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2608,13 +2608,7 @@ virDomainHostdevDefClear(virDomainHostdevDef *def) if (!def) return;
- /* Free all resources in the hostdevdef. Currently the only - * such resource is the virDomainDeviceInfo. - */ - - /* If there is a parentnet device object, it will handle freeing - * def->info. - */ + /* Device info is freed with 'parentnet' if present. */
s/freed/freed elswehere/ maybe?
if (!def->parentnet) virDomainDeviceInfoFree(def->info);
-- 2.39.2
On a Thursday in 2023, Peter Krempa wrote:
Peter Krempa (3): conf: Clear pointer to freed bitmap holding hostdev's 'origstates' conf: Unexport virDomainHostdevDefClear virDomainHostdevDefClear: Fix and shorten comment
src/conf/domain_conf.c | 129 +++++++++++++++++++-------------------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 63 insertions(+), 68 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa