[libvirt] [PATCH v2 3/3] Honor blacklist for modprobe command

[John Ferlan]

On 01/30/2014 07:05 AM, Daniel P. Berrange wrote:
> On Wed, Jan 29, 2014 at 07:52:45PM -0500, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1045124
>>
>> When loading modules, libvirt does not honor the modprobe blacklist.
>> By adding a "-b" to the modprobe command libvirt will fail to load a
>> module if it's on the blacklist
>>
>> Check if the failure to load a driver was due to it being on the blacklist
>> using the output of a "modprobe -c" searching for "blacklist <driver_Name>"
>> where driver_Name is possibly a modified string of the input driver changing
>> all '-' into '_' since that's what modprobe does.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index e2d222e..5d4168a 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -42,6 +42,7 @@
>>  #include "vircommand.h"
>>  #include "virerror.h"
>>  #include "virfile.h"
>> +#include "virkmod.h"
>>  #include "virstring.h"
>>  #include "virutil.h"
>>  
>> @@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver)
>>  {
>>      char *drvpath = NULL;
>>      bool probed = false;
>> +    size_t i;
>> +    char *drvblklst = NULL;
>> +    char *outbuf = NULL;
>>  
>>  recheck:
>>      if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) {
>> @@ -990,18 +994,44 @@ recheck:
>>      VIR_FREE(drvpath);
>>  
>>      if (!probed) {
>> -        const char *const probecmd[] = { MODPROBE, driver, NULL };
>> +        char *errbuf = NULL;
>>          probed = true;
>> -        if (virRun(probecmd, NULL) < 0) {
>> -            char ebuf[1024];
>> -            VIR_WARN("failed to load driver %s: %s", driver,
>> -                     virStrerror(errno, ebuf, sizeof(ebuf)));
>> -            return -1;
>> +        if ((errbuf = virModprobeUseBlacklist(driver))) {
>> +            VIR_WARN("failed to load driver %s: %s", driver, errbuf);
>> +            VIR_FREE(errbuf);
>> +            goto cleanup;
>>          }
>>  
>>          goto recheck;
>>      }
>>  
>> +    /* All error path code - purpose is to determine whether the failure
>> +     * occurs because device is on blacklist in order to add an error
>> +     * message to help detect why load failed
>> +     */
>> +    if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0)
>> +        goto cleanup;
>> +
>> +    /* modprobe will convert all '-' into '_', so we need to as well */
>> +    for (i = 0; i < drvblklst[i]; i++)
>> +        if (drvblklst[i] == '-')
>> +            drvblklst[i] = '_';
>> +
>> +    outbuf = virModprobeConfig();
>> +    if (!outbuf)
>> +        goto cleanup;
>> +
>> +    /* Find driver on blacklist? */
>> +    if (strstr(outbuf, drvblklst)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Failed to load PCI stub module %s: "
>> +                         "administratively prohibited"),
>> +                       driver);
>> +    }
> 
> So when I suggested creation of virkmod.{c,h} I was really meaning
> that all this code would go in there. eg here you'd just invoke
> 
>    if (virKModIsBlacklisted(driver)) {
>       ....report error...
>    }
> 
> so all the kmod config parsing would be isolated from this pci
> code.
> 

So here's the rub (since Jan asked and probably figured it out) -
'modprobe -b' returns 0 and no error when it finds the module on the
blacklist.

I guess I had "brain lock" over removing the VIR_WARN() and already
having gone through a round of testing by the bz submitter in a "real"
environment with the original set of changes.

This was so much simpler when all that was originally asked for was a
"-b" to be added to the MODPROBE command :-)...

A v3 will come soon.

John