[libvirt] [PATCH] conf: resctrl object is not properly handled

Wang Huaqiang posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1566295563-13485-1-git-send-email-huaqiang.wang@intel.com
src/conf/domain_conf.c                    | 12 +++++-------
tests/genericxml2xmlindata/memorytune.xml |  4 ++++
2 files changed, 9 insertions(+), 7 deletions(-)
[libvirt] [PATCH] conf: resctrl object is not properly handled
Posted by Wang Huaqiang 4 years, 7 months ago
resctrl object stored in def->resctrls is shared by cachetune and
memorytune. The domain xml configuration is parsed firstly for
cachetune then memorytune, and the resctrl object will not be created
in parsing settings for memorytune once it found sharing exists.

But resctrl is improperly freed when sharing happens.

Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
---
 src/conf/domain_conf.c                    | 12 +++++-------
 tests/genericxml2xmlindata/memorytune.xml |  4 ++++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 617ccac..604e006 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19590,7 +19590,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
     VIR_AUTOUNREF(virResctrlAllocPtr) alloc = NULL;
     ssize_t i = 0;
     int n;
-    int ret = -1;
 
     ctxt->node = node;
 
@@ -19632,14 +19631,13 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
         if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, flags)))
             return -1;
 
-        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
-            goto cleanup;
+        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) {
+            virDomainResctrlDefFree(resctrl);
+            return -1;
+        }
     }
 
-    ret = 0;
- cleanup:
-    virDomainResctrlDefFree(resctrl);
-    return ret;
+    return 0;
 }
 
 
diff --git a/tests/genericxml2xmlindata/memorytune.xml b/tests/genericxml2xmlindata/memorytune.xml
index ea03e22..7486b54 100644
--- a/tests/genericxml2xmlindata/memorytune.xml
+++ b/tests/genericxml2xmlindata/memorytune.xml
@@ -5,6 +5,10 @@
   <currentMemory unit='KiB'>219136</currentMemory>
   <vcpu placement='static'>4</vcpu>
   <cputune>
+    <cachetune vcpus='0-1'>
+      <cache id='0' level='3' type='both' size='768' unit='KiB'/>
+      <cache id='1' level='3' type='both' size='768' unit='KiB'/>
+    </cachetune>
     <memorytune vcpus='0-1'>
       <node id='0' bandwidth='20'/>
       <node id='1' bandwidth='30'/>
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: resctrl object is not properly handled
Posted by Daniel Henrique Barboza 4 years, 7 months ago

On 8/20/19 7:06 AM, Wang Huaqiang wrote:
> resctrl object stored in def->resctrls is shared by cachetune and
> memorytune. The domain xml configuration is parsed firstly for
> cachetune then memorytune, and the resctrl object will not be created
> in parsing settings for memorytune once it found sharing exists.
>
> But resctrl is improperly freed when sharing happens.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> ---
>   src/conf/domain_conf.c                    | 12 +++++-------
>   tests/genericxml2xmlindata/memorytune.xml |  4 ++++
>   2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 617ccac..604e006 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19590,7 +19590,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>       VIR_AUTOUNREF(virResctrlAllocPtr) alloc = NULL;
>       ssize_t i = 0;
>       int n;
> -    int ret = -1;
>   
>       ctxt->node = node;
>   
> @@ -19632,14 +19631,13 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>           if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, flags)))
>               return -1;
>   
> -        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
> -            goto cleanup;
> +        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0) {
> +            virDomainResctrlDefFree(resctrl);
> +            return -1;
> +        }
>       }
>   
> -    ret = 0;
> - cleanup:
> -    virDomainResctrlDefFree(resctrl);
> -    return ret;
> +    return 0;
>   }

This could also be fixed by putting the free into a conditional:

if (resctrl)
     virDomainRescrtlDefFree(resctrl);


But I appreciate one less 'goto' jump to worry about.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   
>   
> diff --git a/tests/genericxml2xmlindata/memorytune.xml b/tests/genericxml2xmlindata/memorytune.xml
> index ea03e22..7486b54 100644
> --- a/tests/genericxml2xmlindata/memorytune.xml
> +++ b/tests/genericxml2xmlindata/memorytune.xml
> @@ -5,6 +5,10 @@
>     <currentMemory unit='KiB'>219136</currentMemory>
>     <vcpu placement='static'>4</vcpu>
>     <cputune>
> +    <cachetune vcpus='0-1'>
> +      <cache id='0' level='3' type='both' size='768' unit='KiB'/>
> +      <cache id='1' level='3' type='both' size='768' unit='KiB'/>
> +    </cachetune>
>       <memorytune vcpus='0-1'>
>         <node id='0' bandwidth='20'/>
>         <node id='1' bandwidth='30'/>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: resctrl object is not properly handled
Posted by Michal Privoznik 4 years, 7 months ago
On 8/20/19 3:18 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 8/20/19 7:06 AM, Wang Huaqiang wrote:
>> resctrl object stored in def->resctrls is shared by cachetune and
>> memorytune. The domain xml configuration is parsed firstly for
>> cachetune then memorytune, and the resctrl object will not be created
>> in parsing settings for memorytune once it found sharing exists.
>>
>> But resctrl is improperly freed when sharing happens.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>> ---
>>   src/conf/domain_conf.c                    | 12 +++++-------
>>   tests/genericxml2xmlindata/memorytune.xml |  4 ++++
>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 617ccac..604e006 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19590,7 +19590,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>>       VIR_AUTOUNREF(virResctrlAllocPtr) alloc = NULL;
>>       ssize_t i = 0;
>>       int n;
>> -    int ret = -1;
>>       ctxt->node = node;
>> @@ -19632,14 +19631,13 @@ virDomainMemorytuneDefParse(virDomainDefPtr 
>> def,
>>           if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, 
>> flags)))
>>               return -1;
>> -        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, 
>> resctrl) < 0)
>> -            goto cleanup;
>> +        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, 
>> resctrl) < 0) {
>> +            virDomainResctrlDefFree(resctrl);
>> +            return -1;
>> +        }
>>       }
>> -    ret = 0;
>> - cleanup:
>> -    virDomainResctrlDefFree(resctrl);
>> -    return ret;
>> +    return 0;
>>   }
> 
> This could also be fixed by putting the free into a conditional:
> 
> if (resctrl)
>      virDomainRescrtlDefFree(resctrl);

Not really. We want to free @resctrl only if it's a newly allocated 
struct. In case the pointer was obtained via virDomainResctrlVcpuMatch() 
then we must not free it (even though it is not NULL).

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And pushed. Thanks for fixing this.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list