1. Patchew
  2. Libvirt
  3. View series

[PATCH] serialize access pci_get_strings function with a mutex

wangjian posted 1 patch 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/44cd68af-6699-d4d0-0229-3edb80b33985@huawei.com
src/node_device/node_device_udev.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] serialize access pci_get_strings function with a mutex
Posted by wangjian 3 years ago 
serialize access pci_get_strings function with a mutex.

nodedev-init thread:
nodeStateInitializeEnumerate ->
  udevEnumerateDevices->
    udevProcessDeviceListEntry ->
      udevAddOneDevice ->
        udevGetDeviceDetails->
          udevProcessPCI ->
            udevTranslatePCIIds ->
              pci_get_strings -> (libpciaccess)
                find_device_name ->
                  populate_vendor ->
                    d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
                    vend->num_devices++;

udev-event thread:
udevEventHandleThread ->
  udevHandleOneDevice ->
    udevAddOneDevice->
      udevGetDeviceDetails->
        udevProcessPCI ->
          udevTranslatePCIIds ->
            pci_get_strings -> (libpciaccess)
              find_device_name ->
                populate_vendor ->
                  d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
                  vend->num_devices++;

Since the functions provided by libpciaccess are not thread-safe,
when the udev-event and nodedev-init threads of libvirt call
the pci_get_strings function provided by libpaciaccess at the same time.
It will appear that for the same address, realloc a large space first,
and then realloc a small space, which triggers the 'invalid next size' of realloc,
leading to the thread core.

Signed-off-by: WangJian <wangjian161@huawei.com>
---
 src/node_device/node_device_udev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 3f28858..cc752ba 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -331,6 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
     return 0;
 }

