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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: elena.ufimtseva@xxxxxxxxxx [mailto:elena.ufimtseva@xxxxxxxxxx]
> Sent: Saturday, May 30, 2015 5:35 AM
> 
> From: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> 
> Release memory allocated for scope.devices when disabling
> dmar units. Also set device count after memory allocation when
> device scope parsing.
> 
> 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;
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/dmar.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 2b07be9..0985150 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -90,16 +90,19 @@ static void __init disable_all_dmar_units(void)
>      list_for_each_entry_safe ( drhd, _drhd, &acpi_drhd_units, list )
>      {
>          list_del(&drhd->list);
> +        xfree(drhd->scope.devices);
>          xfree(drhd);
>      }
>      list_for_each_entry_safe ( rmrr, _rmrr, &acpi_rmrr_units, list )
>      {
>          list_del(&rmrr->list);
> +        xfree(rmrr->scope.devices);
>          xfree(rmrr);
>      }
>      list_for_each_entry_safe ( atsr, _atsr, &acpi_atsr_units, list )
>      {
>          list_del(&atsr->list);
> +        xfree(atsr->scope.devices);
>          xfree(atsr);
>      }
>  }
> @@ -318,13 +321,13 @@ static int __init acpi_parse_dev_scope(
>      if ( (cnt = scope_device_count(start, end)) < 0 )
>          return cnt;
> 
> -    scope->devices_cnt = cnt;
>      if ( cnt > 0 )
>      {
>          scope->devices = xzalloc_array(u16, cnt);
>          if ( !scope->devices )
>              return -ENOMEM;
>      }
> +    scope->devices_cnt = cnt;
> 
>      while ( start < end )
>      {
> @@ -427,7 +430,10 @@ static int __init acpi_parse_dev_scope(
> 
>   out:
>      if ( ret )
> +    {
> +        scope->devices_cnt = 0;
>          xfree(scope->devices);
> +    }
> 
>      return ret;
>  }
> @@ -478,8 +484,6 @@ acpi_parse_one_drhd(struct acpi_dmar_header
> *header)
> 
>      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 )
>      {
> @@ -496,6 +500,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header
> *header)
>              include_all = 1;
>      }
> 
> +    ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> +                               &dmaru->scope, DMAR_TYPE,
> drhd->segment);

I didn't see this movement leads to any improvement. You at least need
to move it after below ret check as an optimization. Also please move 
dev_scope_start/end together since they are not used by earlier code 
so better to make them close to the real reference place.

>      if ( ret )
>          goto out;
>      else if ( force_iommu || dmaru->include_all )
> @@ -554,6 +560,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header
> *header)
>                      "really want VT-d\n");
>                  ret = -EINVAL;
>              }
> +            dmaru->scope.devices_cnt = 0;
> +            xfree(dmaru->scope.devices);

Not necessary since it will reach later error handling anyway. And looks you
need fix leak in earlier branch instead:

        if ( iommu_workaround_bios_bug &&
          invalid_cnt == dmaru->scope.devices_cnt )
        {
          dprintk(XENLOG_WARNING VTDPREFIX,
               "  Workaround BIOS bug: ignore the DRHD due to all "
               "devices under its scope are not PCI discoverable!\n");
          iommu_free(dmaru);
          xfree(dmaru);
        }

>          }
>          else
>              acpi_register_drhd_unit(dmaru);
> @@ -727,7 +735,11 @@ acpi_parse_one_atsr(struct acpi_dmar_header
> *header)
>      }
> 
>      if ( ret )
> +    {
> +        atsru->scope.devices_cnt = 0;
> +        xfree(atsru->scope.devices);
>          xfree(atsru);
> +    }
>      else
>          acpi_register_atsr_unit(atsru);
>      return ret;
> --
> 2.1.3

and looks you dropped earlier changes to acpi_parse_one_rmrr. any
elaboration why it's not required in this version?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel