We need to drop the server lock before calling virObjectUnlock(client)
since in case we had the last reference to the client, its dispose
callback would be called and that could possibly try to lock the server
and cause a deadlock. This is exactly what happens when there is only
one QEMU domain running and it is marked to be autodestroyed when the
connection dies. This results in qemuProcessAutoDestroy ->
qemuProcessStop -> virNetServerRemoveShutdownInhibition call sequence,
where the last function locks the server.
The function does not report any errors so there should be no need too
reset an existing error first. Moreover, virTypedParamsFree is mostly
called in cleanup phase where it has the potential to reset any useful
reported earlier.
Gcc lets you do:
int ATTRIBUTE_NONNULL(1) foo(void *param);
int foo(void *param) ATTRIBUTE_NONNULL(1);
int ATTRIBUTE_NONNULL(1) foo(void *param) { ... }
but chokes on:
int foo(void *param) ATTRIBUTE_NONNULL(1) { ... }
However, since commit eefb881, we have intentionally been disabling
ATTRIBUTE_NONNULL because of lame gcc handling of the attribute (that
is, gcc doesn't do decent warning reporting, then compiles code that
mysteriously fails if you break the contract of the attribute, which
is surprisingly easy to do), leaving it on only for Coverity (which
does a much better job of improved static analysis when the attribute
is present).
But completely eliding the macro makes it too easy to write code that
uses the fourth syntax option, if you aren't using Coverity. So this
patch forces us to avoid syntax errors, even when not using the
attribute under gcc. It also documents WHY we disable the warning
under gcc, rather than forcing you to find the commit log.
* src/internal.h (ATTRIBUTE_NONNULL): Expand to empty attribute,
rather than nothing, when on gcc.
The conversion to qemuCaps dropped the ability with qemu{,-kvm} 1.2 and
newer to set the lost tick policy for the PIT. While the
-no-kvm-pit-reinjection option is depreacated, it is still supported at
least through 1.4, it is better to not lose the functionality.
Coverity found the DACGenLabel was checking for mgr == NULL after a
possible dereference; however, in order to get into the function the
virSecurityManagerGenLabel would have already dereferenced sec_managers[i]
so the check was unnecessary. Same check is made in SELinuxGenSecurityLabel.
Between revision 65fb9d49 and before this patch, an upgrade of libvirt while
VMs are running and instantiating iptables filtering rules due to nwfilter
rules, may leave stray iptables rules behind when shutting VMs down.
Left-over iptables rules may look like this:
Chain FP-vnet0 (1 references)
target prot opt source destination
DROP tcp -- 0.0.0.0/0 0.0.0.0/0 tcp spt:122
ACCEPT all -- 0.0.0.0/0 0.0.0.0/0
[...]
Chain libvirt-out (1 references)
target prot opt source destination
FO-vnet0 all -- 0.0.0.0/0 0.0.0.0/0 [goto] PHYSDEV match --physdev-out vnet0
The reason is that the recent nwfilter code only removed filtering rules in
the libvirt-out chain that contain the --physdev-is-bridged parameter.
Older rules didn't match and were not removed.
Note that the user-defined chain FO-vnet0 could not be removed due to the
reference from the rule in libvirt-out.
Often the work around may be done through
service iptables restart
kill -SIGHUP $(pidof libvirtd)
This patch now also removes older libvirt versions' iptables rules.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Testing our backing chain handling will make it much easier to
ensure that we avoid issues in the future. If only I had written
this test before I first caused several regressions...
* tests/virstoragetest.c: New test.
* tests/Makefile.am (test_programs): Build it.
* .gitignore: Ignore new files.
If you have a qcow2 file /path1/to/file pointed to by symlink
/path2/symlink, and pass qemu /path2/symlink, then qemu treats
a relative backing file in the qcow2 metadata as being relative
to /path2, not /path1/to. Yes, this means that it is possible
to create a qcow2 file where the choice of WHICH directory and
symlink you access its contents from will then determine WHICH
backing file (if any) you actually find; the results can be
rather screwy, but we have to match what qemu does.
Libvirt and qemu default to creating absolute backing file
names, so most users don't hit this. But at least VDSM uses
symlinks and relative backing names alongside the
--reuse-external flags to libvirt snapshot operations, with the
result that libvirt was failing to follow the intended chain of
backing files, and then backing files were not granted the
necessary sVirt permissions to be opened by qemu.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for
more gory details. This fixes a regression introduced in
commit 8250783.
I tested this patch by creating the following chain:
ls /home/eblake/Downloads/Fedora.iso # raw file for base
cd /var/lib/libvirt/images
qemu-img create -f qcow2 \
-obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one
mkdir sub
cd sub
ln -s ../one onelink
qemu-img create -f qcow2 \
-obacking_file=../sub/onelink,backing_fmt=qcow2 two
mv two ..
ln -s ../two twolink
qemu-img create -f qcow2 \
-obacking_file=../sub/twolink,backing_fmt=qcow2 three
mv three ..
ln -s ../three threelink
then pointing my domain at /var/lib/libvirt/images/sub/threelink.
Prior to this patch, I got complaints about missing backing
files; afterwards, I was able to verify that the backing chain
(and hence DAC and SELinux relabels) of the entire chain worked.
* src/util/virstoragefile.h (_virStorageFileMetadata): Add
directory member.
* src/util/virstoragefile.c (absolutePathFromBaseFile): Drop,
replaced by...
(virFindBackingFile): ...better function.
(virStorageFileGetMetadataInternal): Add an argument.
(virStorageFileGetMetadataFromFD, virStorageFileChainLookup)
(virStorageFileGetMetadata): Update callers.
Prior to this patch, we had the callchains:
external users
\-> virStorageFileGetMetadataFromFD
\-> virStorageFileGetMetadataFromBuf
virStorageFileGetMetadataRecurse
\-> virStorageFileGetMetadataFromFD
\-> virStorageFileGetMetadataFromBuf
However, a future patch wants to add an additional parameter to
the bottom of the chain, for use by virStorageFileGetMetadataRecurse,
without affecting existing external callers. Since there is only a
single caller of the internal function, we can repurpose it to fit
our needs, with this patch giving us:
external users
\-> virStorageFileGetMetadataFromFD
\-> virStorageFileGetMetadataInternal
virStorageFileGetMetadataRecurse /
\-> virStorageFileGetMetadataInternal
* src/util/virstoragefile.c (virStorageFileGetMetadataFromFD):
Move most of the guts...
(virStorageFileGetMetadataFromBuf): ...here, and rename...
(virStorageFileGetMetadataInternal): ...to this.
(virStorageFileGetMetadataRecurse): Use internal helper.
virStorageFileGetMetadataFromFD is the only caller of
virStorageFileGetMetadataFromBuf; and it doesn't care about the
difference between a return of 0 (total success) or 1
(metadata was inconsistent, but pointer was populated as best
as possible); only about a return of -1 (could not read metadata
or out of memory). Changing the return type, and normalizing
the variable names used, will make merging the functions easier
in the next commit.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf):
Change return value, and rename some variables.
(virStorageFileGetMetadataFromFD): Rename some variables.
No semantic change; done so the next patch doesn't need a forward
declaration of a static function.
* src/util/virstoragefile.c (virStorageFileProbeFormatFromBuf):
Hoist earlier.
More mingw build failures:
CCLD libvirt-lxc.la
/usr/lib64/gcc/i686-w64-mingw32/4.7.2/../../../../i686-w64-mingw32/bin/ld: cannot find libvirt_lxc.def: No such file or directory
CC virportallocatortest-virportallocatortest.o
../../tests/virportallocatortest.c: In function 'main':
../../tests/virportallocatortest.c:195:1: error: implicit declaration of function 'setenv' [-Werror=implicit-function-declaration]
* src/Makefile.am (GENERATED_SYM_FILES): Also generate
libvirt_lxc.def.
* bootstrap.conf (gnulib_modules): Import setenv.
CC libvirt_util_la-vircommand.lo
../../src/util/vircommand.c:2358:1: error: 'virCommandHandshakeChild' defined but not used [-Werror=unused-function]
The function is only implemented inside #ifndef WIN32.
* src/util/vircommand.c (virCommandHandshakeChild): Hoist earlier,
so that win32 build doesn't hit an unused forward declaration.
No need to use HAVE_REGEX_H - our use of gnulib guarantees that
the header exists and works, regardless of platform. Similarly,
we can unconditionally assume a compiling <sys/wait.h> (although
the mingw version of this header is not full-featured).
* src/storage/storage_backend.c: Drop useless conditional.
* tests/testutils.c: Likewise.
virCommand was previously calling virSetUIDGID() to change the uid and
gid of the child process, then separately calling
virSetCapabilities(). This did not work if the desired uid was != 0,
since a setuid to anything other than 0 normally clears all
capabilities bits.
The solution is to use the new virSetUIDGIDWithCaps(), sending it the
uid, gid, and capabilities bits. This will get the new process setup
properly.
Since the static functions virSetCapabilities() and
virClearCapabilities are no longer called, they have been removed.
NOTE: When combined with "filecap $path-to-qemu sys_rawio", this patch
will make CAP_SYS_RAWIO (which is required for passthrough of generic
scsi commands to a guest - see commits e8daeeb, 177db08, 397e6a7, and
74e0349) be retained by qemu when necessary. Apparently that
capability has been broken for non-root qemu ever since it was
originally added.
Normally when a process' uid is changed to non-0, all the capabilities
bits are cleared, even those explicitly set with calls to
capng_update()/capng_apply() made immediately before setuid. And
*after* the process' uid has been changed, it no longer has the
necessary privileges to add capabilities back to the process.
In order to set a non-0 uid while still maintaining any capabilities
bits, it is necessary to either call capng_change_id() (which
unfortunately doesn't currently call initgroups to setup auxiliary
group membership), or to perform the small amount of calisthenics
contained in the new utility function virSetUIDGIDWithCaps().
Another very important difference between the capabilities
setting/clearing in virSetUIDGIDWithCaps() and virCommand's
virSetCapabilities() (which it will replace in the next patch) is that
the new function properly clears the capabilities bounding set, so it
will not be possible for a child process to set any new
capabilities.
A short description of what is done by virSetUIDGIDWithCaps():
1) clear all capabilities then set all those desired by the caller (in
capBits) plus CAP_SETGID, CAP_SETUID, and CAP_SETPCAP (which is needed
to change the capabilities bounding set).
2) call prctl(), telling it that we want to maintain current
capabilities across an upcoming setuid().
3) switch to the new uid/gid
4) again call prctl(), telling it we will no longer want capabilities
maintained if this process does another setuid().
5) clear the capabilities that we added to allow us to
setuid/setgid/change the bounding set (unless they were also requested
by the caller via the virCommand API).
Because the modification/maintaining of capabilities is intermingled
with setting the uid, this is necessarily done in a single function,
rather than having two independent functions.
Note that, due to the way that effective capabilities are computed (at
time of execve) for a process that has uid != 0, the *file*
capabilities of the binary being executed must also have the desired
capabilities bit(s) set (see "man 7 capabilities"). This can be done
with the "filecap" command. (e.g. "filecap /usr/bin/qemu-kvm sys_rawio").
This is an interim measure to make sure everything still works in this
order. The next step will be to perform capabilities drop and
setuid/gid as a single operation (which is the only way to keep any
capabilities when switching to a non-root uid).
The qemu driver had been calling virSecurityManagerSetProcessLabel()
from a "pre-exec hook" function that is run after the child is forked,
but before exec'ing qemu. This is problematic because the uid and gid
of the child are set by the security driver, but capabilities are
dropped by virCommand - such separation doesn't work; the two
operations must be done together or the capabilities do not transfer
properly to the child process.
This patch switches to using virSecurityManagerSetChildProcessLabel(),
which is called prior to virCommandRun() (rather than being called
*during* virCommandrun() by the hook function), and doesn't set the
UID/GID/security label directly, but instead merely informs virCommand
what it should set them all to when the time is appropriate.
This lets virCommand choose to do the uid/gid and caps dropping all at
the same time if it wants (it does *want* to, but isn't doing so yet;
that's for an upcoming patch).
The existing virSecurityManagerSetProcessLabel() API is designed so
that it must be called after forking the child process, but before
exec'ing the child. Due to the way the virCommand API works, that
means it needs to be put in a "hook" function that virCommand is told
to call out to at that time.
Setting the child process label is a basic enough need when executing
any process that virCommand should have a method of doing that. But
virCommand must be told what label to set, and only the security
driver knows the answer to that question.
The new virSecurityManagerSet*Child*ProcessLabel() API is the way to
transfer the knowledge about what label to set from the security
driver to the virCommand object. It is given a virCommandPtr, and each
security driver calls the appropriate virCommand* API to tell
virCommand what to do between fork and exec.
1) in the case of the DAC security driver, it calls
virCommandSetUID/GID() to set a uid and gid that must be set for the
child process.
2) for the SELinux security driver, it calls
virCommandSetSELinuxLabel() to save a copy of the char* that will be
sent to setexeccon_raw() *after forking the child process*.
3) for the AppArmor security drivers, it calls
virCommandSetAppArmorProfile() to save a copy of the char* that will
be sent to aa_change_profile() *after forking the child process*.
With this new API in place, we will be able to remove
virSecurityManagerSetProcessLabel() from any virCommand pre-exec
hooks.
(Unfortunately, the LXC driver uses clone() rather than virCommand, so
it can't take advantage of this new security driver API, meaning that
we need to keep around the older virSecurityManagerSetProcessLabel(),
at least for now.)
virCommand gets two new APIs: virCommandSetSELinuxLabel() and
virCommandSetAppArmorProfile(), which both save a copy of a
null-terminated string in the virCommand. During virCommandRun, if the
string is non-NULL and we've been compiled with AppArmor and/or
SELinux security driver support, the appropriate security library
function is called for the child process, using the string that was
previously set. In the case of SELinux, setexeccon_raw() is called,
and for AppArmor, aa_change_profile() is called.
This functionality has been added so that users of virCommand can use
the upcoming virSecurityManagerSetChildProcessLabel() prior to running
a child process, rather than needing to setup a hook function to be
called (and in turn call virSecurityManagerSetProcessLabel()) *during*
the setup of the child process.
This makes it simpler to include the necessary system security driver
libraries for a particular system. For this patch, several existing
conditional sections from the Makfile were replaced; I'll later be
adding SECDRIVER_LIBS to libvirt_util_la_LIBADD, because vircommand.c
will be calling a function from $securitylib.
Setting the uid/gid of the child process was the only thing done by
the hook function in this case, and that can now be done more simply
with virCommandSetUID/GID.
Rather than treating uid:gid of 0:0 as a NOP, we blindly pass that
through to the lower layers. However, we *do* check for a requested
value of "-1" to mean "don't change this setting". setregid() and
setreuid() already interpret -1 as a NOP, so this is just an
optimization, but we are also calling getpwuid_r and initgroups, and
it's unclear what the former would do with a uid of -1.
If a uid and/or gid is specified for a command, it will be set just
after the user-supplied post-fork "hook" function is called.
The intent is that this can replace user hook functions that set
uid/gid. This moves the setting of uid/gid and dropping of
capabilities closer to each other, which is important since the two
should really be done at the same time (libcapng provides a single
function that does both, which we will be unable to use, but want to
mimic as closely as possible).
All args except "cmd" in the call to virExec are now redundant, since
they can all be found in cmd, so remove the args and reference the
data directly in cmd. One exception to this is that "infd" was being
modified within virExec, and modifying the original in cmd caused make
check failures, so cmd->infd is copied to a local, and the local is
used during virExec().
virExecWithHook is only called from one place, so it always has the
same "hook" function (virHookCommand), and the data sent to that
function is always a virCommandPtr, so eliminate the function and
generic data from the arglist, and replace it with "virCommandPtr
cmd". The call to (hook)(data) is replaced with
"virHookCommand(cmd)". Finally, virExecWithHook is renamed to virExec.
Indentation has been updated only for code that will remain after the
next patch, which will remove all other args to virExec (since they
are now redundant, as they're all members of virCommandPtr).
With the majority of fields in the virQEMUDriverPtr struct
now immutable or self-locking, there is no need for practically
any methods to be using the QEMU driver lock. Only a handful
of helper APIs in qemu_conf.c now need it
Currently, if a command wants to do asynchronous IO, a callback
is registered in the libvirtd eventloop to handle writes and
reads. However, there's a race in virCommandWait. The eventloop
may already be executing the callback, while virCommandWait is
mangling internal state of virCommand. To deal with it, we need
to either introduce locking or spawn a separate thread where we
poll() on stdio from child. The former, however, requires to
unlock all mutexes held, as the event loop may execute other
callbacks which tries to lock one of the mutexes, deadlock and
thus never wake us up. So it's safer to spawn a separate thread.
