[PATCH v2] dmar: device scope mem leak fix

[PATCH v2] dmar: device scope mem leak fix

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

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@oracle.com>
---
 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);
     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);
         }
         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

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

[Tian, Kevin]

> From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> Sent: Saturday, May 30, 2015 5:35 AM
> 
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> 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@oracle.com>
> ---
>  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

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

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Monday, June 01, 2015 12:43 PM
> 
> 
> and looks you dropped earlier changes to acpi_parse_one_rmrr. any
> elaboration why it's not required in this version?
> 
> Thanks
> Kevin

Never mind this one. Seems you have it in RMRR patch set.

Thanks
Kevin

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

[Jan Beulich]

>>> On 01.06.15 at 06:47, <kevin.tian@intel.com> wrote:
>>  From: Tian, Kevin
>> Sent: Monday, June 01, 2015 12:43 PM
>> 
>> 
>> and looks you dropped earlier changes to acpi_parse_one_rmrr. any
>> elaboration why it's not required in this version?
> 
> Never mind this one. Seems you have it in RMRR patch set.

No - it belongs here, not there.

Jan

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

[Elena Ufimtseva]

On Mon, Jun 01, 2015 at 09:45:51AM +0100, Jan Beulich wrote:
> >>> On 01.06.15 at 06:47, <kevin.tian@intel.com> wrote:
> >>  From: Tian, Kevin
> >> Sent: Monday, June 01, 2015 12:43 PM
> >> 
> >> 
> >> and looks you dropped earlier changes to acpi_parse_one_rmrr. any
> >> elaboration why it's not required in this version?
> > 
> > Never mind this one. Seems you have it in RMRR patch set.
> 
> No - it belongs here, not there.
> 
> Jan

Yes, Jan is right, it went to rmrr patch, but have to be here in mem
leak fix.
>