[libvirt] [PATCH v5 06/11] qemu_cgroup: Allow /dev/mapper/control for PR

[Michal Privoznik]

On 04/29/2018 02:34 PM, John Ferlan wrote:
> 
> 
> On 04/23/2018 09:14 AM, Michal Privoznik wrote:
>> Just like in previous commit, qemu-pr-helper might want to open
>> /dev/mapper/control under certain circumstances. Therefore we
>> have to allow it in cgroups.
>>
>> The change virdevmapper.c might look spurious but it isn't. After
>> 6dd84f6850ca437 any path that we're allowing in deivces CGroup is
> 
> devices
> 
>> subject to virDevMapperGetTargets() inspection. And libdevmapper
>> returns ENXIO for the path from subject.
> 
> ^^^ IMO: This explanation (minus the commit id reference) belongs where
> you check for ENXIO. As a reader of code I don't necessarily check
> commit messages for reasons a check exists.
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/qemu/qemu_cgroup.c  | 33 ++++++++++++++++++++++++++++++---
>>  src/util/virdevmapper.c |  8 +++++++-
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
> 
> I would say a similar echo here as in the NS patch - since the
> subsequent patch will have a way to know that PR is running/started,
> then why not use that knowledge or similar logic to helper determine
> whether we need to add NS/Cgroup and whether we need to remove the
> cgroup reference (if that even matters by that point in time).

The check is there ...

> 
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index d88eb7881f..546a4c8e63 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>>  }
>>  
>>  
>> +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
>> +
>>  static int
>>  qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>>                               virStorageSourcePtr src,
>> @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
>>          return 0;
>>      }
>>  
>> +    if (virStoragePRDefIsManaged(src->pr) &&

.. here. If PR is managed, i.e. qemu-pr-helper is started by libvirt,
then it will be also placed into the same CGroup as qemu. Therefore, we
need to allow access to /dev/mapper/control.
But, it PR is unmanaged, then the helper process is started by somebody
else (we don't care who), and therefore it's their responsibility to
allow that path in CGroups (if they are placing the helper into any).

The only caveat here is that qemu-pr-helper might access the path only
sometimes, depending whether it was compiled with multipath support or
not. But that's something libvirt can't know. Therefore it has to assume
yes and allow the path.

>> +        qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
>> +        return -1;
>> +
>>      return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly);
>>  }
>>  
>> @@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>>                          virStorageSourcePtr src)
>>  {
>>      qemuDomainObjPrivatePtr priv = vm->privateData;
>> -    int perms = VIR_CGROUP_DEVICE_READ |
>> -                VIR_CGROUP_DEVICE_WRITE |
>> -                VIR_CGROUP_DEVICE_MKNOD;
>> +    int perms = VIR_CGROUP_DEVICE_RWM;
>> +    size_t i;
>>      int ret;
>>  
>>      if (!virCgroupHasController(priv->cgroup,
>> @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
>>          return 0;
>>      }
>>  
>> +    for (i = 0; i < vm->def->ndisks; i++) {
>> +        virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
>> +
>> +        if (src == diskSrc)
>> +            continue;
>> +
>> +        if (virStoragePRDefIsManaged(diskSrc->pr))
>> +            break;
>> +    }
>> +
>> +    if (i == vm->def->ndisks) {
>> +        VIR_DEBUG("Disabling device mapper control");
>> +        ret = virCgroupDenyDevicePath(priv->cgroup,
>> +                                      DEVICE_MAPPER_CONTROL_PATH, perms, true);
>> +        virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
>> +                                 DEVICE_MAPPER_CONTROL_PATH,
>> +                                 virCgroupGetDevicePermsString(perms), ret);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +
>>      VIR_DEBUG("Deny path %s", src->path);
>>  
>>      ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
>> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
>> index d2c25af003..ef4b1e480a 100644
>> --- a/src/util/virdevmapper.c
>> +++ b/src/util/virdevmapper.c
>> @@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path,
>>  
>>      dm_task_no_open_count(dmt);
>>  
>> -    if (!dm_task_run(dmt))
>> +    if (!dm_task_run(dmt)) {
>> +        if (errno == ENXIO) {
>> +            /* In some cases devmapper realizes this late device
>> +             * is not managed by it. */
> 
> So my question here is that is "some cases" limited to just one device
> or would it be multiple? 

To be fair I haven't tested any other devices. Okay, I'll fix the comment.

> Reviewed-by: John Ferlan <jferlan at redhat.com>

Thanks,
Michal