Simplify GenerateName/ReserveName for netdevtap by using common
functions.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Extract ReserveName/GenerateName from netdevtap and netdevmacvlan as
common helper functions.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Until now there has been an extra bit of code in
qemuDomainDeviceCalculatePCIConnectFlag() (one of the two callers of
virPCIDeviceIsPCIExpress()) that tries to determine if a device is
PCIe by looking at the *length* of its sysfs config file; it only does
this when libvirt is running as a non-root process.
This patch takes advantage of our newfound ability to tell the
difference between "I read a 0 from the device PCI config file" and "I
couldn't read the PCI Express Capabilities because I don't have
sufficient permission" to put the file length check down in
virPCIDeviceIsPCIExpress(), and do that check any time we fail while
reading the config file (not only when the process is non-root).
Fixes: https://bugzilla.redhat.com/1901685
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Previously there was no way to differentiate between this function 1)
encountering an error while reading the pci config, and 2) determining
that the device in question is a conventional PCI device, and so has
no Express Capabilities.
The difference between these two conditions is important, because an
unprivileged libvirtd will be unable to read all of the pci config (it
can only read the first 64 bytes, and will get ENOENT when it tries to
seek past that limit) even though the device is in fact a PCIe device.
This patch changes virPCIDeviceFindCapabilityOffset() to put the
determined offset into an argument of the function (rather than
sending it back as the return value), and to return the standard "0 on
success, -1 on failure". Failure is determined by checking the value
of errno after each attemptd read of the config file (which can only
work reliably if errno is reset to 0 before each read, and after
virPCIDeviceFindCapabilityOffset() has finished examining it).
(NB: if the config file is read successfully, but no Express
Capabilities are found, then the function returns success, but the
returned offset will be 0 (which is an impossible offset for Express
Capabilities, and so easily recognizeable).
An upcoming patch will take advantage of the change made here.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The new message is more verbose/useful, but only logged at debug level
instead of as a warning (since it could easily happen in a non-error
situation).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function returned an int, but would only return 0 or 1, and the
one place it was called would just use !! to convert that value to a
bool. Change the function to directly return bool instead.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function returned an int, and that int was being checked for < 0
in its solitary caller, but within the function it would only ever
return 0 or 1. Change the function itself to return a bool, and the
caller to just directly set the flag in the virPCIDevice.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We previous added code for passing FDs which was explicitly derived from
gnulib's passfd code:
commit 17460825f3c78e1635f2beb0165c5a19e3b09f7d
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Fri Jan 17 11:57:17 2020 +0000
src: implement APIs for passing FDs over UNIX sockets
This is a simplified variant of gnulib's passfd module
without the portability code that we do not require.
while the license was unchanged, we mistakenly failed to copy the FSF
copyright header which is required by the license terms.
Reported-by: Bruno Haible <bruno@clisp.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This macro checks whether given number is an integer power of
two. At the same time, I've identified two places where we check
for pow2 and I'm replacing them with the macro.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
The only test we do when checking for UUID validity is that
whether all bytes are the same (invalid UUID) or not (valid
UUID). The algorithm we use is needlessly complicated.
Also, the checked UUID is not modified and hence the argument can
be of 'const' type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Han Han <hhan@redhat.com>
In this previous commit:
commit 65491a2dfe00bfcf9f09a8d6eab60234b56c8cc4
Author: Martin Kletzander <mkletzan@redhat.com>
Date: Thu Nov 12 13:58:53 2020 +0100
Do not disable incompatible-pointer-types-discards-qualifiers
We selectively rewrite G_DEFINE_TYPE to avoid warnings about
mismatched volatile/non-volatile pointers that appeared with
CLang when using GLib2 >= 2.67
We have now just hit the reverse problem, GCC >= 11 has started
warning about mismatched volatile/non-volatile pointers but only
with GLib2 < 2.67. The new GLib2 avoids the warning, as does
older GCC.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since 032548c4 @cmd was never autofree'd. Perhaps as a result of
VIR_AUTOPTR type changes occurring at roughly the same time so the
copy pasta missed this.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Now that no one uses VIR_AUTOSTRINGLIST it can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Glib provides g_auto(GStrv) which is in-place replacement of our
VIR_AUTOSTRINGLIST.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This will open an opportunity to modernize virDomainDiskDefParseXML()
in the next patch.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Just like virCommandPassFD, but it also returns an index of
the passed FD in the FD set.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The virJSONValueObjectGetStringArray() function is given a @key
which is supposed to be an array inside given @object. Well, if
it's not then an error state is returned (NULL), but no error
message is set.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The original logic is incorrect. We would delete the device entry
from eBPF map only if the newval would be same as current val in the
map. In case that the device was allowed only as read-only but later
we remove all permissions for that device it would remain in the table
with empty values.
The old code would still deny the device but it's not working as
intended. Instead we will update the value in advance. If the updated
value is 0 it means that we are removing all permissions so it should
be removed from the map, otherwise we will update the value in map.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1810356
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Kernel commit <d505b8af58912ae1e1a211fabc9995b19bd40828> added proper
check for cpu quota maximum limit to prevent internal overflow.
Even though this change is not present in all kernels it makes sense
to enforce the same limit in libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1750315
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
I previously did a workaround for a glib event loop race
that causes crashes:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
it turns out that the workaround has a significant performance
penalty on I/O intensive workloads. We thus need to avoid the
workaround if we know we have a new enough glib to avoid the
race condition.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When libvirt added support for firewalld, we were unable to use
firewalld's higher level rules, because they weren't detailed enough
and could not be applied to the iptables FORWARD or OUTPUT chains
(only to the INPUT chain). Instead we changed our code so that rather
than running the iptables/ip6tables/ebtables binaries ourselves, we
would send these commands to firewalld as "passthrough commands", and
firewalld would run the appropriate program on our behalf.
This was done under the assumption that firewalld was somehow tracking
all these rules, and that this tracking was benefitting proper
operation of firewalld and the system in general.
Several years later this came up in a discussion on IRC, and we
learned from the firewalld developers that, in fact, adding iptables
and ebtables rules with firewalld's passthrough commands actually has
*no* advantage; firewalld doesn't keep track of these rules in any
way, and doesn't use them to tailor the construction of its own rules.
Meanwhile, users have been complaining for some time that whenever
firewalld is restarted on a system with libvirt virtual networks
and/or nwfilter rules active, the system logs would be flooded with
warning messages whining that [lots of different rules] could not be
deleted because they didn't exist. For example:
firewalld[3536040]: WARNING: COMMAND_FAILED:
'/usr/sbin/iptables -w10 -w --table filter --delete LIBVIRT_OUT
--out-interface virbr4 --protocol udp --destination-port 68
--jump ACCEPT' failed: iptables: Bad rule
(does a matching rule exist in that chain?).
(See https://bugzilla.redhat.com/1790837 for many more examples and a
discussion)
Note that these messages are created by iptables, but are logged by
firewalld - when an iptables/ebtables command fails, firewalld grabs
whatever is in stderr of the program, and spits it out to the system
log as a warning. We've requested that firewalld not do this (and
instead leave it up to the calling application to do the appropriate
logging), but this request has been respectfully denied.
But combining the two problems above ( 1) firewalld doesn't do
anything useful when you use it as a proxy to add/remove iptables
rules, 2) firewalld often insists on logging lots of
annoying/misleading/useless "error" messages when you use it as a
proxy to remove iptables rules that don't already exist), leads to a
solution - simply stop using firewalld to add and remove iptables
rules. Instead, exec iptables/ip6tables/ebtables directly in the same
way we do when firewalld isn't active.
We still need to keep track of whether or not firewalld is active, as
there are some things that must be done, e.g. we need to add some
actual firewalld rules in the firewalld "libvirt" zone, and we need to
take notice when firewalld restarts, so that we can reload all our
rules.
This patch doesn't remove the infrastructure that allows having
different firewall backends that perform their functions in different
ways, as that will very possibly come in handy in the future when we
want to have an nftables direct backend, and possibly a "pure"
firewalld backend (now that firewalld supports more complex rules, and
can add those rules to the FORWARD and OUTPUT chains). Instead, it
just changes the action when the selected backend is "firewalld" so
that it adds rules directly rather than through firewalld, while
leaving as much of the existing code intact as possible.
In order for tests to still pass, virfirewalltest also had to be
modified to behave in a different way (i.e. by capturing the generated
commandline as it does for the DIRECT backend, rather than capturing
dbus messages using a mocked dbus API).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When it is starting up, firewalld will delete all existing iptables
rules and chains before adding its own rules. If libvirtd were to try
to directly add iptables rules during the time before firewalld has
finished initializing, firewalld would end up deleting the rules that
libvirtd has just added.
Currently this isn't a problem, since libvirtd only adds iptables
rules via the firewalld "passthrough command" API, and so firewalld is
able to properly serialize everything. However, we will soon be
changing libvirtd to add its iptables and ebtables rules by directly
calling iptables/ebtables rather than via firewalld, thus removing the
serialization of libvirtd adding rules vs. firewalld deleting rules.
This will especially apparent (if we don't fix it in advance, as this
patch does) when libvirtd is responding to the dbus NameOwnerChanged
event, which is used to learn when firewalld has been restarted. In
that case, dbus sends the event before firewalld has been able to
complete its initialization, so when libvirt responds to the event by
adding back its iptables rules (with direct calls to
/usr/bin/iptables), some of those rules are added before firewalld has
a chance to do its "remove everything" startup protocol. The usual
result of this is that libvirt will successfully add its private
chains (e.g. LIBVIRT_INP, etc), but then fail when it tries to add a
rule jumping to one of those chains (because in the interim, firewalld
has deleted the new chains).
The solution is for libvirt to preface it's direct calling to iptables
with a iptables command sent via firewalld's passthrough command
API. Since commands sent to firewalld are completed synchronously, and
since firewalld won't service them until it has completed its own
initialization, this will assure that by the time libvirt starts
calling iptables to add rules, that firewalld will not be following up
by deleting any of those rules.
To minimize the amount of extra overhead, we request the simplest
iptables command possible: "iptables -V" (and aside from logging a
debug message, we ignore the result, for good measure).
(This patch is being done *before* the patch that switches to calling
iptables directly, so that everything will function properly with any
fractional part of the series applied).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Even though *we* don't call ebtables/iptables/ip6tables (yet) when the
firewalld backend is selected, firewalld does, so these binaries need
to be there; let's check for them. (Also, the patch after this one is
going to start execing those binaries directly rather than via
firewalld).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
iptables and ip6tables have had a "-w" commandline option to grab a
systemwide lock that prevents two iptables invocations from modifying
the iptables chains since 2013 (upstream commit 93587a04 in
iptables-1.4.20). Similarly, ebtables has had a "--concurrent"
commandline option for the same purpose since 2011 (in the upstream
ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).
Libvirt added code to conditionally use the commandline option for
iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,
November 2013), and for ebtables in upstream commit dc33e6e4a5
(libvirt-1.2.11, November 2014) (the latter actually *re*-added the
locking for iptables/ip6tables, as it had accidentally been removed
during a refactor of firewall code in the interim).
I say "conditionally" because a check was made during firewall module
initialization that tried executing a test command with the
-w/--concurrent option, and only continued using it for actual
commands if that test command completed successfully. At the time the
code was added this was a reasonable thing to do, as it had been less
than a year since introduction of -w to iptables, so many distros
supported by libvirt were still using iptables (and possibly even
ebtables) versions too old to have the new commandline options.
It is now 2020, and as far as I can discern from repology.org (and
manually examining a RHEL7.9 system), every version of every distro
that is supported by libvirt now uses new enough versions of both
iptables and ebtables that they all have support for -w/--concurrent.
That means we can finally remove the conditional code and simply
always use them.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
All the unit tests that use iptables/ip6tables/ebtables have been
written to omit the locking/exclusive use primitive on the generated
commandlines. Even though none of the tests actually execute those
commands (and so it doesn't matter for purposes of the test whether or
not the commands support these options), it still made sense when some
systems had these locking options and some didn't.
We are now at a point where every supported Linux distro has supported
the locking options on these commands for quite a long time, and are
going to make their use non-optional. As a first step, this patch uses
the virFirewallSetLockOverride() function, which is called at the
beginning of all firewall-related tests, to set all the bools
controlling whether or not the locking options are used to true. This
means that all the test cases must be updated to include the proper
locking option in their commandlines.
The change to make actual execs of the commands unconditionally use
the locking option will be in an upcoming patch - this one affects
only the unit tests.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Commit 912c6b22fc622cd7c7d29c7f8eaeb816b266daac added abort() when the
'val' parameter is NULL along with setting the error variable for the
command. We don't want to abort in this case, just set the error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The previous commit exported the function but forgot to add
a non-Linux stub.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: 126cb34a206a44f04e364700b46426dff9f387d5
Using virtCgroupNewSelf() is not correct with cgroups v2 because the
the virt-host-validate process is executed from from the same cgroup
context as the terminal and usually not all controllers are enabled
by default.
To do a proper check we need to use the root cgroup to see what
controllers are actually available. Libvirt or systemd ensures that
all controllers are available for VMs as well.
This still doesn't solve the devices controller with cgroups v2 where
there is no controller as it was replaced by eBPF. Currently libvirt
tries to query eBPF programs which usually works only for root as
regular users will get permission denied for that operation.
Fixes: https://gitlab.com/libvirt/libvirt/-/issues/94
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This reverts commit b3710e9a2af402a2b620de570b062294e11190eb.
That check is very valuable for our code, but it causes issue with glib >=
2.67.0 when building with clang.
The reason is a combination of two commits in glib, firstly fdda405b6b1b which
adds a g_atomic_pointer_{set,get} variants that enforce stricter type
checking (by removing an extra cast) for compilers that support __typeof__, and
commit dce24dc4492d which effectively enabled the new variant of glib's atomic
code for clang. This will not be necessary when glib's issue #600 [0] (8 years
old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this
issue was opened 3 weeks ago, so there is a slight sliver of hope.
[0] https://gitlab.gnome.org/GNOME/glib/-/issues/600
[1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Introduced by commit <22494556542c676d1b9e7f1c1f2ea13ac17e1e3e> which
fixed a CVE.
If the @path passed to virDMSanitizepath() is not a DM name or not a
path to DM name this function could return incorrect sanitized path as
it would always be the first device under /dev/mapper/.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
While it's certainly good to log events like "failed to close fd"
and "tried to close invalid fd", which are likely to be the
consequence of some bug in libvirt, logging a message every single
time a file descriptor is closed successfully is perhaps excessive
and can lead to useful information being missed among the noise.
Log filters don't help in this situation, because filtering out all
of util.file is too big a hammer and would cause important messages
to be left out as well.
To give an idea of just how much noise this single debug statement
can cause, here's a real life example from a quite large libvirtd
log I had to look at recently:
$ grep virFile libvirt.log | wc -l
1307
$ grep virFile libvirt.log | grep -v 'Closed fd' | wc -l
343
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
