[GSD-12417] crash in zesDevicePciGetProperties()

[bgoglin]

Hello
hwloc crashes in recent releases of level-zero and compute-runtime. I don't know exactly when it started because it's complicated to test different releases when Debian/Ubuntu official ZE packages (mostly level-zero package names) are incompatible with the deb packages you distribute.

Anyway, here's a small reproducer extracted from hwloc. It basically just list ZES drivers and devices and calls zesDevicePciGetProperties() on each:

#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <assert.h>

#include <level_zero/ze_api.h>
#include <level_zero/zes_api.h>

int main(void)
{
  zes_driver_handle_t *sdrh;
  uint32_t nbdrivers, i, k;
  ze_result_t res;

  printf("testing ZES devices\n");

  res = zesInit(0);
  if (res != ZE_RESULT_SUCCESS) {
    fprintf(stderr, "Failed to initialize LevelZero Sysman in zesInit(): %d\n", (int)res);
    return 0;
  }

  nbdrivers = 0;
  res = zesDriverGet(&nbdrivers, NULL);
  if (res != ZE_RESULT_SUCCESS || !nbdrivers)
    return 0;
  sdrh = malloc(nbdrivers * sizeof(*sdrh));
  if (!sdrh)
    return 0;
  res = zesDriverGet(&nbdrivers, sdrh);
  if (res != ZE_RESULT_SUCCESS) {
    free(sdrh);
    return 0;
  }

  printf("found %u L0 ZES drivers\n", nbdrivers);

  k = 0;
  for(i=0; i<nbdrivers; i++) {
    uint32_t nbdevices, j;
    zes_device_handle_t *sdvh;

    nbdevices = 0;
    res = zesDeviceGet(sdrh[i], &nbdevices, NULL);
    if (res != ZE_RESULT_SUCCESS || !nbdevices)
      continue;
    sdvh = malloc(nbdevices * sizeof(*sdvh));
    if (!sdvh)
      continue;
    res = zesDeviceGet(sdrh[i], &nbdevices, sdvh);
    if (res != ZE_RESULT_SUCCESS) {
      free(sdvh);
      continue;
    }

    printf("found %u L0 ZES devices in driver #%u\n", nbdevices, i);

    for (j=0; j<nbdevices; j++, k++) {
      zes_pci_properties_t pci;

      res = zesDevicePciGetProperties(sdvh[j], &pci);
      if (res != ZE_RESULT_SUCCESS) {
        errno = EINVAL;
        return -1;
      }
      printf("got PCI\n");
    }
    free(sdvh);
  }
  free(sdrh);

  return 0;
}

gdb shows:

testing ZES devices
⚠️ warning: Corrupted shared library list: 0x555557ab6ca0 != 0x555555576310
found 1 L0 ZES drivers
found 1 L0 ZES devices in driver #0

Program received signal SIGSEGV, Segmentation fault.
L0::Sysman::PciImp::pciStaticProperties (this=0x555557ab75b0, 
    pProperties=0x7fffffffe2b0)
    at ../../neo/level_zero/sysman/source/api/pci/sysman_pci_imp.cpp:126
⚠️ warning: 126	../../neo/level_zero/sysman/source/api/pci/sysman_pci_imp.cpp: Aucun fichier ou dossier de ce nom

The machine is a laptop with 13th Gen Intel(R) Core(TM) i7-1370P running 6.18.12+deb14-amd64
I removed all relevant packages and installed your last ones (+ dbgsym ones):

intel-igc-core-2_2.28.4+20760_amd64.deb
intel-ocloc_26.05.37020.3-0_amd64.deb
level-zero-devel_1.27.0+u24.04_amd64.deb
libigdgmm12_22.9.0_amd64.deb
intel-igc-opencl-2_2.28.4+20760_amd64.deb
intel-opencl-icd_26.05.37020.3-0_amd64.deb
level-zero_1.27.0+u24.04_amd64.deb
libze-intel-gpu1_26.05.37020.3-0_amd64.deb

[kgibala]

Hi @bgoglin,

Thank you for your contribution and for reporting this issue. We appreciate you bringing this to our attention.

