Re: [PATCH v3 01/13] util: properly deal with VFIO module name vs. driver name

On Fri, Jan 05, 2024 at 03:20:04 -0500, Laine Stump wrote: > Historically libvirt hasn't differentiated between the name of a > loadable kernel module, and the name of the device driver that module > implements, but these two names can be (and usually are) at least > subtly different. > > For example, the loadable module called "vfio_pci" implements a PCI > driver called "vfio-pci". We have always used the name "vfio-pci" both > to load the module (with modprobe) and to check (in > /sys/bus/pci/drivers) if the driver is available. (This has happened > to work because modprobe "normalizes" all the names it is given by > replacing "-" with "_", so "vfio-pci" works for both loading the > module and checking for the driver.) > > When we recently gained the ability to manually specify the driver for > "virsh nodedev-detach", the fragility of this system became apparent - > if a user gave the "driver name" as "vfio_pci", then we would modprobe > the module correctly, but then erroneously believe it hadn't been > loaded because /sys/bus/pci/drivers/vfio_pci didn't exist. For manual > specification of the driver name, we could deal with this by telling > the user "always use the correct name for the driver, don't assume > that it has the same name as the module", but it would still end up > confusing people, especially since some drivers do use underscore in > their name (e.g. the mlx5_vfio_pci driver/module). > > This will only get worse when an upcoming patch starts automatically > determining the driver to use for VFIO-assigned devices - it will look > in the kernel's modules.alias file to find "best" VFIO variant > *module* for a device, and 3 out of 4 current examples of > vfio-pci/variant drivers have a mismatch between module name and > driver name, so the current code would end up properly loading the > module, but then erroneously think that the driver wasn't available. > > This patch makes the code more forgiving by > > 1) checking for both $drivername and underscore($drivername) in > /sys/bus/pci/drivers > > 2) when we determine a module needs to be loaded, look at the link in > /sys/module/$modulename/driver/pci:$drivername to determine the > name of the driver we need to bind to the device(rather than just > assuming the driver has the same name as the module > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > --- > > Change from V1: I tried to simplify the explanation in the commit log message > > src/util/virpci.c | 193 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 163 insertions(+), 30 deletions(-) [...] > @@ -1154,44 +1161,166 @@ virPCIDeviceReset(virPCIDevice *dev, > } > > > +/** > + * virPCINameDashToUnderscore: > + * @path: the module/driver name or path - will be directly modified > + * > + * Replace all occurences of "-" with "_" in the name > + * part of the path (everything after final "/" > + * > + * return true if any change was made, otherwise false. > + */ > static int 'int' > -virPCIProbeDriver(const char *driverName) > +virPCINameDashToUnderscore(char *path) > { > - g_autofree char *drvpath = NULL; > + bool changed = false; 'bool' > + char *tmp = strrchr(path, '/'); > + > + if (!tmp) > + tmp = path; > + > + while (*tmp) { > + if (*tmp == '-') { > + *tmp = '_'; > + changed = true; > + } > + tmp++; > + } > + > + return changed; bool returned as int.

