In commit 675174d4ba ("conf, tap: Split netlink and pasta
functions, allow interface configuration"), I broke the initial
setting of the observed IPv6 addresses in two ways:
- the size copied from the configured addresses corresponds to an
IPv4 address, not to an IPv6 address
- the observed link-local address is initialised to the configured
unicast address, not the link-local one
If we haven't seen the guest using some type of addresses yet, we
should default to the configured values, hence these initial
settings: fix both.
This resulted in UDP flows to the guest from a unique local address
on the network not working before the guest shows passt a valid
address itself, as reported by Alona.
Reported-by: Alona Paz <alkaplan@redhat.com>
Link: https://bugs.passt.top/show_bug.cgi?id=16
Fixes: 675174d4ba ("conf, tap: Split netlink and pasta functions, allow interface configuration")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We handle SIGQUIT and SIGTERM calling exit(), which is usually
implemented with the exit_group() system call.
If we don't allow exit_group(), we'll get a SIGSYS while handling
SIGQUIT and SIGTERM, which means a misleading non-zero exit code.
Reported-by: Wenli Quan <wquan@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2101990
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
...instead of argv[0], which might or might not contain a valid path
to the executable itself. Instead of mangling argv[0], use the same
link to find out if we're already running the AVX2 build where
supported.
Alternatively, we could use execvpe(), but that might result in
running a different installed version, in case e.g. the set of
binaries is present in both /usr/bin and /usr/local/bin, with both
being in $PATH.
Reported-by: Wenli Quan <wquan@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2101310
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The Fedora test file extracts some information from the host resolv.conf
into a DNS6 variable which is then never used. Remove this unnecessary
step, which is presumably a leftover from an earlier iteration.
This was the only user of 'head' and 'sed' in the test file, so those can
also be removed from the required tools. The debian and ubuntu test files
also listed 'head' and 'sed' as tools, although they don't use them,
I'm guessing because of an earlier version which had the same DNS6 code.
Remove those as well.
The opensuse test file still actually uses DNS6, so leave it there for now.
The DNS handling and network config handling for SuSE looks to be kind of
broken, but fixing that is a job for another day.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Before booting the guest images, the distro test cases need to modify the
guest images, using virt-edit and guestfish, to boot in the way we need.
At present this gets repeated on every test run, even though it's not
really doing anything we want to test for.
In addition many of the images have the same preparation steps leading to
a lot of duplicated stages in the tests. A number of additional images can
be prepared using common steps, even if the ones used now have small
differences.
Therefore move the preparation of most of the guest images to the asset
build phase, where they can be done a single time for multiple test runs,
using a common preparation script. We can even avoid making a copy of the
disk image for booting, by using qemu's -snapshot option.
A few of the distros (openSUSE and older Ubuntu) do need different steps.
For now we don't chage how they are run, they could possibly be handled
more like this in future.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Rather than directly download distro images from the test scripts, handle
all the downloads during the test asset build, then just clone them for
the tests themselves. This avoids repeated downloads which can be very
slow when debugging failing tests.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Add OPENSUSE_IMGS to DOWNLOAD_ASSETS in Makefile, and note
that xzcat doesn't take a -O option in test/distro/opensuse]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Currently test/run uses wildcards to run all of the tests in a directory.
However, that wildcard list is filtered down by the "onlyfor" directives
in the test files... usually to a single file.
Therefore, just explicitly list the files we *really* want to run for this
test mode. This makes it easier to see at the top level what tests will
be executed, and to change that list temporarily while debugging specific
failures.
This means the "onlyfor" directive no longer has any purpose, and we can
remove it. "onlyfor" was also the only used of the $MODE variable, so we
can remove that too.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The top level listing control of which tests to run is in test/run, however
it uses the test() function which runs an entire directory of test files,
filtered by some criteria. This makes it awkward to narrow down to a
subset of tests when debugging a specific failure.
To make this easier, have test() take an explicit list of test files to
run, and have the caller in test/run handle the directory traversal. The
construct we use for this is pretty awkward to handle the fact that we're
in the source tree root directory rather than test/ at this point in
test/run. Later cleanups will improve that.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The test scripts support a "req" directive which requires one test script
to be run before another. It's implemented by doing a topological sort
based on these directives in the runner scripts, which is about as awkward
as you'd expect in Bourne shell.
It turns out we only use this functionality in one place - to make the
"make install" test run after the plain "make" test. We also already have
a simpler way of making sure tests run in a specific order: just put them
into the same test script file.
So, remove support for the "req" directive and just fold the build/all and
build/install test scripts together.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
If the tests are interrupted at the right point a passt.pid file can be
left over. Clean it up with "make clean" and add it to .gitignore so it
doesn't get accidentally committed.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Apparently qemu's ARM virt machine needs to be explicitly given a firmware
image, rather than just supplying a sane default. Unfortunately the EDK2
firmware image we need isn't in the same place on all host distros.
Currently the test scripts hardcode the Debian location, meaning it will
break on hosts that have it somewhere else. This patch searches multiple
locations for the firmware, and creates a local link during the asset build
phase, which the tests can then use.
For now it only searches the locations used by Debian and Fedora, but
that's a small improvement in robustness already, and can be later improved
further if we need to.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Move the download of mbuto and using it to create a sample initramfs to
the asset build makefile, rather than embedding it in the test scripts
themselves.
The two_guests tests used to use two separate copies of the mbuto
image. As an initramfs the mbuto image is strictly readonly though,
so that's not necessary. So, also use the same image for both guests.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
A number of passt/pasta testcases have initial steps which are just about
building images or other assets we need for the test proper. Repeating
these for each test run can be quite costly.
This patch makes a start on moving this sort of test asset building to
a separate phase before running the tests proper. For now just add a
Makefile to handle the asset building (although it doesn't build
anything yet), and make the path where we'll be building the assets
available to the tests.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
A lot of tests and examples invoke qemu with the command "kvm". However,
as far as I can tell, "kvm" being aliased to the appropriate qemu system
binary is Debian specific. The binary names from qemu upstream -
qemu-system-$ARCH - also aren't universal, but they are more common (they
should be good for both Debian and Fedora at least).
In order to still get KVM acceleration when available, we use the option
"-M accel=kvm:tcg" to tell qemu to try using either KVM or TCG in that
order
A number of the places we invoked "kvm" are expecting specifically an x86
guest, and so it's also safer to explicitly invoke qemu-system-x86_64.
Some others appear to be independent of the target arch (just wanting the
same arch as the host to allow KVM acceleration). Although I suspect there
may be more subtle x86 specific options in the qemu command lines, attempt
to preserve arch independence by using $(uname -m).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Several tests run pp64le guests using "qemu-system-ppc64le". But, at the
system level there's no difference between ppc64 and ppc64le - it's the
same hardware, just placed into different endian modes by OS early boot
code. Reflecting that, qemu only supplies a single "qemu-system-ppc64".
Some distros alias qemu-system-ppc64le to qemu-system-ppc64 (Debian does),
but it's best not to count on this (Fedora doesn't, for example).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
By default, passt detects the nameserver used by the host system by reading
/etc/resolv.conf, and advertises that to the guest via DHCP. However this
breaks down if the host's nameserver is local (on 127.0.0.1 or ::1);
connecting to localhost on the guest won't reach the host's nameserver.
Using a local nameserver is a reasonably common case when using dnsmasq
or similar to merge name resolution on a home network with name resolution
from an organization-private VPN.
We already have the gateway mapping support to allow reaching host-local
services from the guest via the address of the default gateway. Add code
to detect the case of a local DNS server and use the gateway mapping to
advertise it usefully to the guest.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
David reports that dhclient-script(8) on Fedora needs a number of
binaries that are not included in PROGS of the current mbuto profile,
and we would also need to include hostnamectl(1) there, which will
fail without a systemd init.
Embed a minimal script for dhclient(8) in the profile itself, written
to /sbin/dhclient-script at boot, to just check what we need to check
out of DHCP and DHCPv6 functionality.
While at it, drop busybox and logger from PROGS, as we don't need them,
and add hostname(1). While DHCP option 12 isn't supported yet by the
DHCP implementation in passt, we should probably add it soon.
Note: owing to the simplicity of this script, we now need to bring up
the interface before starting dhclient: add this in test scripts where
it's not the case yet.
Reported-by: David Gibson <david@gibson.dropbear.id.au>
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
note that we need to bring up the interface before starting dhclient
In commit fca5e11773 ("qrap: Add probe retry on connection reset
from passt for KubeVirt integration") I just used errno to check if
the connection was reset on recv(), but perror() might set it to
EINVAL if e.g. an underlying logging mechanism fails, so we won't
actually catch the connection reset.
And in case recv() returns 0, errno won't be set, but we're still
using it without resetting it first, which leads to unpredictable
results in that case.
Reset errno before probing with connect(), send() and recv(), and
save it for later checks before calling perror().
Fixes: fca5e11773 ("qrap: Add probe retry on connection reset from passt for KubeVirt integration")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Switch the resolv.conf parsing in conf.c to use the new lineread
implementation. This means that it can now handle a resolv.conf file which
contains blank lines.
There are quite a few other fragilities with the resolv.conf parsing, but
that's out of scope for this patch.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Two places in passt need to read files line by line (one parsing
resolv.conf, the other parsing /proc/net/*. They can't use fgets()
because in glibc that can allocate memory. Instead they use an
implementation line_read() in util.c. This has some problems:
* It has two completely separate modes of operation, one buffering
and one not, the relation between these and how they're activated
is subtle and confusing
* At least in non-buffered mode, it will mishandle an empty line,
folding them onto the start of the next non-empty line
* In non-buffered mode it will use lseek() which prevents using this
on non-regular files (we don't need that at present, but it's a
surprising limitation)
* It has a lot of difficult to read pointer mangling
Add a new cleaner implementation of allocation-free line-by-line
reading in lineread.c. This one always buffers, using a state
structure to keep track of what we need. This is larger than I'd
like, but it turns out handling all the edge cases of line-by-line
reading in C is surprisingly hard.
This just adds the code, subsequent patches will change the existing
users of line_read() to the new implementation.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
This depends on a future change in mbuto to accept external profile
files. Add a file defining what we need for tests and demos, dropping
udhcpc and script as they're not needed anymore, and switch to it.
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
...if there are two or more instances of libvirt in a KubeVirt
scenario using a number of instances of passt, the overlap period
with probing instances of qemu becomes much longer. Switch to 50
retries instead of 5.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
One retry after 100ms was enough for static builds, where qrap
takes a while to start, but it's sometimes not enough with
regular builds. Switch that to five retries with 50ms delay.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In order to probe availability of certain features the Makefile test
compiles a handful of tiny snippets, feeding those in from stdin. However
in one case - the one for -fstack-protector - it forgets to redirect the
output to stdout, meaning it creates a stray '-.s' file when make is
invoked (even make clean).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The use of rm commands in the clean and uninstall targets adds an explicit
leading - to ignore errors. However the built-in RM variable in make is
actually "rm -f" which already ignores errors, so the - isn't neccessary.
Also replace ${RM} with $(RM) which is the more conventional form in
Makefiles.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
pasta, pasta.avx2 and pasta.1 are all generated as a link to the
corresponding passt file. We can consolidate the 3 rules for these targets
into a single pattern rule.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
There are several places which explicitly list the various generated
binaries, even though a $(BIN) variable already lists them. There are
several more places that list all the manpage files, introduce a
$(MANPAGES) variable to remove that repetition as well.
Tweak the generation of pasta.1 as a link to passt.1 so it's not just made
as a side effect of the pasta target.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: add passt.1 and qrap.1 to guest files for distro tests]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The passt/pasta Makefile makes fairly heavy use of GNU make's $(wildcard)
function to locate the sources and headers to build. Using wildcards for
the things to compile is usually a bad idea though: if somehow you end up
with a .c or .h file in your tree you didn't expect it can misbuild in an
exceedingly confusing way. In particular this can sometimes happen if
switching between releases / branches where files have been added or
removed without 100% cleaning the tree.
It also makes life a bit complicated if building multiple different
binaries in the same tree: we already have some rather awkward
$(filter-out) constructions to avoid including qrap.c in the passt build.
Replace use of $(wildcard) with the more idiomatic approach of defining
variables listing all the relevant source files then using that throughout.
In the rule for seccomp.h there was also a bare "*.c" which caused make to
always rebuild that target. Fix that as well.
Similarly, seccomp.sh uses a wildcard to locate the sources, which is
unwise for the same reasons. Make it take the sources to examine on the
command line instead, and have the Makefile pass them in from the same
variables.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
...namely, as connections are discarded or accepted. This was quite
useful to debug an issue with libvirtd failing to start qemu (because
passt refused the new connection) as a previous qemu instance was
still active.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
KubeVirt uses libvirt to start qrap in its current draft integration
(https://github.com/kubevirt/kubevirt/pull/7849/), and libvirtd
starts qrap three times every time a new virtual machine is created:
once on domain creation, and twice on domain start (for "probing")
and to finally start it for real.
Very often, a subsequent invocation of qrap happen before the
previously running instance of qemu terminates, which means that
passt will refuse the new connection as the old one is still active.
Introduce a single retry with a 100ms delay to work around this. This
should be checked again once native libvirt support is there, and
that point qrap will have no reason to exist anymore.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
New from Cppcheck 2.8: all the fields of struct msg that are not
directly manipulated are now reported as unused, which is kind of
correct as those fields are used as a blob "copied" from request
to response and not as separate fields.
However, keeping the message composition explicit is probably
desirable, and adding inline suppressions makes the whole thing
rather unreadable, so just suppress unusedStructMember warnings for
dhcp.c, while also adding a suppression for unmatched suppressions
to keep earlier versions of Cppcheck happy.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A number of the testcases use options specific the OpenBSD version of
netcat. That's available in Debian, but not easily available in Fedora.
Switch the pasta tests to using the nmap version of netcat (a.k.a. ncat).
This is easily available in both Debian and Fedora, and appears to be a
bit more modern and maintained as well.
ncat generally requires explicit listen addresses (which is good for
clarity anywhere). Its default options appear to remove the need for the
-N and -q options.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: changed one ncat listening address to IPv6 loopback]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
For some reason, the passt/pasta tests and examples use dhclient for
DHCPv6, but in most cases use udhcpc for DHCPv4. Change it to use dhclient
for both DHCPv4 and DHCPv6. This means one less tool we need for testing,
plus dhclient is easily available on Fedora whereas udhcpc is not.
Note that the passt tests still rely on udhcpc indirectly because mbuto
wants to put it into the guest images it generates.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
A number of tests and examples use dhclient in both IPv4 and IPv6 modes.
We use "dhclient -6" for IPv6, but usually just "dhclient" for IPv4. Add
an explicit "-4" argument to make it more clear and explicit.
In addition, when dhclient is run from within pasta it usually won't be
"real" root, and so will not have access to write the default global pid
file. This results in a mostly harmless but irritating error:
Can't create /var/run/dhclient.pid: Permission denied
We can avoid that by using the --no-pid flag to dhclient.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
ip(8)'s ability to take abbreviated arguments (e.g. "li sh" instead of
"link show") is very handy when using it interactively, but it doesn't make
for very readable scripts and examples when shown that way.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
distro/fedora contains two versions of the basic tests, used for different
Fedora versions. One uses explicit listening address for netcat in some
extra places, the other does not. Apparently the older netcat versions
didn't require the explicit addresses. Not supplying addresses doesn't
test anything useful though, just a detail in netcat's behaviour. So,
it's cleaner to just always supply explicit addresses.
In addition, we're explicitly expecting the nmap version of ncat, also
known as "ncat". So, it's more explicit what we're after if we invoke it
via that name rather than "nc", which will go via an /etc/alternatives
link.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Fix port argument in distro_quick_pasta_test{,_fedora34} too]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If the first packet_get() call doesn't assign len, the second one
will also return NULL, but gcc doesn't see this.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In conf_runas(), Coverity reports that we might dereference uid and
gid despite possibly being NULL (CWE-476) because of the check after
the first sscanf(). They can't be NULL, but I actually wanted to
check that UID and GID are non-zero (the user could otherwise pass
--runas root:root and defy the whole mechanism).
Later on, we have the same type of warning for 'gr': it's compared
against NULL, so it might be NULL, which is actually the case: but
in that case, we don't dereference it, because we'll return -ENOENT
right away. Rewrite the clause to silence the warning.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
gcc 12.1.x (e.g. current OpenSUSE Tumbleweed, x86_64 only,
gcc-12-1.4.x86_64) reports:
tcp.c: In function ‘tcp_send_flag’:
tcp.c:1014:9: warning: writing 16 bytes into a region of size 0 [-Wstringop-overflow=]
1014 | memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6));
| ^
tcp.c:559:24: note: at offset -16 into destination object ‘low_rtt_dst’ of size 128
559 | static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
|
but 'hole' can't be -1, because the low_rtt_dst table is guaranteed
to have a hole: if we happened to write to the last entry, we'll go
back to index 0 and clear that one.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
On some systems, user and group "nobody" might not be available. The
new --runas option allows to override the default "nobody" choice if
started as root.
Now that we allow this, drop the initgroups() call that was used to
add any additional groups for the given user, as that might now
grant unnecessarily broad permissions. For instance, several
distributions have a "kvm" group to allow regular user access to
/dev/kvm, and we don't need that in passt or pasta.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
With current OpenSUSE Tumbleweed on aarch64 (gcc-12-1.3.aarch64) and
on x86_64 (gcc-12-1.4.x86_64), but curiously not on armv7hl
(gcc-12-1.3.armv7hl), gcc warns about using the _pointer_ to the
802.3 header to write the whole frame to the tap descriptor:
reading between 62 and 4294967357 bytes from a region of size 14
which is bogus:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103483
Probably declaring udp_sock_fill_data_v{4,6}() as noinline would
"fix" this, but that's on the data path, so I'd rather not. Use
a gcc pragma instead.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Having all those 'echo $?' is rather distracting in demos.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Sefano Brivio <sbrivio@redhat.com>