We will be investigating this potential regression within our internal scope. The issue will be forwarded to the appropriate Level Zero Sysman Team for proper handling.

We will keep you updated as soon as we have meaningful information to share.

Thank you for your patience.

[kgibala]

Hi @bgoglin,

After reviewing the provided source code, We have identified an issue with the sample implementation.

The declaration zes_pci_properties_t pci; does not initialize the pNext pointer, which leads to undefined behavior.

The correct initialization should be:

zes_pci_properties_t pci = {};

This ensures that the pNext pointer (and all other members) are properly initialized to nullptr/zero values, as required by the Level Zero specification: https://oneapi-src.github.io/level-zero-spec/level-zero/latest/sysman/api.html#_CPPv4N20zes_pci_properties_t5pNextE

Valgrind detected the uninitialized value issue:

==4741== 1 errors in context 1 of 2:
==4741== Conditional jump or move depends on uninitialised value(s)
==4741== at 0x877F204: L0::Sysman::PciImp::pciStaticProperties(_zes_pci_properties_t*) (sysman_pci_imp.cpp:123)
==4741== by 0x40137E: main
==4741== Uninitialised value was created by a stack allocation
==4741== at 0x401230: main

After applying the correct initialization, the code runs successfully:

...
devices found 1
L0 ZES drivers found 1
L0 ZES devices in driver #0
got PCI

@bgoglin could you please review this fix and provide feedback on whether there are any additional aspects that should be addressed for this ticket?

[bgoglin]

Thank you. It seems to fix the issue here too.
It would be convenient to document this inside the documentation of the function itself since the requirement to initialize to NULL is actually for the function, not for the structure. I see [in, out] for that parameter, but a function called GetProperties() lets users thinks its [in] only, which leds to my assumption that there's nothing to initialize. Looks like it's the same for many other functions/structures, I am going to fix all those too.

[kgibala]

Hi @bgoglin,

Thank for the response.

Documentation for zesDevicePciGetProperties:

• https://oneapi-src.github.io/level-zero-spec/level-zero/latest/sysman/api.html#zesdevicepcigetproperties

ZE_APIEXPORT ze_result_t ZE_APICALL zesDevicePciGetProperties(zes_device_handle_t hDevice, zes_pci_properties_t *pProperties)

Parameters:

• hDevice – [in] Sysman handle of the device.
• pProperties – [in,out] Will contain the PCI properties.

In here we can see that [in,out] parameter is mentioned. Could you share a proposition how the documentation could be improved here?

[bgoglin]

Put an explicit sentence saying that pProperties->pNext should be NULL before input.

By the way, is there an actual case where this pNext pointer is used?

Looking more at the doc of other structures, it looks like we're also supposed to initialize the stype field to the type of structure. However I never do it (except in zeDevicePciGetPropertiesExt), and this never caused any damage. If this is going to be enforced at some point, it should be documented inside functions' doc too. Or maybe it's only necessary when some other struct has its pNext pointer pointing to such a struct ?

[kgibala]

Hi @bgoglin,

Put an explicit sentence saying that pProperties->pNext should be NULL before input.

The specification already contains that kind of statement:

• https://oneapi-src.github.io/level-zero-spec/level-zero/latest/sysman/api.html#_CPPv4N20zes_pci_properties_t5pNextE

void *pNext
[in,out][optional] must be null or a pointer to an extension-specific structure (i.e. contains stype and pNext).

---

By the way, is there an actual case where this pNext pointer is used?

Yes. It is used to query zes_pci_link_speed_downgrade_ext_properties_t

• https://oneapi-src.github.io/level-zero-spec/level-zero/latest/sysman/api.html#_CPPv445zes_pci_link_speed_downgrade_ext_properties_t

[bgoglin]

As I said, putting it in the struct definition isn't really correct since the requirement is in the function, not in the structure itself. My original bug report was caused by this: I didn't see that requirement in the doc because I wasn't expecting anything like this in the structure doc, but rather in the function doc.
Anyway, in the documentation of the function, after "pProperties – [in,out] Will contain the PCI properties." you should add "The pNext field should be NULL before the call and the stype type should be ....".