+static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;

 static int
 udevTranslatePCIIds(unsigned int vendor,
@@ -350,11 +351,13 @@ udevTranslatePCIIds(unsigned int vendor,
     m.match_data = 0;

     /* pci_get_strings returns void */
+    pthread_mutex_lock(&g_pciaccess_mutex);
     pci_get_strings(&m,
                     &device_name,
                     &vendor_name,
                     NULL,
                     NULL);
+    pthread_mutex_unlock(&g_pciaccess_mutex);

     *vendor_string = g_strdup(vendor_name);
     *product_string = g_strdup(device_name);
-- 
2.23.0
Re: [PATCH] serialize access pci_get_strings function with a mutex
Posted by Michal Privoznik 3 years ago 
There's no need to CC random developers - we are subscribed to the list.

On 3/26/21 4:21 AM, wangjian wrote:> serialize access pci_get_strings 
function with a mutex.> > nodedev-init thread:
> nodeStateInitializeEnumerate ->
>    udevEnumerateDevices->
>      udevProcessDeviceListEntry ->
>        udevAddOneDevice ->
>          udevGetDeviceDetails->
>            udevProcessPCI ->
>              udevTranslatePCIIds ->
>                pci_get_strings -> (libpciaccess)
>                  find_device_name ->
>                    populate_vendor ->
>                      d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>                      vend->num_devices++;
> 
> udev-event thread:
> udevEventHandleThread ->
>    udevHandleOneDevice ->
>      udevAddOneDevice->
>        udevGetDeviceDetails->
>          udevProcessPCI ->
>            udevTranslatePCIIds ->
>              pci_get_strings -> (libpciaccess)
>                find_device_name ->
>                  populate_vendor ->
>                    d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>                    vend->num_devices++;
> 
> Since the functions provided by libpciaccess are not thread-safe,
> when the udev-event and nodedev-init threads of libvirt call
> the pci_get_strings function provided by libpaciaccess at the same time.
> It will appear that for the same address, realloc a large space first,
> and then realloc a small space, which triggers the 'invalid next size' of realloc,
> leading to the thread core.
> 
> Signed-off-by: WangJian <wangjian161@huawei.com>
> ---
>   src/node_device/node_device_udev.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 3f28858..cc752ba 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -331,6 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
>       return 0;
>   }
> 
> +static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;

We tend to use virMutex (even though it is pthread_mutex under the hood).

> 
>   static int
>   udevTranslatePCIIds(unsigned int vendor,
> @@ -350,11 +351,13 @@ udevTranslatePCIIds(unsigned int vendor,
>       m.match_data = 0;
> 
>       /* pci_get_strings returns void */
> +    pthread_mutex_lock(&g_pciaccess_mutex);
>       pci_get_strings(&m,
>                       &device_name,
>                       &vendor_name,
>                       NULL,
>                       NULL);
> +    pthread_mutex_unlock(&g_pciaccess_mutex);
> 
>       *vendor_string = g_strdup(vendor_name);
>       *product_string = g_strdup(device_name);
> 

Apart from that, the patch is correct. I'd squash this in and push if 
you agree:

diff --git i/src/node_device/node_device_udev.c 
w/src/node_device/node_device_udev.c
index cc752bad32..3daa5c90ad 100644
--- i/src/node_device/node_device_udev.c
+++ w/src/node_device/node_device_udev.c
@@ -331,7 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
      return 0;
  }

-static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;
+static virMutex pciaccessMutex = VIR_MUTEX_INITIALIZER;

  static int
  udevTranslatePCIIds(unsigned int vendor,
@@ -350,14 +350,14 @@ udevTranslatePCIIds(unsigned int vendor,
      m.device_class_mask = 0;
      m.match_data = 0;

-    /* pci_get_strings returns void */
-    pthread_mutex_lock(&g_pciaccess_mutex);
+    /* pci_get_strings returns void and unfortunately is not thread 
safe. */
+    virMutexLock(&pciaccessMutex);
      pci_get_strings(&m,
                      &device_name,
                      &vendor_name,
                      NULL,
                      NULL);
-    pthread_mutex_unlock(&g_pciaccess_mutex);
+    virMutexUnlock(&pciaccessMutex);

      *vendor_string = g_strdup(vendor_name);
      *product_string = g_strdup(device_name);


Michal
Re: [PATCH] serialize access pci_get_strings function with a mutex
Posted by wangjian (AN) 3 years ago 
Ok. I agree.

On 2021/3/26 17:10, Michal Privoznik wrote:
> There's no need to CC random developers - we are subscribed to the list.
> 
> On 3/26/21 4:21 AM, wangjian wrote:> serialize access pci_get_strings function with a mutex.> > nodedev-init thread:
>> nodeStateInitializeEnumerate ->
>>    udevEnumerateDevices->
>>      udevProcessDeviceListEntry ->
>>        udevAddOneDevice ->
>>          udevGetDeviceDetails->
>>            udevProcessPCI ->
>>              udevTranslatePCIIds ->
>>                pci_get_strings -> (libpciaccess)
>>                  find_device_name ->
>>                    populate_vendor ->
>>                      d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>>                      vend->num_devices++;
>>
>> udev-event thread:
>> udevEventHandleThread ->
>>    udevHandleOneDevice ->
>>      udevAddOneDevice->
>>        udevGetDeviceDetails->
>>          udevProcessPCI ->
>>            udevTranslatePCIIds ->
>>              pci_get_strings -> (libpciaccess)
>>                find_device_name ->
>>                  populate_vendor ->
>>                    d = realloc( vend->devices, (vend->num_devices + 1), * sizeof( struct pci_device_leaf ) );
>>                    vend->num_devices++;
>>
>> Since the functions provided by libpciaccess are not thread-safe,
>> when the udev-event and nodedev-init threads of libvirt call
>> the pci_get_strings function provided by libpaciaccess at the same time.
>> It will appear that for the same address, realloc a large space first,
>> and then realloc a small space, which triggers the 'invalid next size' of realloc,
>> leading to the thread core.
>>
>> Signed-off-by: WangJian <wangjian161@huawei.com>
>> ---
>>   src/node_device/node_device_udev.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>> index 3f28858..cc752ba 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -331,6 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
>>       return 0;
>>   }
>>
>> +static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;
> 
> We tend to use virMutex (even though it is pthread_mutex under the hood).
> 
>>
>>   static int
>>   udevTranslatePCIIds(unsigned int vendor,
>> @@ -350,11 +351,13 @@ udevTranslatePCIIds(unsigned int vendor,
>>       m.match_data = 0;
>>
>>       /* pci_get_strings returns void */
>> +    pthread_mutex_lock(&g_pciaccess_mutex);
>>       pci_get_strings(&m,
>>                       &device_name,
>>                       &vendor_name,
>>                       NULL,
>>                       NULL);
>> +    pthread_mutex_unlock(&g_pciaccess_mutex);
>>
>>       *vendor_string = g_strdup(vendor_name);
>>       *product_string = g_strdup(device_name);
>>
> 
> Apart from that, the patch is correct. I'd squash this in and push if you agree:
> 
> diff --git i/src/node_device/node_device_udev.c w/src/node_device/node_device_udev.c
> index cc752bad32..3daa5c90ad 100644
> --- i/src/node_device/node_device_udev.c
> +++ w/src/node_device/node_device_udev.c
> @@ -331,7 +331,7 @@ udevGenerateDeviceName(struct udev_device *device,
>      return 0;
>  }
> 
> -static pthread_mutex_t g_pciaccess_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static virMutex pciaccessMutex = VIR_MUTEX_INITIALIZER;
> 
>  static int
>  udevTranslatePCIIds(unsigned int vendor,
> @@ -350,14 +350,14 @@ udevTranslatePCIIds(unsigned int vendor,
>      m.device_class_mask = 0;
>      m.match_data = 0;
> 
> -    /* pci_get_strings returns void */
> -    pthread_mutex_lock(&g_pciaccess_mutex);
> +    /* pci_get_strings returns void and unfortunately is not thread safe. */
> +    virMutexLock(&pciaccessMutex);
>      pci_get_strings(&m,
>                      &device_name,
>                      &vendor_name,
>                      NULL,
>                      NULL);
> -    pthread_mutex_unlock(&g_pciaccess_mutex);
> +    virMutexUnlock(&pciaccessMutex);
> 
>      *vendor_string = g_strdup(vendor_name);
>      *product_string = g_strdup(device_name);
> 
> 
> Michal
> 
> .
>
Re: [PATCH] serialize access pci_get_strings function with a mutex
Posted by Michal Privoznik 3 years ago 
On 3/26/21 10:36 AM, wangjian (AN) wrote:
> Ok. I agree.
> 

Alright, then:

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

And congratulations on your first libvirt contribution!

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.