December 2014 - Devel - libvirt List Archives

[libvirt] [PATCH] qemu: Allow system pages to <memoryBacking/>
by Michal Privoznik 12 Dec '14

12 Dec '14

It occurred to me that OpenStack uses the following XML when not using regular huge pages: <memoryBacking> <hugepages> <page size='4' unit='KiB'/> </hugepages> </memoryBacking> However, since we are expecting to see huge pages only, we fail to startup the domain with following error: libvirtError: internal error: Unable to find any usable hugetlbfs mount for 4 KiB While regular system pages are not huge pages technically, our code is prepared for that and if it helps OpenStack (or other management applications) we should cope with that. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_command.c | 84 +++++++++++++--------- .../qemuxml2argv-hugepages-pages6.args | 4 ++ .../qemuxml2argv-hugepages-pages6.xml | 32 +++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 87 insertions(+), 34 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 48bdf4e..c44560f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6616,6 +6616,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, char *nodemask = NULL; char *mem_path = NULL; int ret = -1; + const long system_page_size = sysconf(_SC_PAGESIZE); if (virDomainNumatuneHasPerNodeBinding(def->numatune) && !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || @@ -6626,7 +6627,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (def->mem.nhugepages && def->mem.hugepages[0].size && + if (def->mem.nhugepages && + def->mem.hugepages[0].size != system_page_size && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("huge pages per NUMA node are not " @@ -6720,6 +6722,13 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, hugepage = master_hugepage; } + if (hugepage && hugepage->size == system_page_size) { + /* However, if user specified to use "huge" page + * of regular system page size, it's as if they + * hasn't specified any huge pages at all. */ + hugepage = NULL; + } + if (hugepage) { /* Now lets see, if the huge page we want to use is even mounted * and ready to use */ @@ -7882,44 +7891,51 @@ qemuBuildCommandLine(virConnectPtr conn, def->mem.max_balloon = VIR_DIV_UP(def->mem.max_balloon, 1024) * 1024; virCommandAddArgFormat(cmd, "%llu", def->mem.max_balloon / 1024); if (def->mem.nhugepages && (!def->cpu || !def->cpu->ncells)) { - char *mem_path; + const long system_page_size = sysconf(_SC_PAGESIZE) / 1024; + char *mem_path = NULL; - if (!cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugetlbfs filesystem is not mounted " - "or disabled by administrator config")); - goto error; - } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("hugepage backing not supported by '%s'"), - def->emulator); - goto error; - } - - if (def->mem.hugepages[0].size) { - for (j = 0; j < cfg->nhugetlbfs; j++) { - if (cfg->hugetlbfs[j].size == def->mem.hugepages[0].size) - break; - } - - if (j == cfg->nhugetlbfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find any usable hugetlbfs mount for %llu KiB"), - def->mem.hugepages[0].size); - goto error; - } - - if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) - goto error; + if (def->mem.hugepages[0].size == system_page_size) { + /* There is one special case: if user specified "huge" + * pages of regular system pages size. */ } else { - if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, - cfg->nhugetlbfs))) + if (!cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("hugetlbfs filesystem is not mounted " + "or disabled by administrator config")); goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hugepage backing not supported by '%s'"), + def->emulator); + goto error; + } + + if (def->mem.hugepages[0].size) { + for (j = 0; j < cfg->nhugetlbfs; j++) { + if (cfg->hugetlbfs[j].size == def->mem.hugepages[0].size) + break; + } + + if (j == cfg->nhugetlbfs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find any usable hugetlbfs mount for %llu KiB"), + def->mem.hugepages[0].size); + goto error; + } + + if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[j]))) + goto error; + } else { + if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, + cfg->nhugetlbfs))) + goto error; + } } - virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", - mem_path, NULL); + virCommandAddArg(cmd, "-mem-prealloc"); + if (mem_path) + virCommandAddArgList(cmd, "-mem-path", mem_path, NULL); VIR_FREE(mem_path); } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args new file mode 100644 index 0000000..4eccb86 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 1024 -mem-prealloc -smp 2 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.xml b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.xml new file mode 100644 index 0000000..4f318fd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages6.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>SomeDummyHugepagesGuest</name> + <uuid>ef1bdff4-27f3-4e85-a807-5fb4d58463cc</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <memoryBacking> + <hugepages> + <page size='4' unit='KiB'/> + </hugepages> + </memoryBacking> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9e39068..bce88a8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -713,6 +713,7 @@ mymain(void) DO_TEST_FAILURE("hugepages-pages4", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("hugepages-pages5", QEMU_CAPS_MEM_PATH); + DO_TEST("hugepages-pages6", NONE); DO_TEST("nosharepages", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE); DO_TEST("disk-cdrom-network-http", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE, -- 2.0.4

