The passt tests include two static checking tools: clang-tidy and
cppcheck. However, newer versions of those tools have introduced
extra checks, and may cause these tests to fail.
This series fixes all the clang-tidy and cppcheck warnings, either by
altering our code, or by suppressing them with relevant options to the
checkers. With this series, the checks are now clean on both my
Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and
cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with
some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1).
Changes since v1:
* Fixed a whitespace error
* Added extra background information and details to comments and
commit messages when removing old suppressions
* Improved conf_runas() rework to give better error messages
David Gibson (28):
Clean up parsing of port ranges
clang-tidy: Suppress warning about unchecked error in logfn macro
clang-tidy: Fix spurious null pointer warning in pasta_start_ns()
clang-tidy: Remove duplicate #include from icmp.c
Catch failures when installing signal handlers
Pack DHCPv6 "on wire" structures
Clean up parsing in conf_runas()
cppcheck: Reduce scope of some variables
Don't shadow 'i' in conf_ports()
Don't shadow global function names
Stricter checking for nsholder.c
cppcheck: Work around false positive NULL pointer dereference error
cppcheck: Use inline suppression for ffsl()
cppcheck: Use inline suppressions for qrap.c
cppcheck: Use inline suppression for strtok() in conf.c
Avoid ugly 'end' members in netlink structures
cppcheck: Broaden suppression for unused struct members
cppcheck: Remove localtime suppression for pcap.c
qrap: Handle case of PATH environment variable being unset
cppcheck: Suppress same-value-in-ternary branches warning
cppcheck: Suppress NULL pointer warning in tcp_sock_consume()
Regenerate seccomp.h if seccomp.sh changes
cppcheck: Avoid errors due to zeroes in bitwise ORs
cppcheck: Remove unused knownConditionTrueFalse suppression
cppcheck: Remove unused objectIndex suppressions
cppcheck: Remove unused va_list_usedBeforeStarted suppression
Mark unused functions for cppcheck
cppcheck: Remove unused unmatchedSuppression suppressions
Makefile | 26 +---
arch.c | 4 +-
conf.c | 359 +++++++++++++++++++++++-------------------------
dhcpv6.c | 26 ++--
icmp.c | 1 -
igmp.c | 1 +
netlink.c | 22 +--
passt.c | 7 +-
pasta.c | 5 +-
qrap.c | 18 ++-
seccomp.sh | 2 +
siphash.c | 1 +
tap.c | 5 +-
tcp.c | 5 +
test/Makefile | 2 +-
test/nsholder.c | 2 +-
util.c | 2 +-
util.h | 1 +
18 files changed, 236 insertions(+), 253 deletions(-)
--
2.37.3
With gcc 11 and 12, passing -flto, or -flto=auto, and -O2,
intra-procedural optimisation gets rid of a fundamental bit in ndp():
the store of hop_limit in the IPv6 header, before the checksum is
calculated, which on x86_64 looks like this:
ip6hr->hop_limit = IPPROTO_ICMPV6;
b8c0: c6 44 24 35 3a movb $0x3a,0x35(%rsp)
Here, hop_limit is temporarily set to the protocol number, to
conveniently get the IPv6 pseudo-header for ICMPv6 checksum
calculation in memory.
With LTO, the assignment just disappears from the binary.
This is rather visible as NDP messages get a wrong checksum, namely
the expected checksum plus 58, and they're ignored by the guest or
in the namespace, meaning we can't get any IPv6 routes, as reported
by Wenli Quan.
The issue affects a significant number of distribution builds,
including the ones for CentOS Stream 9, EPEL 9, Fedora >= 35,
Mageia Cauldron, and openSUSE Tumbleweed.
As a quick workaround, declare csum_unaligned() as "noipa" for gcc
11 and 12, with -flto and -O2. This disables inlining and cloning,
which causes the assignment to be compiled again.
Leave a TODO item: we should figure out if a gcc issue has already
been reported, and report one otherwise. There's no apparent
justification as to why the store could go away.
Reported-by: Wenli Quan <wquan(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
Makefile | 7 +++++++
checksum.c | 3 +++
2 files changed, 10 insertions(+)
diff --git a/Makefile b/Makefile
index 1d45f17..d4b623f 100644
--- a/Makefile
+++ b/Makefile
@@ -50,11 +50,18 @@ HEADERS = $(PASST_HEADERS) seccomp.h
# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78993
# from the pointer arithmetic used from the tcp_tap_handler() path to get the
# remote connection address.
+#
+# TODO: With the same combination, in ndp(), gcc optimises away the store of
+# hop_limit in the IPv6 header (temporarily set to the protocol number for
+# convenience, to mimic the ICMPv6 checksum pseudo-header) before the call to
+# csum_unaligned(). Mark csum_unaligned() as "noipa" as a quick work-around,
+# while we figure out if a corresponding gcc issue has already been reported.
ifeq (,$(filter-out 11 12, $(shell $(CC) -dumpversion)))
ifneq (,$(filter -flto%,$(FLAGS) $(CFLAGS)))
ifneq (,$(filter -O2,$(FLAGS) $(CFLAGS)))
FLAGS += -DTCP_HASH_NOINLINE
FLAGS += -DSIPHASH_20B_NOINLINE
+ FLAGS += -DCSUM_UNALIGNED_NO_IPA
endif
endif
endif
diff --git a/checksum.c b/checksum.c
index acb1e3e..56ad01e 100644
--- a/checksum.c
+++ b/checksum.c
@@ -97,6 +97,9 @@ uint16_t csum_fold(uint32_t sum)
*
* Return: 16-bit IPv4-style checksum
*/
+#if CSUM_UNALIGNED_NO_IPA
+__attribute__((__noipa__)) /* See comment in Makefile */
+#endif
uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
{
return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
--
2.35.1
The passt tests include two static checking tools: clang-tidy and
cppcheck. However, newer versions of those tools have introduced
extra checks, and may cause these tests to fail.
This series fixes all the clang-tidy and cppcheck warnings, either by
altering our code, or by suppressing them with relevant options to the
checkers. With this series, the checks are now clean on both my
Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and
cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with
some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1).
David Gibson (28):
Clean up parsing of port ranges
clang-tidy: Suppress warning about unchecked error in logfn macro
clang-tidy: Fix spurious null pointer warning in pasta_start_ns()
clang-tidy: Remove duplicate #include from icmp.c
Catch failures when installing signal handlers
Pack DHCPv6 "on wire" structures
Clean up parsing in conf_runas()
cppcheck: Reduce scope of some variables
Don't shadow 'i' in conf_ports()
Don't shadow global function names
Stricter checking for nsholder.c
cppcheck: Work around false positive NULL pointer dereference error
cppcheck: Use inline suppression for ffsl()
cppcheck: Use inline suppressions for qrap.c
cppcheck: Use inline suppression for strtok() in conf.c
Avoid ugly 'end' members in netlink structures
cppcheck: Broaden suppression for unused struct members
cppcheck: Remove localtime suppression for pcap.c
qrap: Handle case of PATH environment variable being unset
cppcheck: Suppress same-value-in-ternary branches warning
cppcheck: Suppress NULL pointer warning in tcp_sock_consume()
Regenerate seccomp.h if seccomp.sh changes
cppcheck: Avoid errors due to zeroes in bitwise ORs
cppcheck: Remove unused knownConditionTrueFalse suppression
cppcheck: Remove unused objectIndex suppressions
cppcheck: Remove unused va_list_usedBeforeStarted suppression
Mark unused functions for cppcheck
cppcheck: Remove unused unmatchedSuppression suppressions
Makefile | 26 +---
arch.c | 4 +-
conf.c | 344 ++++++++++++++++++++++--------------------------
dhcpv6.c | 26 ++--
icmp.c | 1 -
igmp.c | 1 +
netlink.c | 22 ++--
passt.c | 7 +-
pasta.c | 5 +-
qrap.c | 18 ++-
seccomp.sh | 2 +
siphash.c | 1 +
tap.c | 5 +-
tcp.c | 2 +
test/Makefile | 2 +-
test/nsholder.c | 2 +-
util.c | 2 +-
util.h | 1 +
18 files changed, 222 insertions(+), 249 deletions(-)
--
2.37.3
The first one is a bogus report of an uninitialised field passed as
input to hash functions for TCP connections, already seen with gcc 11.
The second one is a bogus stringop-overread which already happened
with gcc 12.1, but this time a "cute" pragma won't make it go away.
Both harmless, but noisy.
Based on pending series posted by David, if it matters.
Stefano Brivio (2):
Makefile: Extend noinline workarounds for LTO and -O2 to gcc 12
udp: Replace pragma to ignore bogus stringop-overread warning with
workaround
Makefile | 6 +++---
udp.c | 26 ++++++++++++++++++--------
util.h | 23 -----------------------
3 files changed, 21 insertions(+), 34 deletions(-)
--
2.35.1
Here's another bundle of test fixes, focused on making the tests
faster and more reliable.
Based on my earlier series cleaning up the port forwarding data
structures.
Changes since v1:
* Rebased and fixed conflict with another Makefile change
* Dropped patch removing sleeps from layout functions
David Gibson (11):
test: Add wait_for() shell helper
test: Remove unnecessary sleeps from shutdown tests
test: More robust wait for pasta/passt to be ready
test: Use --config-net for namespace setup
test: Simplify data handling for transfer tests
test: Remove unneccessary pane naming from layout_two_guests
clang-tidy: Disable 'readability-identifier-length'
cppcheck: Avoid excessive scanning due to system headers
cppcheck: Run quietly
Makefile: Simplify getting target triple for compiler
cppcheck: Add target specific headers
Makefile | 33 +++----
test/.gitignore | 1 +
test/Makefile | 15 ++-
test/lib/context | 4 +-
test/lib/layout | 4 -
test/lib/setup | 32 +++----
test/lib/util | 8 ++
test/passt.mbuto | 6 +-
test/passt/shutdown | 1 -
test/passt/tcp | 53 +++++------
test/passt/udp | 29 +++---
test/passt_in_ns/shutdown | 1 -
test/passt_in_ns/tcp | 187 ++++++++++++++++----------------------
test/passt_in_ns/udp | 93 ++++++++-----------
test/pasta/tcp | 79 +++++++---------
test/pasta/udp | 43 ++++-----
test/two_guests/basic | 2 +-
17 files changed, 251 insertions(+), 340 deletions(-)
--
2.37.3
The information we need to keep track of which ports are forwarded
where is currently split across several different values and data
structures in several different places. Worse, a number of those
structures are incorrectly sized due to an off by one error, which
could lead to buffer overruns.
Fix the sizing, and re-organize the data structures in a way that
should make it less likely to repeat that mistake.
While we're there, correct a similar off-by-one mis-sizing of a number
of other arrays.
Changes since v1:
* Use a define for the array size, rather than a typedef to handle
the bitmaps of ports
David Gibson (8):
Improve types and names for port forwarding configuration
Consolidate port forwarding configuration into a common structure
udp: Delay initialization of UDP reversed port mapping table
Don't use indirect remap functions for conf_ports()
Pass entire port forwarding configuration substructure to conf_ports()
Treat port numbers as unsigned
Fix widespread off-by-one error dealing with port numbers
icmp: Correct off by one errors dealing with number of echo request
ids
Makefile | 2 +-
conf.c | 136 +++++++++++++++++++++++----------------------------
icmp.c | 5 +-
passt.h | 1 +
port_fwd.h | 34 +++++++++++++
tcp.c | 68 ++++++++------------------
tcp.h | 13 ++---
tcp_splice.c | 4 +-
udp.c | 59 ++++++++++------------
udp.h | 27 +++++-----
10 files changed, 168 insertions(+), 181 deletions(-)
create mode 100644 port_fwd.h
--
2.37.3
Hello everyone!
So, while playing with out passt package for openSUSE, my colleague
Vasiliy found out an issue in how passt assumes the qemu binary to be
called.
In fact, in openSUSE, while we have a package that allows people to
launch qemu by invoking `qemu-kvm`, we don't install it by default, and
we therefore only have the standard upstream names in the filesystem,
which have the ARCH part in lowercase.
This patch, which we're currently carrying as a downstream one in our
package, should fix the problem.
Thanks and Regards,
Dario
---
Vasiliy Ulyanov (1):
Fix the name of the qemu-system-* executable
Makefile | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
The information we need to keep track of which ports are forwarded
where is currently split across several different values and data
structures in several different places. Worse, a number of those
structures are incorrectly sized due to an off by one error, which
could lead to buffer overruns.
Fix the sizing, and re-organize the data structures in a way that
should make it less likely to repeat that mistake.
While we're there, correct a similar off-by-one mis-sizing of a number
of other arrays.
David Gibson (8):
Improve types and names for port forwarding configuration
Consolidate port forwarding configuration into a common structure
udp: Delay initialization of UDP reversed port mapping table
Don't use indirect remap functions for conf_ports()
Pass entire port forwarding configuration substructure to conf_ports()
Treat port numbers as unsigned
Fix widespread off-by-one error dealing with port numbers
icmp: Correct off by one errors dealing with number of echo request
ids
Makefile | 2 +-
conf.c | 136 +++++++++++++++++++++++----------------------------
icmp.c | 5 +-
passt.h | 1 +
port_fwd.h | 34 +++++++++++++
tcp.c | 68 ++++++++------------------
tcp.h | 13 ++---
tcp_splice.c | 4 +-
udp.c | 59 ++++++++++------------
udp.h | 27 +++++-----
10 files changed, 168 insertions(+), 181 deletions(-)
create mode 100644 port_fwd.h
--
2.37.3
After the recent rework of command dispatch and handling of temporary
files, a few issues prevent demos from completing, and CI files from
being uploaded. Fix those.
On top of that, rebase to Podman's latest upstream so that the Podman
demo can run again, shorten paths used to change directories in demo
scripts, and fix a number of inconsistencies in the README.
Given that the current state of the website is inconsistent, I'd be
inclined to apply these right away, while posting to ease later
review anyway.
Stefano Brivio (11):
contrib/podman: Rebase to latest upstream
hooks/pre_push: Fix upload of CI's logs and terminal capture file
test/demo: Use relative paths to change directories when possible
test/demo: Avoid using port 5201 on the host
test/lib: Drop perf_report_append() from perf_report
test/lib: Don't try to write to perf.js when running demos
README: Fix misspellings of openSUSE
README: Point openSUSE links to Dario's OBS repository
README: Fix indentation in "Try It" section
README: Fix paragraph in Try It section of passt
README: Add legend for Features section
README.md | 24 ++-
...001-libpod-Add-pasta-networking-mode.patch | 95 +++++-----
hooks/pre-push | 11 +-
test/demo/passt | 45 +++--
test/demo/pasta | 20 +-
test/demo/podman | 176 +++++++++---------
test/lib/perf_report | 13 +-
7 files changed, 198 insertions(+), 186 deletions(-)
--
2.35.1