While glibc provides qsort(), which usually is just a mergesort,
until sorting arrays so huge that temporary array used by
mergesort would not fit into physical memory (which in our case
is never), we are not guaranteed it'll use mergesort. The
advantage of mergesort is clear - it's stable. IOW, if we have an
array of values parsed from XML, qsort() it and produce some
output based on those values, we can then compare the output with
some expected output, line by line.
But with newer glibc this is all history. After [1], qsort() is
no longer mergesort but introsort instead, which is not stable.
This is suboptimal, because in some cases we want to preserve
order of equal items. For instance, in ebiptablesApplyNewRules(),
nwfilter rules are sorted by their priority. But if two rules
have the same priority, we want to keep them in the order they
appear in the XML. Since it's hard/needless work to identify
places where stable or unstable sorting is needed, let's just
play it safe and use stable sorting everywhere.
Fortunately, glib provides g_qsort_with_data() which indeed
implement mergesort and it's a drop in replacement for qsort(),
almost. It accepts fifth argument (pointer to opaque data), that
is passed to comparator function, which then accepts three
arguments.
We have to keep one occurance of qsort() though - in NSS module
which deliberately does not link with glib.
1: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=03bf8357e8291857a435afcc3048e0b697b6cc04
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It's somewhat confusing that some of the services have a
corresponding foo.service.extra.in and foo.socket.extra.in, some
have just one of the two, and some have neither.
In order to make things more approachable, make sure that both
files exists for each service.
In most cases the extra units are currently unused, so they will
just contain a comment briefly explaining their purpose and
pointing users to meson.build, where they can find more
information. The same comment is also added to the top of
extra units that already have some contents in them for
consistency.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Hypervisors are referred to by their user-facing name rather
than the name of their libvirt driver, the monolithic daemon is
explicitly referred to as legacy, and a consistent format is
used throughout.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We're about to change the defaults and start migrating to common
templates: in order to be able to switch units over one at a
time, make the input files that are currently used explicit
rather than implicit.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now that providing the value is optional, we can remove almost
all uses.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
For most services, the value provided explicitly matches the
documented default.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Error messages are exempt from the 80 columns rule. Move them
onto one line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
This is a more concise approach and guarantees there is
no time window where the struct is uninitialized.
Generated using the following semantic patch:
@@
type T;
identifier X;
@@
- T X;
+ T X = { 0 };
... when exists
(
- memset(&X, 0, sizeof(X));
|
- memset(&X, 0, sizeof(T));
)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
All services are ordered after local-fs.target unless they have set
DefaultDependencies=no, which we do not do.
https://gitlab.com/libvirt/libvirt/-/issues/489
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Convert the fields to the proper types and use virXMLPropEnum for
parsing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The parameter was added for consistency with virPidFileAcquirePath.
However, all callers of virPidFileAcquire pass false.
Remove the argument.
Partially-reverts: 2250a2b5d2
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Use a temporary variable to avoid memory alignment issues on ARM:
../src/nwfilter/nwfilter_dhcpsnoop.c: In function ‘virNWFilterSnoopLeaseFileLoad’:
../src/nwfilter/nwfilter_dhcpsnoop.c:1745:20: error: cast increases required alignment of target type [-Werror=cast-align]
1745 | (unsigned long long *) &ipl.timeout,
|
Fixes: 0d278aa089
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Coverity scan reports:
"A time_t value is stored in an integer with too few bits to accommodate
it. The expression timeout is cast to unsigned int"
We are already casting and storing time_t timeout variable into unsigned int.
We can use time_t for timeout and cast it to unsigned long (should be big enough)
instead of unsigned int in sscanf, g_strdup_printf as required.
Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace virNWFilterDefParseString/File with the common function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Remove the virNWFilterBindingDefParseString/File thin wrappers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Upcoming patch which is fixing the opening of drivers in monolithic mode
needs to know whether we are inside 'libvirtd' but the code where the
decision needs to happen is not re-compiled per daemon. Thus we need to
pass this information to the stateful driver init function so that it
can be remebered.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When firewalld is restarted or has its rules reloaded, we trigger a
reload of the nwfilter driver. This is done directly in the main
event loop thread which is a bad idea.
In a previous commit we fixed a actual deadlock problem with the
virStateReload API, when triggered from SIGHUP:
commit 33c6eb9689
Author: Jim Fehlig <jfehlig@suse.com>
Date: Thu Mar 8 15:04:48 2018 -0700
libvirtd: fix potential deadlock when reloading
The same deadlock problem previously existed with the firewalld reload
trigger, however, today it is not quite so series. The QEMU driver uses
a private event thread for each VM, so the particular deadlock would
not occur. None the less during the time the filters are reloading all
use of the event loop is blocked, which prevents APIs being serviced.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Under certain circumstances nwfilterStateInitialize could leak memory:
If e.g. the call to virNWFilterConfLayerInit fails, the error path
err_techdrivers_shutdown does not free the previously allocated memory
held in driver->stateDir.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Allow nwfilterStateCleanupLocked to be called on a partially constructed
driver object.
This enables the next patch to simplify and fix error handling in
nwfilterStateInitialize.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Allow nwfilterDriverRemoveDBusMatches to be called without
nwfilterDriverInstallDBusMatches being called previously.
This enables a later patch to use nwfilterDriverRemoveDBusMatches
as a cleanup function safely.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
virNWFilterDHCPSnoopShutdown would never destroy the mutexes created
in virNWFilterDHCPSnoopInit. Additionally, if in virNWFilterDHCPSnoopInit
the call to virMutexInitRecursive succeeds and the call to virMutexInit
fails, this would lead to either virNWFilterSnoopState.snoopLock being
initialized twice or virNWFilterSnoopState.activeLock destroyed without
being initialized first.
This enables a later patch to use virNWFilterDHCPSnoopShutdown as a
cleanup function safely, as it is a no-op if virNWFilterSnoopState was
not yet initialized.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The updateLock is a R/W lock held by anything which needs to read or
modify the rules associated with an NWFilter.
APIs for defining/undefining NW filters rules hold a write lock on
updateLock.
APIs for creating/deleting NW filter bindings hold a read lock on
updateLock, which prevents define/undefine taking place concurrently.
The problems arise when we attempt to creating two NW filter bindings in
parallel.
Thread 1 can acquire the mutex for filter A
Thread 2 can acquire the mutex for filter B
Consider if filters A and B both reference filters C and D, but in
different orders:
Filter A
-> filter C
-> filter D
Filter B
-> filter D
-> filter C
Thread 1 will try to acquire locks in order A, C, D while thread 1 will
try to acquire in order A, D, C. Deadlock can still occur.
Think we can sort the list of filters before acquiring locks on all of
them ? Nope, we allow arbitrary recursion:
Filter A
-> filter C
-> filter E
-> filter F
-> filter H
-> filter K
-> filter D
-> filter G
-> filter I
So we can't tell from looking at 'A' which filters we're going to
need to lock. We can only see the first level of filters references
and we need to lock those before we can see the second level of
filters, etc.
We could probably come up with some cleverness to address this but
it isn't worth the time investment. It is simpler to just keep the
process of creating NW filter bindings totally serialized.
Using two separate locks for this serialization though is pointless.
Every code path which gets a read(updateLock) will go on to hold
updateMutex. It is simpler to just hold write(updateLock) and
get rid of updateMutex. At that point we don't need updateLock
to be a R/W lock, it can be a plain mutex.
Thus this patch gets rid of the current updateLock and updateMutex
and introduces a new top level updateMutex.
This has a secondary benefit of introducing fairness into the
locking. With a POSIX R/W lock, you get writer starvation if
you have lots of readers. IOW, if we call virNWFilterBIndingCreate
and virNWFilterBindingDelete in a tight loop from a couple of
threads, we can prevent virNWFilterDefine from ever acquiring
a write lock.
Getting rid of the R/W lock gives us FIFO lock acquisition
preventing starvation of any API call servicing.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In the not so distant past, the lock ordering in
virNWFilterLockIface() was as follows: global mutex ifaceMapLock
was acquired, then internal representation of given interface was
looked up in a hash table (or created brand new if none was
found), the global lock was released and the lock of the
interface was acquired.
But this was mistakenly changed as the function was rewritten to
use automatic mutexes, because now the global lock is held
throughout the whole run of the function and thus the interface
specific lock is acquired with the global lock held. This results
in a deadlock.
Fixes: dd8150c48d
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This allows nwfilterStateCleanupLocked to be used in
nwfilterStateInitialize in a later patch.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This enables a later patch to simplify locking during initialization
and cleanup of virNWFilterDriverState.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The current use of an array for nwfilter objects requires
the caller to iterate over all elements to find a filter,
and also requires locking each filter.
Switching to a pair of hash tables enables O(1) lookups
both by name and uuid, with no locking required.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The comment against the 'updateMutex' refers to a problem with
lock ordering when looking up filters in the virNWFilterObjList
which uses an array. That problem does indeed exist.
Unfortunately it claims that switching to a hash table would
solve the lock ordering problems during instantiation. That
is not correct because there is a second lock ordering
problem related to how we traverse related filters when
instantiating filters. Consider a set of filters:
Filter A:
Reference Filter C
Reference Filter D
Filter B:
Reference Filter D
Reference Filter C
In one example, we lock A, C, D, in the other example
we lock A, D, C.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virNWFilterObjListNumOfNWFilters method iterates over the
driver->nwfilters, accessing virNWFilterObj instances. As such
it needs to be protected against concurrent modification of
the driver->nwfilters object.
This API allows unprivileged users to connect, so users with
read-only access to libvirt can cause a denial of service
crash if they are able to race with a call of virNWFilterUndefine.
Since network filters are usually statically defined, this is
considered a low severity problem.
This is assigned CVE-2022-0897.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Long ago we adapted to Linux kernel changes which inverted the
behaviour of the conntrack --ctdir setting:
commit a6a04ea47a
Author: Stefan Berger <stefanb@us.ibm.com>
Date: Wed May 15 21:02:11 2013 -0400
nwfilter: check for inverted ctdir
Linux netfilter at some point (Linux 2.6.39) inverted the meaning of the
'--ctdir reply' and newer netfilter implementations now expect
'--ctdir original' instead and vice-versa.
We check for the kernel version and assume that all Linux kernels with version
2.6.39 have the newer inverted logic.
Any distro backporting the Linux kernel patch that inverts the --ctdir logic
(Linux commit 96120d86f) must also backport this patch for Linux and
adapt the kernel version being tested for.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Given our supported platform targets, we no longer need to
consider a version of Linux before 2.6.39, so can drop
support for the old direction behaviour.
The test suite updates are triggered because that never
probed for the ctdir direction, and so the iptables syntax
generator unconditionally dropped the ctdir args.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Long ago we adapted to iptables changes by introducing support
for '-m conntrack':
commit 06844ccbaa
Author: Stefan Berger <stefanb@us.ibm.com>
Date: Tue Aug 6 20:30:46 2013 -0400
nwfilter: Use -m conntrack rather than -m state
Since iptables version 1.4.16 '-m state --state NEW' is converted to
'-m conntrack --ctstate NEW'. Therefore, when encountering this or later
versions of iptables use '-m conntrack --ctstate'.
Given our supported platform targets, we no longer need to
consider a version of iptables before 1.4.16, so can drop
support for the old syntax.
The test suite updates are triggered because that never
probed for the new syntax, and so unconditionally
generated the old syntax.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The virNWFilterTechDriverForName & virNWFilterUpdateInstantiateFilter
methods are only used within the same source file, so don't need to
be exported.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This method doesn't exist since
commit d1a7c08eb1
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Thu Apr 26 12:26:51 2018 +0100
nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The nwfilter update lock is historically acquired by the virt
drivers in order to achieve serialization between nwfilter
define/undefine, and instantiation/teardown of filters.
When running in the modular daemons, however, the mutex that
the virt drivers are locking is in a completely different
process from the mutex that the nwfilter driver is locking.
Serialization is lost and thus call from the virt driver to
virNWFilterBindingCreateXML can deadlock with a concurrent
call to the virNWFilterDefineXML method.
The solution is surprisingly easy, the update lock simply
needs acquiring in the virNWFilterBindingCreateXML method
and virNWFilterBindingUndefine method instead of in the
virt drivers.
The only semantic difference here is that when a virtual
machine has multiple NICs, the instantiation and teardown
of filters is no longer serialized for the whole VM, but
rather for each NIC. This should not be a problem since
the virt drivers already need to cope with tearing down
a partially created VM where only some of the NICs are
setup.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The returned packet can have less strict alignment (u_char) than the struct
(ether_header) we are casting it to, so to avoid alignment issues just copy the
header into the struct on the stack.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This change was generated using the following spatch:
@ rule1 @
expression a;
identifier f;
@@
<...
- f(*a);
... when != a;
- *a = NULL;
+ g_clear_pointer(a, f);
...>
@ rule2 @
expression a;
identifier f;
@@
<...
- f(a);
... when != a;
- a = NULL;
+ g_clear_pointer(&a, f);
...>
Then, I left some of the changes out, like tools/nss/ (which
doesn't link with glib) and put back a comment in
qemuBlockJobProcessEventCompletedActiveCommit() which coccinelle
decided to remove (I have no idea why).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Some files do not include what they use and rely on virutil.h
to pull in the necessary header files.
Fix it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
We recently started listing these in the spec file and, since we
were not creating them during the installation phase, that broke
RPM builds.
Fixes: 4b43da0bff
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The service files were copied out of the service file for libvirtd and
the name of the corresponding manpage was not fixed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2045959
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