2 1

[libvirt] [PATCH] conf: Ignore device address for model=none usb controller and memballon
by Luyao Huang 12 Dec '14

12 Dec '14

It make no sense at all to have it there. Signed-off-by: Luyao Huang <lhuang(a)redhat.com> --- src/conf/domain_conf.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec45b8c..2965d8d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6683,8 +6683,12 @@ virDomainControllerDefParseXML(xmlNodePtr node, _("Malformed 'max_sectors' value %s'"), max_sectors); } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + VIR_DEBUG("Ignoring device address for none model usb controller"); + } else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) { goto error; + } switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: { @@ -9989,7 +9993,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, goto error; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) + VIR_DEBUG("Ignoring device address for none model Memballoon"); + else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; cleanup: -- 1.8.3.1

2 3

[libvirt] [PATCH v4 00/17] LXC IP configuration feature
by Cédric Bosdonnat 12 Dec '14

12 Dec '14

Hi all, Here is an updated version of the lxc IP configuration patch series. Diffs to v3: * Took care of Daniel's feedback * Fixed openvz driver to add support for multiple --ipadd * Report an error for drivers using IP addresses, but only the first one in case there are more provided. * Squashed Patch 11 to have virDomainNetIpsFormat right in the first place * Fixed regression introduced in qemuConnectDomainXMLToNative: IPs were not kept over memset for bridge type. * Reformatted to fit latest make syntax-check changes Cédric Bosdonnat (17): Forgot to cleanup ifname_guest* in domain network def parsing Domain conf: allow more than one IP address for net devices LXC: set IP addresses to veth devices in the container lxc conf2xml: convert IP addresses Allow network capabilities hostdev to configure IP addresses lxc conf2xml: convert ip addresses for hostdev NICs Domain network devices can now have a <gateway> element lxc conf2xml: convert lxc.network.ipv[46].gateway LXC: use the new net devices gateway definition LXC: honour network devices link state virNetDevSetIPv4Address: libnl implementation Renamed virNetDevSetIPv4Address to virNetDevSetIPAddress virNetDevAddRoute: implementation using netlink virNetDevClearIPv4Address: netlink implementation Renamed virNetDevClearIPv4Address to virNetDevClearIPAddress Openvz --ipadd can be provided multiple times Report error if a driver can't handle multiple IP addresses docs/formatdomain.html.in | 39 +++ docs/schemas/domaincommon.rng | 65 ++++- src/conf/domain_conf.c | 251 +++++++++++++++-- src/conf/domain_conf.h | 21 +- src/libvirt_private.syms | 6 +- src/lxc/lxc_container.c | 69 ++++- src/lxc/lxc_native.c | 165 +++++++---- src/network/bridge_driver.c | 4 +- src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 11 +- src/qemu/qemu_driver.c | 29 +- src/qemu/qemu_hotplug.c | 5 +- src/uml/uml_conf.c | 2 +- src/util/virnetdev.c | 305 ++++++++++++++++++--- src/util/virnetdev.h | 12 +- src/util/virnetlink.c | 38 +++ src/util/virnetlink.h | 2 + src/util/virsocketaddr.h | 3 + src/vbox/vbox_common.c | 16 +- src/xenconfig/xen_common.c | 29 +- src/xenconfig/xen_sxpr.c | 26 +- .../lxcconf2xmldata/lxcconf2xml-physnetwork.config | 4 + tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 + tests/lxcconf2xmldata/lxcconf2xml-simple.config | 4 + tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 + tests/lxcxml2xmldata/lxc-hostdev.xml | 4 + tests/lxcxml2xmldata/lxc-idmap.xml | 3 + tests/openvzutilstest.c | 2 +- tests/sexpr2xmldata/sexpr2xml-bridge-ipaddr.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-net-routed.xml | 2 +- 30 files changed, 963 insertions(+), 166 deletions(-) -- 2.1.2

4 25

[libvirt] [PATCH] CVE-2014-8131: Fix possible deadlock and segfault in qemuConnectGetAllDomainStats()
by Martin Kletzander 12 Dec '14

12 Dec '14

