Re: [Xen-devel] [PATCH v3] dmar: device scope mem leak fix

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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