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@xxxxxxxxxx>
---
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