When user doesn't have read access on one of the domains he requested, the for loop could exit abruptly or continue and override pointer which pointed to locked object. This patch fixed two issues at once. One is that domflags might have had QEMU_DOMAIN_STATS_HAVE_JOB even when there was no job started (this is fixed by doing domflags |= QEMU_DOMAIN_STATS_HAVE_JOB only when the job was acquired and cleaning domflags on every start of the loop. Second one is that the domain is kept locked when virConnectGetAllDomainStatsCheckACL() fails and continues the loop when it didn't end. Adding a simple virObjectUnlock() and clearing the pointer ought to do. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- Since this is very low priority and it was mistakenly disclosed in another patch there is no embargo on this CVE. Patches to maint branches will be pushed after back-porting (couple of minutes). Having said that, I am probably not the right one to write up the libvirt security notice, but I'll try drafting one, eventually. src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index be37c8f..ae6225b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18740,20 +18740,23 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; for (i = 0; i < ndoms; i++) { - domflags = privflags; virDomainStatsRecordPtr tmp = NULL; + domflags = 0; if (!(dom = qemuDomObjFromDomain(doms[i]))) continue; if (doms != domlist && - !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) + !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) { + virObjectUnlock(dom); + dom = NULL; continue; + } - if (HAVE_JOB(domflags) && + if (HAVE_JOB(privflags) && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) /* As it was never requested. Gather as much as possible anyway. */ - domflags &= ~QEMU_DOMAIN_STATS_HAVE_JOB; + domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0) goto endjob; @@ -18761,9 +18764,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (tmp) tmpstats[nstats++] = tmp; - if (HAVE_JOB(domflags) && !qemuDomainObjEndJob(driver, dom)) { - dom = NULL; - continue; + if (HAVE_JOB(domflags)) { + domflags = 0; + if (!qemuDomainObjEndJob(driver, dom)) { + dom = NULL; + continue; + } } virObjectUnlock(dom); -- 2.2.0

2 3

[libvirt] [PATCH] qemu: bulk stats: typo in monitor handling
by Francesco Romani 11 Dec '14

11 Dec '14

A typo in qemuConnectGetAllDomainStats makes the code mark the monitor as available when qemuDomainObjBeginJob fails, instead of when it succeeds, as the correct flow requires. This patch fixes the check and updates the code documentation accordingly. Signed-off-by: Francesco Romani <fromani(a)redhat.com> --- src/qemu/qemu_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 830fca7..129e10c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18745,9 +18745,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, } if (HAVE_JOB(privflags) && - qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) - /* As it was never requested. Gather as much as possible anyway. */ + qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) == 0) domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + /* + * else: as it was never requested. + * Gather as much as possible anyway. + */ if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0) goto endjob; -- 1.9.3

4 3

[libvirt] [PATCH] qemu: bulk stats: Clean up code to check whether we need a domain job
by Peter Krempa 11 Dec '14

11 Dec '14

Storing the information as a flag in a unsigned int seems a bit wasteful. Store it in a bool instead. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..5c60127 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18691,7 +18691,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, int nstats = 0; size_t i; int ret = -1; - unsigned int privflags = 0; + bool needjob; unsigned int domflags = 0; if (ndoms) @@ -18727,8 +18727,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) goto cleanup; - if (qemuDomainGetStatsNeedMonitor(stats)) - privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + bool needjob = qemuDomainGetStatsNeedMonitor(stats); for (i = 0; i < ndoms; i++) { virDomainStatsRecordPtr tmp = NULL; @@ -18744,7 +18743,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, continue; } - if (HAVE_JOB(privflags) && + if (needjob && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) == 0) domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; /* else: without a job it's still possible to gather some data */ -- 2.2.0

2 1

Re: [libvirt] [virt-tools-list] [PATCH] virtinst: refresh pools status before fetch_pools
by Chun Yan Liu 11 Dec '14

11 Dec '14

