[libvirt] [PATCH 8/9] util: Extend virresctl API to retrieve multiple monitor statistics

[Michal Privoznik]

On 5/28/19 10:32 AM, Huaqiang,Wang wrote:
> 
> 
> On 2019年05月27日 23:26, Michal Privoznik wrote:
>> On 5/23/19 11:34 AM, Wang Huaqiang wrote:
>>> Export virResctrlMonitorGetStats and make
>>> virResctrlMonitorGetCacheOccupancy obsoleted.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>>> ---
>>>   src/libvirt_private.syms |  1 +
>>>   src/qemu/qemu_driver.c   | 33 +++++++++++++++++++++++++--------
>>>   src/util/virresctrl.c    | 46 
>>> +++++++++++++++++++++++++++-------------------
>>>   src/util/virresctrl.h    |  6 ++++++
>>>   4 files changed, 59 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 9099757..2e3d48c 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2771,6 +2771,7 @@ virResctrlMonitorDeterminePath;
>>>   virResctrlMonitorFreeStats;
>>>   virResctrlMonitorGetCacheOccupancy;
>>>   virResctrlMonitorGetID;
>>> +virResctrlMonitorGetStats;
>>>   virResctrlMonitorNew;
>>>   virResctrlMonitorRemove;
>>>   virResctrlMonitorSetAlloc;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 4ea346c..bc19171 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -19987,6 +19987,7 @@ 
>>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>>>     /**
>>>    * qemuDomainGetResctrlMonData:
>>> + * @driver: Pointer to qemu driver
>>>    * @dom: Pointer for the domain that the resctrl monitors reside in
>>>    * @resdata: Pointer of virQEMUResctrlMonDataPtr pointer for 
>>> receiving the
>>>    *            virQEMUResctrlMonDataPtr array. Caller is responsible 
>>> for
>>> @@ -20005,16 +20006,29 @@ 
>>> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
>>>    * Returns -1 on failure, or 0 on success.
>>>    */
>>>   static int
>>> -qemuDomainGetResctrlMonData(virDomainObjPtr dom,
>>> +qemuDomainGetResctrlMonData(virQEMUDriverPtr driver,
>>> +                            virDomainObjPtr dom,
>>>                               virQEMUResctrlMonDataPtr **resdata,
>>>                               size_t *nresdata,
>>>                               virResctrlMonitorType tag)
>>>   {
>>>       virDomainResctrlDefPtr resctrl = NULL;
>>>       virQEMUResctrlMonDataPtr res = NULL;
>>> +    char **features = NULL;
>>>       size_t i = 0;
>>>       size_t j = 0;
>>>   +    if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
>>> +        features = driver->caps->host.cache.monitor->features;
>>
>>
>> No, we have virQEMUDriverGetCapabilities() which ensures that 
>> driver->caps object doesn't go away while accessing this.
>>
> 
> I can't refer 'driver->caps->host.cache.monitor->features' directly 
> because this function (qemuDomainGetResctrlMonData)
> will be used for 'memory bandwidth' monitors also.
> at that time that the piece of code looks like:
>    +    if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
>     +      if (driver->caps->host.cache.monitor)
> +            features = driver->caps->host.cache.monitor->features;
>    +    if (tag == VIR_RESCTRL_MONITOR_TYPE_MEMBW) {
>     +      if(driver->caps->host.membw)
> +            features = driver->caps->host.membw.monitor->features;

What I meant is that you can obtain virCaps object and then access it 
instead:

virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);

if (tag == VIR_RES..)
   features = caps->host.cache.monitor->features;

It's not safe to access driver->caps directly because another thread 
might refresh those meanwhile. What may happen for instance is the 
following:

Thread1:
fetches driver pointer
adds ->caps displacement
fetches value from that address (which is a long way of saying 'fetch 
driver->caps')

Thread2:
Executes virQEMUDriverGetCapabilities(), which boils down to:
virObjectUnref(driver->caps);
driver->caps = caps;

Thread1:
resumes execution and tries to access ->host member.

This is where Thread1 would be accessing invalid memory because the 
memory it's trying to access was freed meanwhile by Thread2.

Michal