This reverts commit 42ab0148dd.
This patch was supposed to fix the checksum of dhcp response packets
by setting it to 0 (because having a non-0 but incorrect checksum was
causing the packets to be droppe on FreeBSD guests).
Early testing was positive, but after the patch was pushed upstream
and more people could test it, it turned out that while it fixed the
dhcp checksum problem for virtio-net interfaces on FreeBSD and
OpenBSD, it also *broke* dhcp checksums for the e1000 emulated NIC on
*all* guests (but not e1000e).
So we're reverting this fix and looking for something more universal
to be included in the next release.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.
Translation: libvirt/libvirt
Translate-URL: https://translate.fedoraproject.org/projects/libvirt/libvirt/
Signed-off-by: Fedora Weblate Translation <i18n@lists.fedoraproject.org>
The docs for submitting a patch describe using your "Legal Name" with
the Signed-off-by line.
In recent times, there's been a general push back[1] against the notion
that use of Signed-off-by in a project automatically requires / implies
the use of legal ("real") names and greater awareness of the downsides.
Full discussion of the problems of such policies is beyond the scope of
this commit message, but at a high level they are liable to marginalize,
disadvantage, and potentially result in harm, to contributors.
TL;DR: there are compelling reasons for a person to choose distinct
identities in different contexts & a decision to override that choice
should not be taken lightly.
A number of key projects have responded to the issues raised by making
it clear that a contributor is free to determine the identity used in
SoB lines:
* Linux has clarified[2] that they merely expect use of the
contributor's "known identity", removing the previous explicit
rejection of pseudonyms.
* CNCF has clarified[3] that the real name is simply the identity
the contributor chooses to use in the context of the community
and does not have to be a legal name, nor birth name, nor appear
on any government ID.
Since we have no intention of ever routinely checking any form of ID
documents for contributors[4], realistically we have no way of knowing
anything about the name they are using, except through chance, or
through the contributor volunteering the information. IOW, we almost
certainly already have people using pseudonyms for contributions.
This proposes to accept that reality and eliminate unnecessary friction,
by following Linux & the CNCF in merely asking that a contributors'
commonly known identity, of their choosing, be used with the SoB line.
[1] Raised in many contexts at many times, but a decent overall summary
can be read at https://drewdevault.com/2023/10/31/On-real-names.html
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
[3] https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md
[4] Excluding the rare GPG key signing parties for regular maintainers
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Many years ago (April 2010), soon after "vhost" in-kernel packet
processing was added to the virtio-net driver, people running RHEL5
virtual machines with a virtio-net interface connected via a libvirt
virtual network noticed that when vhost packet processing was enabled,
their VMs could no longer get an IP address via DHCP - the guest was
ignoring the DHCP response packets sent by the host.
(I've been informed by danpb that the same issue had been encountered,
and "fixed" even earlier than that, in 2006, with Xen as the
hypervisor.)
The "gory details" of the 2010 discussion are chronicled here:
https://lists.isc.org/pipermail/dhcp-hackers/2010-April/001835.html
but basically it was because packet checksums weren't being fully
computed on the host side (because QEMU on the host and the NIC driver
in the guest had agreed between themselves to turn off checksums
because they were unnecessary due to the "link" between the two being
entirely in local memory rather than an error-prone physical cable),
but
1) a partial checksum was being put into the packets at some point by
"someone"
2) the "don't use checksums" info was known by the guest kernel, which
would properly ignore the "bad" checksum), and
3) the packets were being read by the dhclient application on the
guest side with a "raw" socket (thus bypassing the guest kernel UDP
processing that would have known the checksum was irrelevant and
ignore it)),
The "fix" for this ended up being two-tiered:
1) The ISC DHCP package (which contains the aforementioned dhclient
program) made a fix to their dhclient code which caused it to accept
packets anyway even if they didn't have a proper checksum (NB: that's
not a full explanation, and possibly not accurate). This remedied the
problem for guests with an updated dhclient. Here is the code with the
fix to ISC DHCP:
https://github.com/isc-projects/dhcp/blob/master/common/packet.c#L365
This eliminated the issue for any new/updated guests that had the
fixed dhclient, but it didn't solve the problem for existing/old guest
images that didn't/couldn't get their dhclient updated. This brings us
to:
2) iptables added a new "CHECKSUM" target and "--checksum-fill"
action:
http://patchwork.ozlabs.org/patch/58525/
and libvirt added an iptables rule for each virtual network to match
DHCP response packets and perform --checksum-fill. This way by the
time dhclient on the guest read the raw packet, the checksum would be
corrected, and the packet would be accepted. This was pushed upstream
in libvirt commit v0.8.2-142-gfd5b15ff1a.
The word at the time from those more knowledgeable than me was that
the bad checksum problem was really specific to ISC's dhclient running
on Linux, and so once their fix was in use everywhere dhclient was
used, bad checksums would be a thing of the past and the
--checksum-fill iptables rules would no longer be needed (but would
otherwise be harmless if they were still there).
(Plot twist: the dhclient code in fix (1) above apparently is on a
Linux-only code path - this is very important later!)
Based on this information (and also due to the opinion that fixing it
by having iptables modify the packet checksum was really the wrong way
to permanently fix things, i.e. an "ugly hack"), the nftables
developers made the decision to not implement an equivalent to
--checksum-fill in nftables. As a result, when I wrote the nftables
firewall backend for libvirt virtual networks earlier this year, it
didn't add in any rule to "fix" broken UDP checksums (since there was
apparently no equivalent in nftables and, after all, that was fixed
somewhere else 14 years ago, right???)
But last week, when Rich Jones was doing routine testing using a Fedora
40 host (the first Fedora release to use the nftables backend of libvirt's
network driver by default) and a FreeBSD guest, for "some strange
reason", the FreeBSD guest was unable to get an IP address from DHCP!!
https://www.spinics.net/linux/fedora/libvirt-users/msg14356.html
A few quick tests proved that it was the same old "bad checksum"
problem from 2010 come back to haunt us - it wasn't a Linux-only issue
after all.
Phil Sutter and Eric Garver (nftables people) pointed out that, while
nftables doesn't have an action that will *compute* the checksum of a
packet, it *does* have an action that will set the checksum to 0, and
suggested we try adding a "zero the checksum" rule for dhcp response
packets to our nftables ruleset. (Why? Because a checksum value of 0
in a IPv4 UDP packet is defined by RFC768 to mean "no checksum
generated", implying "checksum not needed"). It turns out that this
works - dhclient properly recognizes that a 0 checksum means "don't
bother with the checksum", and accepts the packet as valid.
So to once again fix this timeless bug, this patch adds such a
checksum zeroing rule to the nftables rules setup for each virtual
network.
This has been verified (on a Fedora 40 host) to fix DHCP with FreeBSD
and OpenBSD guests, while not breaking it for Fedora or Windows (10)
guests.
Fixes: b89c4991da
Reported-by: Rich Jones <rjones@redhat.com>
Fix-Suggested-by: Eric Garver <egarver@redhat.com>
Fix-Suggested-by: Phil Sutter <psutter@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Let us introduce the xml and reply files for QEMU 9.1.0 on s390x.
Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Any time the firewalld zone for an interface is set, by definition
that removes it from any previous zone that it was in, so there is
really no point in unsetting the zone if it's just going to be
immediately set again.
This is useful because when firewalld reloads its rules, 3 things happen:
1) firewalld flushes *all* firewall rules (including those added by libvirt)
2) firewalld unsets the zones for all interfaces (including those set
by libvirt)
3) firewalld re-adds its own rules, and sets the zone for all the
interfaces it manages
4) firewalld sends a dbus message that libvirt is watching for, and
when libvirt receives that message, it reloads all of the
libvirt-generated rules, and also re-sets the firewalld zone for
the bridge interfaces managed by libvirt.
libvirt accomplishes step 4 by a) calling
networkRemoveFirewallRules(), and then b) calling
networkAddFirewallRules(). But (because it is useful in other
contexts) networkRemoveFirewallRules() will attempt to *unset* the
zone for each bridge interface, and when firewalld receives this
request, it sees that the bridge interface *has no zone* (because it
was unset by firewalld in step (2) above), and thus logs an error
message.
There is no way for libvirt to suppress an error message that is
logged by firewalld when a request to firewalld fails. But what
libvirt *can* do is realize that in these cases, the firewalld zone is
about to be set again anyway, and so we don't need to unset the zone.
This patch handles that by adding a bool unsetZone to the arguments of
networkRemoveFirewallRules(); most calls to networkRemoveFirewallRules()
have unsetZone=true, but in two cases where the zone is about to be
reset, networkRemoveFirewallRules() is called with unsetZone=false,
which prevents the call to virFirewallDInterfaceUnsetZone() and thus
avoids the unnecessary (and confusing to users!) error message that
would have been logged by firewalld.
Signed-off-by: Laine Stump <laine@redat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The most common "error" when trying to unset the firewalld zone of an
interface is for firewalld to tell us that the interface already isn't
in any zone. Since this is what we want, no need to alarm the user by
logging it as an error.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When a CPU model is reported as usable='no' an additional
<blockers model='...'> element is added for that CPU model to show which
features are missing for the CPU model to become usable.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When starting a domain we check whether the guest CPU definition is
compatible with the host (i.e., when the host supports all features
required both explicitly and by the specified CPU model) as long as
check == 'partial', which is the default.
We are doing so by checking our definition of the CPU model in the CPU
map amending it with explicitly mentioned features and comparing it to
features QEMU would enabled when started with -cpu host. But since our
CPU model definitions often slightly differ from QEMU we may be checking
features which are not actually needed and on the other hand not
checking something that is part of the CPU model in QEMU.
This patch changes the algorithm for CPU models added in the future
(changing it for existing models could cause them to suddenly become
incompatible with the host and domains using them would fail to start).
The new algorithm uses information we probe from QEMU about features
that block each model from being directly usable. If all those features
are explicitly disabled in the CPU definition we consider the base model
compatible with the host. Then we only need to check that all explicitly
required features are supported by QEMU on the host to get the result
for the whole CPU definition.
After this we only use the model definitions (for newly added models)
from CPU map for creating a CPU definition for host-model.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
A function for accessing a list of features blocking CPU model
usability.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
As opposed to the existing virCPUCompare{,XML} this function does not
use CPU model definitions from CPU map. It relies on CPU model usability
info from a hypervisor with a list of blockers that make the selected
CPU model unusable. Explicitly requested features are checked against
the hypervisor's view of a host CPU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The new qemuDomainCheckCPU function is used as a replacement for
virCPUCompare to make sure all callers use the same comparison
algorithm. As a side effect qemuConnectCompareHypervisorCPU now properly
reports CPU compatibility for CPU model that are considered runnable by
QEMU even if our definition of the model disagrees.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function already parses CPU XML on s390. By parsing it consistently
on all architecture we can switch to virCPUCompare and easily replace it
with a QEMU specific helper in the following patch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's get rid of the only explicitly freed variable left in
qemuConnectCompareHypervisorCPU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
On x86 the function returns whether an old style compat check mode
should be used for a specified CPU model according to the CPU map. All
other architectures will always use compat mode.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
CPU models in the CPU map may be marked with <check partial="compat"/>
to indicate a backward compatible partial check (comparing our
definition of the model with the host CPU) should be performed. Other
models will be checked using just runnability info from QEMU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Update with latest lcitool.
Update the build templates to move the definition of exit codes which
are allowed to fail for cirrus jobs for cases when we run out of CI
minutes. The previous location was overridden with the per-job
'allow_failure' value and thus didn't apply.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The libvirt xen driver does not support nwfilters. In fact, since
commit d721b6840f, the driver rejects VM configuration referencing
nwfilters. Drop the needless nwfilter dependency from
libvirt-daemon-xen.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Laine Stump <Laine@redhat.com>
Mention that hypervisors may need a temporary file and document the qemu
template for creating them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When a VM is terminated by host reboot libvirt doesn't get to cleaning
out the temporary overlay file used for transient disks. Since we create
those files with a very specific suffix it's almost guaranteed that if
it exists it's a leftover from a libvirt run. Delete them instead of
complaining to preserve functionality.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/684
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
It's not only used on x86_64 these days. See virSysinfoRead().
Technically we should include loongarch64 in the list as well,
but Fedora hasn't been bootstrapped on the architecture yet,
and when the time comes several more changes are going to be
necessary anyway.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Rather than reimplement everything manually use virBitmapBuffsize to
find the current number of units, realloc the buffer and clear the tail
using virBitmapClearTail().
This fixes a corner case where the buffer would be over-allocated by one
unit when shrinking to the boundary of the unit size.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Extract the clearing of the traling bits from 'virBitmapSetAll' into a
new helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Calculating the number of element can come handy in multiple places,
extract it from virBitmapNew.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
'virBitmapNewCopy()' allocates a new bitmap with the same number of bits
but uses the internal allocation length as argument for the memcpy()
operation to copy the bits. Due to bugs in other code these may not be
the same resulting into a buffer overflow if the source is
over-allocated. Use the buffer length of the target bitmap instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
There's a typo in NEWS.rst where
VIR_MIGRATE_PARAM_MIGRATE_DISKS_DETECT_ZEROES has the _ZEROES
suffix duplicated referring to a non-existent migration
parameter. Drop the suffix.
Fixes: 2e29ab3269
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When constructing the command line for QEMU, some Hyper-V features were
hardcoded, probably due to the fact that they could not have been
automatically translated from the libvirt feature name to QEMU CPU
feature name.
Well now they can be, thanks to their additions to the
virQEMUCapsCPUFeaturesX86 translation table.
Translate all such features the same way when constructing the command
line. This way any future feature that is not translated will be caught
by tests (if a test is added for it) which was not the case when it was
just hardcoded. Hopefully this avoids at least some possible future
issues.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Hyper-V enlightenment features can have hyphenated names which libvirt
exposes under Hyper-V features with underscored names. When libvirt
checks that all requested features were enabled by QEMU (on x86
architectures) it first queries for all those that QEMU knows and
compiles them in a map while using the virQEMUCapsCPUFeaturesX86 for
translations.
Some features (well, all Hyper-V features with underscores) were not
present in the translation table and were incorrectly reported as not
enabled, consequently failing the start of any such domain.
Add all hyphenated/underscored Hyper-V feature names into the
aforementioned translation table. That way domains with these features
enabled can be started when QEMU and the kernel support them.
Resolves: https://issues.redhat.com/browse/RHEL-7122
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Major changes:
* macOS 13 is removed. Cirrus CI now only supports a single
version, macOS 14, so there is no addition of macOS 15
possible.
* The polkit lcitool mapping is renamed to pkcheck
* The polkit package is renamed on Debian & Ubuntu
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The wireshark address.h header uses 'g_memdup2' but this triggers
warnings under clang due to the max version cap:
In file included from ../tools/wireshark/src/plugin.c:27:
In file included from /usr/include/wireshark/epan/proto.h:30:
In file included from /usr/include/wireshark/epan/packet_info.h:15:
/usr/include/wireshark/epan/address.h:107:18: error: 'g_memdup2' is deprecated: Not available before 2.68 [-Werror,-Wdeprecated-declarations]
107 | addr->priv = g_memdup2(&val, sizeof(val));
| ^
/usr/include/glib-2.0/glib/gstrfuncs.h:341:1: note: 'g_memdup2' has been explicitly marked deprecated here
341 | GLIB_AVAILABLE_IN_2_68
| ^
/usr/include/glib-2.0/glib/glib-visibility.h:771:32: note: expanded from macro 'GLIB_AVAILABLE_IN_2_68'
771 | #define GLIB_AVAILABLE_IN_2_68 GLIB_UNAVAILABLE (2, 68)
| ^
/usr/include/glib-2.0/glib/glib-visibility.h:32:35: note: expanded from macro 'GLIB_UNAVAILABLE'
32 | #define GLIB_UNAVAILABLE(maj,min) G_UNAVAILABLE(maj,min) _GLIB_EXTERN
| ^
/usr/include/glib-2.0/glib/gmacros.h:1285:47: note: expanded from macro 'G_UNAVAILABLE'
1285 | #define G_UNAVAILABLE(maj,min) __attribute__((deprecated("Not available before " #maj "." #min)))
| ^
1 error generated.
It is unclear why clang warns, but gcc does not. Our plugin doesn't
actually use the inline helper in address.h that references g_memdup2,
but we get the warning regardless.
Interestingly removing the 'gmodule.h' include avoids the warning. Since
there is nothing in plugin.c that appears to need gmodule.h, removing it
should be safe & done regardless.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We currently create stub 'setcon', 'setcon_raw' and 'security_disable'
APIs in the securityselinuxhelper.c mock, which set env variables to
control how other mock'd libselinux APIs respond. These stubs merely
set some env variables, and we have no need to call these stubs from
the library code, only test code.
The 'security_disable' API is now deprecated in libselinux, so we
stubbing it generates compiler warnings. Rather than workaround that,
just stop stubbing these APIs and set the required env variables
directly. With this change, we now only mock API calls we actually
use from the library code.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
virDomainConfNWFilterInstantiate() was called without updated
net->ifname, it caused in some cases throwing error message. If
function failed, change is reverted.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/658
Signed-off-by: Adam Julis <ajulis@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