>>> On 12/5/2014 at 09:54 PM, in message <5481B907.4040507(a)redhat.com>, Cole Robinson <crobinso(a)redhat.com> wrote: > On 12/05/2014 03:40 AM, Chunyan Liu wrote: > > Currently, when connecting to hypervisor, if there are pools active > > but in fact target path already deleted (or for other reasons the > > pool is not working), libvirtd not refresh status yet, fetch_pools > > will fail, that will cause "connecting to hypervisor" process > > reporting error and exit. The whole connection work failed. > > > > With the patch, always refresh pool status before fetch pools. Let > > the libvirtd pool status reflect the reality, avoid the non-synced > > status affects the hypervisor connection. > > > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com> > > --- > > virtinst/pollhelpers.py | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/virtinst/pollhelpers.py b/virtinst/pollhelpers.py > > index a9b1527..e8702f0 100644 > > --- a/virtinst/pollhelpers.py > > +++ b/virtinst/pollhelpers.py > > @@ -133,6 +133,19 @@ def fetch_pools(backend, origmap, build_func): > > > > if backend.check_support( > > backend.SUPPORT_CONN_LISTALLSTORAGEPOOLS) and not > _force_old_poll: > > + > > + # Refresh pools before poll_helper. For those > > + # 'active' but target path not exist (or other reasons > > + # causing the pool not working), but libvirtd not > > + # refresh the status, this will make it refreshed > > + # and mark that pool as 'inactive'. > > + objs = backend.listAllStoragePools() > > + for obj in objs: > > + try: > > + obj.refresh(0) > > + except Exception, e: > > + pass > > + > > return _new_poll_helper(origmap, name, > > backend.listAllStoragePools, build_func) > > else: > > > > This is a very heavy hammer, refresh is a potentially long running operation > > so this could cause decent slowdown in some scenarios. > > IMO this is essentially a libvirt bug, for pools with target directories > (dir, > fs, netfs), libvirt should be periodically checking the directory ctime and > doing the pool refresh for us. And if the target has disappeared, it shuts > down the pool (like shutting down a VM if it crashes). Hi, libvirt list, I'm not sure if Cole's suggestion could be done in libvirt, so just forward the mail to libvirt mailing list. Any opinions? Chunyan > > We have so many hacks sprinkled around in virtinst/virt-manager dealing with > the fallout of this lacking libvirt feature. Really wish I had implemented > it > years ago. But I'd rather focus on that then adding yet more hacks. > > If there's a specific issue you're hitting that's manifesting itself > elsewhere > in the app, let us know and maybe there's a way to mitigate it in the > interim > > - Cole > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/virt-tools-list > >

3 2

[libvirt] [PATCHv3 0/2] Fix snapshot-revert and managedsave with host-passthrough CPU mode
by Ján Tomko 11 Dec '14

11 Dec '14

v1: https://www.redhat.com/archives/libvir-list/2014-October/msg01120.html v2: https://www.redhat.com/archives/libvir-list/2014-November/msg00218.html v3: * do not format the features at all, instead of adding a model to them Ján Tomko (2): Do not format CPU features without a model Ignore CPU features without a model for host-passthrough src/conf/cpu_conf.c | 7 ++- ...argv-cpu-host-passthrough-features-invalid.args | 22 +++++++++ ...2argv-cpu-host-passthrough-features-invalid.xml | 55 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-host-passthrough-features-invalid.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-host-passthrough-features-invalid.xml -- 2.0.4

2 4

[libvirt] [PATCH] storage: unify permission formatting
by Martin Kletzander 11 Dec '14

11 Dec '14

Volume and pool formatting functions took different approaches to unspecified uids/gids. When unknown, it is always parsed as -1, but one of the functions formatted it as unsigned int (wrong) and one as int (better). Due to that, our two of our XML files from tests cannot be parsed on 32-bit machines. RNG schema needs to be modified as well, but because both storagepool.rng and storagevol.rng need same schema for permission element, save some space by moving it to storagecommon.rng. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- docs/schemas/storagecommon.rng | 29 +++++++++++++++++++++++++ docs/schemas/storagepool.rng | 30 +------------------------- docs/schemas/storagevol.rng | 23 -------------------- src/conf/storage_conf.c | 9 ++++---- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 4 ++-- tests/storagevolxml2xmlout/vol-sheepdog.xml | 4 ++-- 6 files changed, 38 insertions(+), 61 deletions(-) diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 06b2f81..629505f 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -93,4 +93,33 @@ <notAllowed/> </define> + <define name='permissions'> + <optional> + <element name='permissions'> + <interleave> + <element name='mode'> + <ref name='octalMode'/> + </element> + <element name='owner'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + <element name='group'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + <optional> + <element name='label'> + <text/> + </element> + </optional> + </interleave> + </element> + </optional> + </define> + </grammar> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 0f05c5c..db6ff49 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -3,6 +3,7 @@ <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <include href='basictypes.rng'/> + <include href='storagecommon.rng'/> <start> <ref name='pool'/> </start> @@ -224,35 +225,6 @@ </interleave> </define> - <define name='permissions'> - <optional> - <element name='permissions'> - <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> - <element name='owner'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> - <element name='group'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> - <optional> - <element name='label'> - <text/> - </element> - </optional> - </interleave> - </element> - </optional> - </define> - <define name='target'> <element name='target'> <interleave> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 1b2d4cc..7450547 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -59,29 +59,6 @@ </interleave> </define> - <define name='permissions'> - <optional> - <element name='permissions'> - <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> - <element name='owner'> - <ref name='unsignedInt'/> - </element> - <element name='group'> - <ref name='unsignedInt'/> - </element> - <optional> - <element name='label'> - <text/> - </element> - </optional> - </interleave> - </element> - </optional> - </define> - <define name='timestamps'> <optional> <element name='timestamps'> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3987470..e1be064 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1203,7 +1203,6 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) (int) def->target.perms.uid); virBufferAsprintf(&buf, "<group>%d</group>\n", (int) def->target.perms.gid); - virBufferEscapeString(&buf, "<label>%s</label>\n", def->target.perms.label); @@ -1527,10 +1526,10 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAsprintf(buf, "<mode>0%o</mode>\n", def->perms->mode); - virBufferAsprintf(buf, "<owner>%u</owner>\n", - (unsigned int) def->perms->uid); - virBufferAsprintf(buf, "<group>%u</group>\n", - (unsigned int) def->perms->gid); + virBufferAsprintf(buf, "<owner>%d</owner>\n", + (int) def->perms->uid); + virBufferAsprintf(buf, "<group>%d</group>\n", + (int) def->perms->gid); virBufferEscapeString(buf, "<label>%s</label>\n", diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index f188ceb..538b31d 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -10,8 +10,8 @@ <format type='dir'/> <permissions> <mode>0600</mode> - <owner>4294967295</owner> - <group>4294967295</group> + <owner>-1</owner> + <group>-1</group> </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index e08e36c..0a1f32c 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -9,8 +9,8 @@ <format type='unknown'/> <permissions> <mode>0600</mode> - <owner>4294967295</owner> - <group>4294967295</group> + <owner>-1</owner> + <group>-1</group> </permissions> </target> </volume> -- 2.2.0

2 2

[libvirt] [PATCHv4 2/2] security: Add a new func use stat to get process DAC label
by Luyao Huang 11 Dec '14

11 Dec '14

When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel. Signed-off-by: Luyao Huang <lhuang(a)redhat.com> --- v2 add support freeBSD. v3 use snprintf instead of VirAsprintf and move the error settings in virSecurityDACGetProcessLabelInternal. v4 remove errno.h include and thanks Eric advice move this version comment to this place. src/security/security_dac.c | 84 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 3 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..300b245 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -23,6 +23,11 @@ #include <sys/stat.h> #include <fcntl.h> +#ifdef __FreeBSD__ +# include <sys/sysctl.h> +# include <sys/user.h> +#endif + #include "security_dac.h" #include "virerror.h" #include "virfile.h" @@ -1236,17 +1241,90 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +#ifdef __linux__ +static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; + + if (lstat(path, &sb) < 0) { + virReportSystemError(errno, + _("unable to get PID %d uid and gid via stat"), + pid); + goto cleanup; + } + + snprintf(seclabel->label, VIR_SECURITY_LABEL_BUFLEN, + "+%u:+%u", (unsigned int) sb.st_uid, (unsigned int) sb.st_gid); + ret = 0; + +cleanup: + VIR_FREE(path); + return ret; +} +#elif defined(__FreeBSD__) +static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct kinfo_proc p; + int mib[4]; + size_t len = 4; + + sysctlnametomib("kern.proc.pid", mib, &len); + + len = sizeof(struct kinfo_proc); + mib[3] = pid; + + if (sysctl(mib, 4, &p, &len, NULL, 0) < 0) { + virReportSystemError(errno, + _("unable to get PID %d uid and gid via sysctl"), + pid); + return -1; + } + + snprintf(seclabel->label, VIR_SECURITY_LABEL_BUFLEN, + "+%u:+%u", (unsigned int) p.ki_ruid, (unsigned int) p.ki_rgid); + + return 0; +} +#else +static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + "Cannot get proccess DAC label for pid %d on this platform", + (int) pid); + return -1; +} +#endif + static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!secdef || !seclabel) - return -1; + if (secdef == NULL) { + VIR_DEBUG("missing label for DAC security " + "driver in domain %s", def->name); + + if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) + return -1; + return 0; + } if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, -- 1.8.3.1

3 2