> +} > + > + > +static int > +virPCIProbeModule(const char *moduleName) > +{ > + g_autofree char *modulePath = NULL; > g_autofree char *errbuf = NULL; > > - drvpath = virPCIDriverDir(driverName); > + modulePath = virPCIModuleDir(moduleName); > > /* driver previously loaded, return */ > - if (virFileExists(drvpath)) > + if (virFileExists(modulePath)) > return 0; > > - if ((errbuf = virKModLoad(driverName))) { > - VIR_WARN("failed to load driver %s: %s", driverName, errbuf); > - goto cleanup; > + if ((errbuf = virKModLoad(moduleName))) { > + /* If we know failure was because of admin config, let's report that; > + * otherwise, report a more generic failure message > + */ > + if (virKModIsProhibited(moduleName)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to load PCI driver module %1$s: administratively prohibited"), Based on this message it's definitely not an VIR_ERR_INTERNAL_ERROR

> + moduleName); > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, and this too is not really internal > + _("Failed to load PCI driver module %1$s: %2$s"), > + moduleName, errbuf); > + } > + return -1; > } > > /* driver loaded after probing */ > - if (virFileExists(drvpath)) > + if (virFileExists(modulePath)) > return 0; > > - cleanup: > - /* If we know failure was because of admin config, let's report that; > - * otherwise, report a more generic failure message > + virReportError(VIR_ERR_INTERNAL_ERROR, and this too. > + _("modprobe reported success loading module '%1$s', but module is missing from /sys/module"), > + moduleName); > + return -1; > +} > + > +/** > + * virPCIDeviceFindDriver: > + * @dev: initialized virPCIDevice, including desired stubDriverName > + * > + * Checks if there is a driver named @dev->stubDriverName already > + * loaded. If there is, we're done. If not, look for a driver with > + * that same name, except with dashes replaced with underscores. > + > + * If neither of the above is found, then look for/load the module of > + * the underscored version of the name, and follow the links from > + * /sys/module/$name/drivers/pci:* to the PCI driver associated with that > + * module, and update @dev->stubDriverName with that name. > + * > + * On a successful return, @dev->stubDriverName will be updated with > + * the proper name for the driver, and that driver will be loaded. > + * > + * returns 0 on success, -1 on failure > + */ > +static int > +virPCIDeviceFindDriver(virPCIDevice *dev) > +{ > + g_autofree char *driverPath = virPCIDriverDir(dev->stubDriverName); > + g_autofree char *moduleName = NULL; > + g_autofree char *moduleDriversDir = NULL; > + g_autoptr(DIR) dir = NULL; > + struct dirent *ent; > + int direrr; > + > + /* unchanged stubDriverName */ > + if (virFileExists(driverPath)) > + return 0; > + > + /* try replacing "-" with "_" */ > + if (virPCINameDashToUnderscore(driverPath) && virFileExists(driverPath)) { > + > + /* update original name in dev */ > + virPCINameDashToUnderscore(dev->stubDriverName); > + return 0; > + } > + > + /* look for a module with this name (but always replacing > + * "-" with "_", since that's what modprobe does) > */ > - if (virKModIsProhibited(driverName)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to load PCI driver module %1$s: administratively prohibited"), > - driverName); > - } else { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to load PCI driver module %1$s"), > - driverName); > + > + moduleName = g_strdup(dev->stubDriverName); > + virPCINameDashToUnderscore(moduleName); > + > + if (virPCIProbeModule(moduleName) < 0) > + return -1; > + > + /* module was found/loaded. Now find the PCI driver it implements, > + * linked to by /sys/module/$moduleName/drivers/pci:$driverName > + */ > + > + moduleDriversDir = g_strdup_printf("/sys/module/%s/drivers", moduleName); > + > + if (virDirOpen(&dir, moduleDriversDir) < 0) > + return -1; > + > + while ((direrr = virDirRead(dir, &ent, moduleDriversDir))) { This is only assigned but not read. Also iteration must not continue if -1 is returned, which is usually what direrr is used for.

> + > + if (STRPREFIX(ent->d_name, "pci:")) { > + /* this is the link to the driver we want */ > + > + g_autofree char *drvName = NULL; > + g_autofree char *drvPath = NULL; > + > + /* extract the driver name from the link name */ > + drvName = g_strdup(ent->d_name + strlen("pci:")); > + > + /* make sure that driver is actually loaded */ > + drvPath = virPCIDriverDir(drvName); > + if (!virFileExists(drvPath)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, Not an VIR_ERR_INTERNAL_ERROR > + _("pci driver '%1$s' supposedly loaded by module '%2$s' not found in sysfs"), > + drvName, moduleName); > + return -1; > + } > + > + g_free(dev->stubDriverName); > + dev->stubDriverName = g_steal_pointer(&drvName); > + return 0; > + } > } > > + virReportError(VIR_ERR_INTERNAL_ERROR, Not an VIR_ERR_INTERNAL_ERROR > + _("module '%1$s' does not implement any pci driver"), > + moduleName); > return -1; > } > > + > int > virPCIDeviceUnbind(virPCIDevice *dev) > { > @@ -1291,7 +1420,6 @@ virPCIDeviceUnbindFromStub(virPCIDevice *dev) > static int > virPCIDeviceBindToStub(virPCIDevice *dev) > { > - const char *stubDriverName = dev->stubDriverName; > g_autofree char *stubDriverPath = NULL; > g_autofree char *driverLink = NULL; > > @@ -1303,30 +1431,35 @@ virPCIDeviceBindToStub(virPCIDevice *dev) > return -1; > } > > - if (!stubDriverName > - && !(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Unknown stub driver configured for PCI device %1$s"), > - dev->name); > - return -1; > + if (!dev->stubDriverName) { > + > + const char *stubDriverName = NULL; > + > + if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriverType))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, Not an VIR_ERR_INTERNAL_ERROR > + _("Unknown stub driver configured for PCI device %1$s"), > + dev->name); > + return -1; > + } > + dev->stubDriverName = g_strdup(stubDriverName); > } > > - if (virPCIProbeDriver(stubDriverName) < 0) > + if (virPCIDeviceFindDriver(dev) < 0) > return -1; > > - stubDriverPath = virPCIDriverDir(stubDriverName); > + stubDriverPath = virPCIDriverDir(dev->stubDriverName); > driverLink = virPCIFile(dev->name, "driver"); > > if (virFileExists(driverLink)) { > if (virFileLinkPointsTo(driverLink, stubDriverPath)) { > /* The device is already bound to the correct driver */ > VIR_DEBUG("Device %s is already bound to %s", > - dev->name, stubDriverName); > + dev->name, dev->stubDriverName); > return 0; > } > } > > - if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0) > + if (virPCIDeviceBindWithDriverOverride(dev, dev->stubDriverName) < 0) > return -1; > > dev->unbind_from_stub = true; With the stuff above addressed: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;