[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] dmar: device scope mem leak fix
>>> On 02.06.15 at 23:13, <elena.ufimtseva@xxxxxxxxxx> wrote: > From: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx> > > Third attempt to incorporate memory leak fix. > Thanks for comment on v2. Neither this nor ... > Release memory allocated for scope.devices when disabling > dmar units. Also set device count after memory allocation when > device scope parsing. > > Changes in v3: > - make freeing memory for scope devices and zeroing device counter > a function and use it; > - make sure parse_one_rmrr has memory leak fix in this patch; > - make sure ret values are not lost acpi_parse_one_drhd; > > Changes in v2: > - release memory for devices scope on error paths in acpi_parse_one_drhd > and acpi_parse_one_atsr and set the count to zero; ... these belong above the first --- marker. While the patch now looks correct to me, there's still some room for improvement: > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -81,6 +81,12 @@ static int __init acpi_register_rmrr_unit(struct > acpi_rmrr_unit *rmrr) > return 0; > } > > +static void scope_devices_free(struct dmar_scope *scope) > +{ > + scope->devices_cnt = 0; > + xfree(scope->devices); > +} While it looks like this isn't an active problem, not storing NULL in scope->devices here looks like calling for future problems. > @@ -476,11 +485,6 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header) > if ( ret ) > goto out; > > - dev_scope_start = (void *)(drhd + 1); > - dev_scope_end = ((void *)drhd) + header->length; > - ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end, > - &dmaru->scope, DMAR_TYPE, drhd->segment); > - > if ( dmaru->include_all ) > { > if ( iommu_verbose ) > @@ -495,7 +499,13 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header) > if ( drhd->segment == 0 ) > include_all = 1; > } > + if ( ret ) > + goto out; > > + dev_scope_start = (void *)(drhd + 1); > + dev_scope_end = ((void *)drhd) + header->length; > + ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end, > + &dmaru->scope, DMAR_TYPE, drhd->segment); > if ( ret ) > goto out; > else if ( force_iommu || dmaru->include_all ) I'm still lacking a sentence in the patch description explaining why this code needs to move - offhand I can't see the reason. > @@ -552,6 +563,7 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header) > "its scope are not PCI discoverable! Pls try option " > "iommu=force or iommu=workaround_bios_bug if you " > "really want VT-d\n"); > + scope_devices_free(&dmaru->scope); > ret = -EINVAL; This would seem to belong ... > @@ -565,6 +577,7 @@ out: > iommu_free(dmaru); > xfree(dmaru); > } > + > return ret; ... in the if() body seen in the context here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |