Ján Tomko [Fri, 14 Jun 2019 07:17:39 +0000 (09:17 +0200)]
api: disallow virConnect*HypervisorCPU on read-only connections
These APIs can be used to execute arbitrary emulators.
Forbid them on read-only connections.
Fixes: CVE-2019-10168 Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit bf6c2830b6c338b1f5699b095df36f374777b291) Signed-off-by: Ján Tomko <jtomko@redhat.com>
Ján Tomko [Fri, 14 Jun 2019 07:16:14 +0000 (09:16 +0200)]
api: disallow virConnectGetDomainCapabilities on read-only connections
This API can be used to execute arbitrary emulators.
Forbid it on read-only connections.
Fixes: CVE-2019-10167 Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 8afa68bac0cf99d1f8aaa6566685c43c22622f26) Signed-off-by: Ján Tomko <jtomko@redhat.com>
Ján Tomko [Fri, 14 Jun 2019 07:14:53 +0000 (09:14 +0200)]
api: disallow virDomainManagedSaveDefineXML on read-only connections
The virDomainManagedSaveDefineXML can be used to alter the domain's
config used for managedsave or even execute arbitrary emulator binaries.
Forbid it on read-only connections.
Fixes: CVE-2019-10166 Reported-by: Matthias Gerstner <mgerstner@suse.de> Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit db0b78457f183e4c7ac45bc94de86044a1e2056a) Signed-off-by: Ján Tomko <jtomko@redhat.com>
Ján Tomko [Fri, 14 Jun 2019 06:47:42 +0000 (08:47 +0200)]
api: disallow virDomainSaveImageGetXMLDesc on read-only connections
The virDomainSaveImageGetXMLDesc API is taking a path parameter,
which can point to any path on the system. This file will then be
read and parsed by libvirtd running with root privileges.
Forbid it on read-only connections.
Fixes: CVE-2019-10161 Reported-by: Matthias Gerstner <mgerstner@suse.de> Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit aed6a032cead4386472afb24b16196579e239580) Signed-off-by: Ján Tomko <jtomko@redhat.com>
The virtlogd daemon's only intended client is the libvirtd daemon. As
such it should never allow clients from other user accounts to connect.
The code already enforces this and drops clients from other UIDs, but
we can get earlier (and thus stronger) protection against DoS by setting
the socket permissions to 0600
Fixes CVE-2019-10132
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit e37bd65f9948c1185456b2cdaa3bd6e875af680f)
The virtlockd daemon's only intended client is the libvirtd daemon. As
such it should never allow clients from other user accounts to connect.
The code already enforces this and drops clients from other UIDs, but
we can get earlier (and thus stronger) protection against DoS by setting
the socket permissions to 0600
Fixes CVE-2019-10132
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit f111e09468693909b1f067aa575efdafd9a262a1)
admin: reject clients unless their UID matches the current UID
The admin protocol RPC messages are only intended for use by the user
running the daemon. As such they should not be allowed for any client
UID that does not match the server UID.
Fixes CVE-2019-10132
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 96f41cd765c9e525fe28ee5abbfbf4a79b3720c7)
The bit is set when microcode provides the mechanism to invoke a flush
of various exploitable CPU buffers by invoking the VERW instruction.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 538d873571d7a682852dc1d70e5f4478f4d64e85)
Conflicts:
tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-guest.xml
tests/cputestdata/x86_64-cpuid-Xeon-Platinum-8268-host.xml
- test data missing downstream
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
cputest: Add data for Intel(R) Xeon(R) CPU E3-1225 v5
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
(cherry picked from commit 5cd9db3ac11e88846cbcf95fad9f6fae9d880dee)
My earlier commit be46f61326 was incomplete. It removed caching of
microcode version in the CPU driver, which means the capabilities XML
will see the correct microcode version. But it is also cached in the
QEMU capabilities cache where it is used to detect whether we need to
reprobe QEMU. By missing the second place, the original commit be46f61326 made the situation even worse since libvirt would report
correct microcode version while still using the old host CPU model
(visible in domain capabilities XML).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
(cherry picked from commit 673c62a3b7855a0685d8f116e227c402720b9ee9)
The microcode version checks are used to invalidate cached CPU data we
get from QEMU. To minimize /proc/cpuinfo parsing the microcode version
was only read when libvirtd started and cached for the daemon's
lifetime. However, the CPU microcode can change anytime (updating the
microcode package can automatically upload it to the CPU) and we need to
stop caching it to avoid using stale CPU model data.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
(cherry picked from commit be46f613261d3b655a1f15afd635087e68a9c39b)
network: avoid trying to create global firewall rules if unprivileged
The unprivileged libvirtd does not have permission to create firewall
rules, or bridge devices, or do anything to the host network in
general. Historically we still activate the network driver though and
let the network start API call fail.
The startup code path which reloads firewall rules on active networks
would thus effectively be a no-op when unprivileged as it is impossible
for there to be any active networks
With the change to use a global set of firewall chains, however, we now
have code that is run unconditionally.
Ideally we would not register the network driver at all when
unprivileged, but the entanglement with the virt drivers currently makes
that impractical. As a temporary hack, we just make the firewall reload
into a no-op.
network: split setup of ipv4 and ipv6 top level chains
During startup libvirtd creates top level chains for both ipv4
and ipv6 protocols. If this fails for any reason then startup
of virtual networks is blocked.
The default virtual network, however, only requires use of ipv4
and some servers have ipv6 disabled so it is expected that ipv6
chain creation will fail. There could equally be servers with
no ipv4, only ipv6.
This patch thus makes error reporting a little more fine grained
so that it works more sensibly when either ipv4 or ipv6 is
disabled on the server. Only the protocols that are actually
used by the virtual network have errors reported.
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 686803a1a2e1e0641916b1c9e2c7e3910fe598d4)
network: improve error report when firewall chain creation fails
During startup we create some top level chains in which all
virtual network firewall rules will be placed. The upfront
creation is done to avoid slowing down creation of individual
virtual networks by checking for chain existance every time.
There are some factors which can cause this upfront creation
to fail and while a message will get into the libvirtd log
this won't be seen by users who later try to start a virtual
network. Instead they'll just get a message saying that the
libvirt top level chain does not exist. This message is
accurate, but unhelpful for solving the root cause.
This patch thus saves any error during daemon startup and
reports it when trying to create a virtual network later.
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 9f4e35dc73ec9e940aa61bc7c140c2b800218ef3)
The rbd_list method has been deprecated in Ceph >= 14.0.0
in favour of the new rbd_list2 method which populates an
array of structs.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 3aa190f2a43a632b542a6ba751a6c3ab4d51f1dd)
The rbd_list method has a quite unpleasant signature returning an
array of strings in a single buffer instead of an array. It is
being deprecated in favour of rbd_list2. To maintain clarity of
code when supporting both APIs in parallel, split the rbd_list
code out into a separate method.
In splitting this we now honour the rbd_list failures.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 28c8403ed07896d6d7e06d7726ed904027206719)
Eric Blake [Wed, 27 Feb 2019 19:38:18 +0000 (13:38 -0600)]
qemu: Fix snapshot redefine vs. domain state bug
The existing qemu snapshot code has a slight bug: if the domain
is currently pmsuspended, you can't use the _REDEFINE flag even
though the current domain state should have no bearing on being
able to recreate metadata state; and conversely, you can use the
_REDEFINE flag to create snapshot metadata claiming to be
pmsuspended as a bypass to the normal restrictions that you can't
create an original qemu snapshot in that state (the restriction
against pmsuspend is specific to qemu, rather than part of the
driver-agnostic snapshot_conf code).
Fix this by checking the snapshot state (when redefining) instead
of the domain state (which is a subset of snapshot states).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Jiri Denemark [Thu, 28 Feb 2019 14:12:31 +0000 (15:12 +0100)]
storage: Fix iscsi-direct volume size for volumes > 4GiB
Both block_size and nb_block are unit32_t and multiplying them overflows
at 4GiB.
Moreover, the iscsi_*10_* APIs use 32bit number of blocks and thus they
can only address images up to 2TiB with 512B blocks. Let's use 64b
iscsi_*16_* APIs instead.
Michal Privoznik [Thu, 28 Feb 2019 14:08:19 +0000 (15:08 +0100)]
virISCSIDirectRefreshVol: Don't clear volumes in each run
When fetching LUNs from iscsi server the
virISCSIDirectReportLuns() is called. This function does some
libiscsi calls and then calls virISCSIDirectRefreshVol() over
each LUN found. It's unfortunate that the latter calls
virStoragePoolObjClearVols() as we lose all LUNs processed
in previous iterations.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Michal Privoznik [Thu, 28 Feb 2019 10:16:51 +0000 (11:16 +0100)]
news: Update for 5.1.0 release
Not exhaustive list of new features, improvements and bugfixes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Michal Privoznik [Thu, 28 Feb 2019 11:02:06 +0000 (12:02 +0100)]
iscsi_direct: Reset pool capacity and allocation just before refresh
Jirka reported a bug that with every 'virsh pool-refresh' an
iscsi-direct pool would grow and grow. The problem is that
virISCSIDirectRefreshVol() only adds to def->capacity and
def->allocation but nothing clears it out to begin with.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Eric Blake [Mon, 25 Feb 2019 16:37:58 +0000 (10:37 -0600)]
snapshot: Improve message for VIR_ERR_INVALID_DOMAIN_SNAPSHOT
For consistency with other error messages, and the fact that
the object is always called a virDomainSnapshot rather than
a mere virSnapshot, include the word "domain" in the error
message.
Suggested-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Eric Blake [Wed, 13 Feb 2019 23:07:32 +0000 (17:07 -0600)]
domain: Document VIR_DOMAIN_XML_MIGRATABLE
Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but
failed to document its effects. And considering that the
MIGRATABLE flag has been the source of past bugs (CVE-2014-7823,
fixed in commit b1674ad5 (1.2.11), or even cf2d4c60 (1.2.13) where
flag mismatch broke virsh edit), make the wording wishy-washy
enough to discourage using the flag casually, by mentioning that
the resulting XML is more for internal use than for validation
against the schema.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Sat, 23 Feb 2019 19:21:38 +0000 (13:21 -0600)]
snapshot: Permit redefine of offline external snapshot
Due to historical back-compat, bare 'virsh snapshot-create-as'
favors internal snapshots (but can't be used on domains with raw
storage), while 'virsh snapshot-create-as --disk-only' favors
external snapshots. What's more, snapshots created with
--disk-only while the domain was running are marked as snapshot
state 'disk-snapshot', while snapshots created while the domain
was offline are marked as snapshot state 'shutdown' (a
'disk-snapshot' image might not be quiescent, while a 'shutdown'
snapshot always is).
But this leads to some interesting problems: if we create a
--disk-only snapshot of an offline guest, and then immediately try
to 'virsh snapshot-create --redefine' using the resulting XML to
overwrite the existing snapashot in place, things silently succeed,
but 'virsh snapshot-create --redefine --disk-only' fails with an
error message that the snapshot state is not 'disk-only'. Worse,
if we delete the snapshot metadata first and then try to recreate
things, omitting --disk-only fails because the verification code
wants to force the default of an internal snapshot (which doesn't
work with raw disks), and using --disk-only still fails because the
snapshot XML is not 'disk-only' - making it impossible to recreate
the snapshot metadata (or to transfer it from one libvirtd host to
another). Ideally, the presence or absence of the --disk-only
flag, and the presence or absence of an existing snapshot being
overwritten, shouldn't matter; if the XML is valid for one
situation, it should always be valid to redefine the metadata for
that snapshot.
Fix things by uniformly using virDomainSnapshotDefIsExternal()
(caching the results up front, and eliminating other 'if' clauses
now rendered redundant) when deciding whether the XML being
requested for redefinition should permit external or force internal
state capture (we got it right in only one out of three places in
the function).
See also https://bugzilla.redhat.com/1680304; this fixes the
domain-agnostic problems mentioned there, but another patch is
needed to fix further oddities with the qemu driver. I did not
check for sure when the problems were introduced (git blame puts
some affected hunks as far back as 1.0.0), but it was definitely
been broken even before when commit 670e86bf (1.1.4) factored
redefine prep out of qemu code into the common snapshot_conf code.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Mon, 11 Jun 2018 13:59:10 +0000 (08:59 -0500)]
snapshots: Avoid term 'checkpoint' for full system snapshot
Upcoming patches plan to introduce virDomainCheckpointPtr as a new
object for use in incremental backups, along with documentation on
how incremental backups differ from snapshots. But first, we need
to rename any existing mention of a 'system checkpoint' to instead
be a 'full system snapshot', so that we aren't overloading
the term checkpoint.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Tue, 26 Feb 2019 18:49:25 +0000 (12:49 -0600)]
virsh: Elide backslash-newline in batch mode
The previous patch made it possible to split multiple commands by
adding newline, but not to split a long single command. The sequence
backslash-newline was being used as if it were a quoted newline
character, rather than completely elided the way the shell does.
Again, add more tests, although this time it seems more like I am
suffering from a leaning-toothpick syndrome with all the \.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Let's be more like the shell, and treat unquoted newline as a
command separator just as we do for semicolon. In fact, with
that, I can even now mix styles:
$ virsh -c test:///default '
echo a; echo b
echo c
'
a
b
c
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Julio Faracco [Mon, 18 Feb 2019 19:09:10 +0000 (16:09 -0300)]
lxc: Converting 'if, else' logic into a 'switch, case'
The structure used to handle network entries was based on 'if,else'
conditions. This commit converts this ugly structure into a switch to
clearify each option of the handler.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Julio Faracco [Mon, 18 Feb 2019 19:09:07 +0000 (16:09 -0300)]
lxc: Introduce lxcNetworkParseDataSuffix
This commit removes the full network entry setting: "lxc.network.X" to
type only. Like "type", "name", "flags", etc. This will handle entries
regardless of whether they are prefixed by "lxc.network." (today) or
"lxc.net.X." (the future).
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Julio Faracco [Mon, 18 Feb 2019 19:09:06 +0000 (16:09 -0300)]
lxc: Introduce lxcNetworkParseDataEntry
Refactor lxcNetworkWalkCallback to be a simple method to handle
both possible network settings with indexes or the simple one. It is
better the decouple the whole algorithm to parse data to only parse
which entry type libvirt is handling.
The new method is responsible to verify is the settings correspond to
network entry. Right now, it is only verifying "lxc.network.", but in
the future, it can be used to verify "lxc.net.X." too. Any other case
would be rejected.
On the other hand, the idea here is working only with types. If we know
that entry is part of network settings, after we just need to know which
type is. It keeps the handler simple.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Julio Faracco [Mon, 18 Feb 2019 19:09:05 +0000 (16:09 -0300)]
lxc: Create a separate method to handle IPv{4, 6} outside parser
The new method called lxcNetworkParseDataIPs() is responsible to handle
IPv{4,6} settings now. The idea is let lxcNetworkWalkCallback() method
handle all entries related to network definition only.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Andrea Bolognani [Tue, 19 Feb 2019 16:20:41 +0000 (17:20 +0100)]
util: Report error in virFileWrapperFdClose()
libvirt_iohelper is used internally by the virFileWrapperFd APIs;
more specifically, in the QEMU driver we have the doCoreDump() and
qemuDomainSaveMemory() helper functions as users, and those in turn
end up being called by the implementation of several driver APIs.
By calling virReportError() if libvirt_iohelper has failed, we
overwrite whatever generic error message QEMU might have raised
with the more useful one generated by the helper program.
After this commit, the user will be able to see the error directly
instead of having to dig in the journal or libvirtd log.
Andrea Bolognani [Wed, 20 Feb 2019 13:16:46 +0000 (14:16 +0100)]
util: Move error reporting back to virFileWrapperFdClose()
virFileWrapperFdFree(), like all free functions, is supposed
to only release allocated resources, so error reporting is
better suited for virFileWrapperFdClose().
Andrea Bolognani [Tue, 19 Feb 2019 16:18:07 +0000 (17:18 +0100)]
qemu: Always call virFileWrapperFdClose()
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
In a couple of cases we can just move the call, but for the
remaining ones we need to duplicate it instead in order not
to alter the existing behavior. We do, however, make sure
that in all cases a failure to properly close the wrapper
results in the overall operation being reported as failed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Andrea Bolognani [Tue, 19 Feb 2019 15:58:27 +0000 (16:58 +0100)]
util: Make it safe to call virFileWrapperFdClose() multiple times
We'll want to use this function in the cleanup path soon,
and in order to be able to do that we need to make sure we
can call it multiple times on the same virFileWrapperFd
without side effects.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Marc Hartmayer [Wed, 20 Feb 2019 08:51:07 +0000 (09:51 +0100)]
qemu: Replace virDomainChrSourceDefFree with virObjectUnref
Replace virDomainChrSourceDefFree with virObjectUnref.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com>
Marc Hartmayer [Wed, 20 Feb 2019 08:51:06 +0000 (09:51 +0100)]
qemu: Use refcounting for priv->monConfig
Use refcounting for priv->monConfig instead of asymmetric freeing.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com>
security: aa-helper: generate more rules for gl devices
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But further testing showed
that it will need much more access for the full gl stack
to work.
Upstream apparmor just recently split those things out and now
has two related abstractions at
https://gitlab.com/apparmor/apparmor/blob/master:
- dri-common at /profiles/apparmor.d/abstractions/dri-common
- mesa: at /profiles/apparmor.d/abstractions/mesa
If would be great to just include that for the majority of
rules, but they are not yet in any distribution so we need
to add rules inspired by them based on the testing that we
can do.
Furthermore qemu with opengl will also probe the backing device
of the rendernode for attributes which should be safe as
read-only wildcard rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452 Acked-by: Jamie Strandboge <jamie@canonical.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
security: aa-helper: allow virt-aa-helper to read /dev/dri
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But it will in certain cases e.g. if
no rendernode was explicitly specified need to read /dev/dri
which it currently isn't allowed.
Add a rule to the apparmor profile of virt-aa-helper itself to
be able to do that.
Acked-by: Jamie Strandboge <jamie@canonical.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Introduce the 'msrs' feature element that controls Model Specific
Registers related behaviour. At this moment it allows only
single tunable attribute "unknown":
<msrs unknown='ignore|fault'/>
Which tells hypervisor to ignore accesses to unimplemented
Model Specific Registers. The only user of that for now is going
to be the bhyve driver.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Reviewed-by: Cole Robinson <crobinso@redhat.com>
Peter Krempa [Thu, 21 Feb 2019 14:49:29 +0000 (15:49 +0100)]
util: alloc: Introduce 'VIR_AUTOCLEAN' macros for clearing stack'd structs
The new utility macros are useful for variables we put on the stack but
require some cleanup. The most prominent of those is virBuffer which is
used almost exclusively in that way.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
Peter Krempa [Thu, 21 Feb 2019 15:07:34 +0000 (16:07 +0100)]
util: buf: Fix memory leak in virBufferEscapeN
The conversion to VIR_AUTOFREE of 'escapeList' introduced memory leak of
the copied item to be escaped:
==17517== 2 bytes in 1 blocks are definitely lost in loss record 1 of 32
==17517== at 0x483880B: malloc (vg_replace_malloc.c:309)
==17517== by 0x54D666D: strdup (in /usr/lib64/libc-2.28.so)
==17517== by 0x497663E: virStrdup (virstring.c:956)
==17517== by 0x497663E: virStrdup (virstring.c:945)
==17517== by 0x48F8853: virBufferEscapeN (virbuffer.c:707)
==17517== by 0x403C9D: testBufEscapeN (virbuftest.c:383)
==17517== by 0x405FA8: virTestRun (testutils.c:174)
==17517== by 0x403A70: mymain (virbuftest.c:517)
==17517== by 0x406BC9: virTestMain (testutils.c:1097)
==17517== by 0x5470412: (below main) (in /usr/lib64/libc-2.28.so)
[...] (all other have same backtrace as it happens in a loop)
Fix it by reverting all the VIR_AUTO nonsense in this function as there
is exactly one place where it's handled.
Laine Stump [Mon, 18 Feb 2019 21:34:50 +0000 (16:34 -0500)]
network: add netmask to dhcp range of dnsmasq conf file for IPv4
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
A user had reported a bug on a host where one of the interfaces was a
superset of the libvirt network where dhcp is needed (e.g., the host's
ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For
some reason dnsmasq was supplying the netmask for the /8 network to
clients requesting an address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org> Reviewed-by: John Ferlan <jferlan@redhat.com>
Laine Stump [Mon, 18 Feb 2019 21:33:57 +0000 (16:33 -0500)]
util: set missing data length in virSocketAddrPrefixToNetmask()
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask with virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump <laine@laine.org> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Thu, 21 Feb 2019 15:43:49 +0000 (09:43 -0600)]
snapshot: Saner use of uuid
Most of the code base is fairly consistent about using the name
'uuidstr' when dealing with a formatted human-readable form, and
'uuid' when dealing with the smaller raw bytes form. Fix
snapshot_conf to comply, as well as reducing the scope of a human
string to only the error message that needs it.
Michal Privoznik [Tue, 19 Feb 2019 17:15:23 +0000 (18:15 +0100)]
testutils: Explicitly name virTestCompare*() arguments
Currently, some arguments are called strcontent and strsrc, or
content and src or some other combination. This makes it
impossible to see at the first glance what argument is supposed
to represent 'expected' value and which one represents 'actual'
value. Rename the arguments to make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Andrea Bolognani [Tue, 19 Feb 2019 15:03:54 +0000 (16:03 +0100)]
iohelper: Remove remaining newlines from error messages
The iohelper is an internal program that's only supposed to
be called by libvirt, and whatever output it might produce
will ultimately be passed to virReportError() or similar.
Since we do not want strings passed to those functions to
contain newlines, we can simply not output them in the first
place.
This is what happens in pretty much all cases already, but
in a couple instances newlines have managed to slip in.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Laine Stump <laine@laine.org>
Eric Blake [Wed, 13 Feb 2019 21:09:05 +0000 (15:09 -0600)]
snapshot: Define explicit flags for snapshot xml
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit 28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the snapshot documentation to declare it as invalid.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; esx and vbox don't support flags;
qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during a snapshot creation
needs to be migration-friendly (as the snapshot is not the source of
a migration), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from domain API, and risking confusion if even more domain
flags are added later (in fact, I have an upcoming patch that plans to
add a new flag to virDomainGetXMLDesc that makes no sense for
snapshots).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Wed, 13 Feb 2019 21:09:05 +0000 (15:09 -0600)]
domain: Define explicit flags for saved image xml
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit 28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the save image documentation to declare it as invalid.
Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text
into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; and qemu is the only supporting
driver for either API, with support for just VIR_DOMAIN_XML_SECURE),
it is easier to just define an explicit set of supported flags
directly related to the save image API rather than trying to borrow
from live domain API, and risking confusion if even more domain flags
are added later (in fact, I have an upcoming patch that plans to add
a new flag to virDomainGetXMLDesc that makes no sense for saved
images). We may someday decide that saved images need to support the
_MIGRATABLE flag, as it is possible to load a saved image with a
different version of libvirt than the one that created it, but that
can be a separate patch if it is ever needed. Meanwhile, it DOES make
sense to reuse the same flags for SaveImage and for ManagedSave (since
ManagedSave is really just sugar for creating a normal SaveImage in a
location controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the old reused name).
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Thu, 14 Feb 2019 18:53:36 +0000 (12:53 -0600)]
qemu: Use correct domain xml flag
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE
happen to have the same value (1<<1), they come from different enums;
and it is nicer to reason about a 'flags' variable if all uses of
that variable are compared against the same enum type. Messed up in
commit 06f75ff2 (3.8.0).
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Thu, 14 Feb 2019 20:25:01 +0000 (14:25 -0600)]
domain: Fix unknown flags diagnosis in virDomainGetXMLDesc
Many drivers had a comment that they did not validate the incoming
'flags' to virDomainGetXMLDesc() because they were relying on
virDomainDefFormat() to do it instead. This used to be the case
(at least since 461e0f1a and friends in 0.9.4 added unknown flag
checking in general), but regressed in commit 0ecd6851 (1.2.12),
when all of the drivers were changed to pass 'flags' through the
new helper virDomainDefFormatConvertXMLFlags(). Since this helper
silently ignores unknown flags, we need to implement flag checking
in each driver instead.
Annoyingly, this means that any new flag values added will silently
be ignored when targeting an older libvirt, rather than our usual
practice of loudly diagnosing an unsupported flag. Add comments
in domain_conf.[ch] to remind us to be extra vigilant about the
impact when adding flags (a new flag to add data is safe if the
older server omitting the requested data doesn't break things in
the newer client; a new flag to suppress data rather than enhancing
the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a
security hole).
In the qemu driver, there are multiple callers all funnelling to
qemuDomainDefFormatBufInternal(); many of them already validated
flags (and often only a subset of the full set of possible flags),
but for ease of maintenance, we can also check flags at the common
helper function.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Chris Venteicher [Thu, 14 Feb 2019 10:25:50 +0000 (11:25 +0100)]
qemu_process: Enter QMP command mode when starting QEMU Process
qemuProcessQMPStart starts a QEMU process and monitor connection that
can be used by multiple functions possibly for multiple QMP commands.
The QMP exchange to exit capabilities negotiation mode and enter command
mode can only be performed once after the monitor connection is
established.
Move responsibility for entering QMP command mode into the
qemuProcessQMP code so multiple functions can issue QMP commands in
arbitrary orders.
This also simplifies the functions using the connection provided by
qemuProcessQMPStart to issue QMP commands.
Test code now needs to call qemuMonitorSetCapabilities to send the
message to switch to command mode because the test code does not use the
qemuProcessQMP command that internally calls qemuMonitorSetCapabilities.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Wed, 13 Feb 2019 16:22:31 +0000 (17:22 +0100)]
qemu_process: Use unique directories for QMP processes
Multiple QEMU processes for QMP commands can operate concurrently.
Use a unique directory under libDir for each QEMU process to avoid
pidfile and unix socket collision between processes.
The pid file name is changed from "capabilities.pidfile" to "qmp.pid"
because we no longer need to avoid a possible clash with a qemu domain
called "capabilities" now that the processes artifacts are stored in
their own unique temporary directories.
"Capabilities" was changed to "qmp" in the pid file name because these
processes are no longer specific to the capabilities usecase and are
more generic in terms of being used for any general purpose QMP message
exchanges with a QEMU process that is not associated with a domain.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jiri Denemark [Wed, 13 Feb 2019 16:18:51 +0000 (17:18 +0100)]
qemu_process: Hide qemuProcessQMPStop
Users qemuProcessQMP struct were always forced to call both
qemuProcessQMPStop and qemuProcessQMPFree when they are done with the
process. We can just call qemuProcessQMPStop from qemuProcessQMPFree and
let users call qemuProcessQMPFree only.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Sun, 13 Jan 2019 00:50:14 +0000 (18:50 -0600)]
qemu_process: Stop retaining monitor config in qemuProcessQMP
The monitor config data is removed from the qemuProcessQMP struct.
The monitor config data can be initialized immediately before call to
qemuMonitorOpen and does not need to be maintained after the call
because qemuMonitorOpen copies any strings it needs.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Sun, 13 Jan 2019 00:50:10 +0000 (18:50 -0600)]
qemu_process: Introduce qemuProcessQMPStart
This is a replacement for qemuProcessQMPRun to make the name consistent
with qemuProcessStart. The original qemuProcessQMPRun function is
renamed as qemuProcessQMPLaunch and becomes one of the simpler functions
called from the main qemuProcessQMPStart entry point. The following
patches will move parts of the code in qemuProcessQMPLaunch to the other
functions (qemuProcessQMPInit and qemuProcessQMPConnectMonitor).
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jiri Denemark [Tue, 12 Feb 2019 13:38:40 +0000 (14:38 +0100)]
qemu_process: Don't ignore errors in virQEMUCapsInit
While qemuProcessQMPRun and virQEMUCapsInitQMPMonitor* functions called
from virQEMUCapsInit ignore some errors, the caller of virQEMUCapsInit
would report an error unless usedQMP is true anyway. And since usedQMP
can only be true if the probing code really succeeded (i.e., no errors
were ignored), we can just simplify the logic by not ignoring the errors
in the first place.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Sun, 13 Jan 2019 00:50:06 +0000 (18:50 -0600)]
qemu_process: Use qemuProcessQMP struct for a single process
In new process code, move from model where qemuProcessQMP struct can be
used to activate a series of Qemu processes to model where one
qemuProcessQMP struct is used for one and only one Qemu process.
By allowing only one process activation per qemuProcessQMP struct, the
struct can safely store process outputs like status and stderr, without
being overwritten, until qemuProcessQMPFree is called.
By doing this, process outputs like status and stderr can remain stored
in the qemuProcessQMP struct without being overwritten by subsequent
process activations.
The forceTCG parameter (use / don't use KVM) will be passed when the
qemuProcessQMP struct is initialized since the qemuProcessQMP struct
won't be reused.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Sun, 13 Jan 2019 00:50:05 +0000 (18:50 -0600)]
qemu_capabilities: Stop QEMU process before freeing
virQEMUCapsInitQMP now stops QEMU process in all execution paths,
before freeing the process structure.
The qemuProcessQMPStop function can be called multiple times without
problems... Won't attempt to stop processes and free resources multiple
times.
Follow the convention established in qemu_process of
1) alloc process structure
2) start process
3) use process
4) stop process
5) free process data structure
The process data structure persists after the process activation fails
or the process dies or is killed so stderr strings can be retrieved
until the process data structure is freed.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Chris Venteicher [Sun, 13 Jan 2019 00:50:02 +0000 (18:50 -0600)]
qemu_process: Limit qemuProcessQMPNew to const input strings
Add the const qualifier on non modified strings
(string only copied inside qemuProcessQMPNew)
so that const strings can be used directly in calls to
qemuProcessQMPNew in future patches.
Signed-off-by: Chris Venteicher <cventeic@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
iohelper: Don't include newlines in error messages
The newline was pretty arbitrary, and we're better off
without it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
The memory allocated by VIR_REALLOC_N() is uninitialized,
which means it's not possible to figure out whether any
output was produced at all after the fact.
Since we don't care about the previous contents of buffers,
if any, use VIR_FREE() followed by VIR_ALLOC_N() instead.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
projects / thirdparty / libvirt.git / log
