[libvirt] [PATCH 1/2] qemu: Create hugepage path on per domain basis

[Michal Privoznik]

On 25.11.2016 09:35, Martin Kletzander wrote:
> On Tue, Nov 22, 2016 at 01:45:42PM +0100, Michal Privoznik wrote:
>> If you've ever tried running a huge page backed guest under
>> different user than root, you probably failed. Problem is even
> 
> Surely you mean different than the default user from qemu.conf.
> 
>> though we have corresponding APIs in the security drivers,
>> there's no implementation and thus we don't relabel the huge page
>> path. But even if we did, so far all of the domains share the
>> same path:
>>
>>   /hugepageMount/libvirt/qemu
>>
>> Our only option there would be to set 0777 mode on the qemu dir
>> which is totally unsafe. Therefore, we can create dir on
>> per-domain basis, i.e.:
>>
>>   /hugepageMount/libvirt/qemu/domainName
>>
>> and chown domainName dir to the user that domain is configured to
>> run under.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/qemu/qemu_command.c                            |  4 +-
>> src/qemu/qemu_conf.c                               | 45
>> ++++++++++++++++------
>> src/qemu/qemu_conf.h                               | 16 +++++---
>> src/qemu/qemu_driver.c                             | 19 +++------
>> src/qemu/qemu_process.c                            | 25 +++++++++++-
>> .../qemuxml2argv-hugepages-numa.args               |  4 +-
>> .../qemuxml2argv-hugepages-pages.args              | 14 +++----
>> .../qemuxml2argv-hugepages-pages2.args             |  2 +-
>> .../qemuxml2argv-hugepages-pages3.args             |  2 +-
>> .../qemuxml2argv-hugepages-pages5.args             |  2 +-
>> .../qemuxml2argv-hugepages-shared.args             | 12 +++---
>> tests/qemuxml2argvdata/qemuxml2argv-hugepages.args |  2 +-
>> .../qemuxml2argv-memory-hotplug-dimm-addr.args     |  4 +-
>> .../qemuxml2argv-memory-hotplug-dimm.args          |  4 +-
>> 14 files changed, 97 insertions(+), 58 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 0ed88f5..942ad86 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -1468,8 +1468,26 @@ qemuGetHugepagePath(virHugeTLBFSPtr hugepage)
>> }
>>
>>
>> +char *
>> +qemuGetDomainHugepagePath(const virDomainDef *def,
>> +                          virHugeTLBFSPtr hugepage)
>> +{
>> +    char *base = qemuGetBaseHugepagePath(hugepage);
>> +    char *ret;
>> +
>> +    if (!base ||
>> +        virAsprintf(&ret, "%s/%s", base, def->name) < 0) {
>> +        VIR_FREE(base);
>> +        return NULL;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
> 
> You can't simply user the name because our restrictions for the name are
> too lax.  You should get unique directory name usable for this using
> virDomainObjGetShortName() to make sure the creation doesn't fail.

I thought that when we are using plain domain name for storing domain
status XML or pid file that I'm safe here too. But okay, I can change
it. I jut hope that by the time the command line is built domain already
has id allocated.

> 
> However, that reminds me that you might need to deal with similar thing
> I had to deal with when adding per-domain subdirectories for private
> domain paths.  You should save the path (or at least the information
> that the newer path is used) in the domain object and save/restore it
> in/from the state XML.  The way it's implemented now will break for
> example hotplug of hugepage-backed memory after libvirt upgrade.

Not really. We don't expose the path anywhere, and whenever it is needed
we construct it. I've tested this and basically the only problem I ran
into was that we don't build the path on domain hotplug (rather than on
domain startup), but it is trivial to fix.

Michal