- dev - passt

passt: new version 2023_02_27.c538ee8 available
by Stefano Brivio 27 Feb '23

27 Feb '23

The new version with tag 2023_02_27.c538ee8 includes the following changes: c538ee8 util: Add own prototype for __clone2() on ia64 b1b75bd contrib/apparmor: Split profile into abstractions, use them 0d547a5 qrap: Generate -netdev as JSON 4f2341f qrap: Introduce machine-specific PCI address base 8828a63 qrap: Drop args in JSON format 16f19c8 qrap: Fix support for pc machines 9cec430 qrap: Fix limits for PCI addresses d2df763 log, conf, tap: Define die() as err() plus exit(), drop cppcheck workarounds fb05a71 doc/demo: Fix and suppress ShellCheck warnings 26a0e4d Fix definitions of SOCKET_MAX, TCP_MAX_CONNS 4f523c3 tcp: Avoid (theoretical) resource leak (CWE-772) Coverity warning a1d5537 tcp: Avoid false (but convoluted) positive Coverity CWE-476 warning 5474bc5 tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls() a48c5c2 treewide: Disable gcc strict aliasing rules as needed, drop workarounds da46fda tcp: Suppress knownConditionTrueFalse cppcheck false positive 834b9a3 log: Send identifier string in log messages, openlog() won't work for us bad2526 conf, udp: Allow any loopback address to be used as resolver 8ca907a conf: Split add_dns{4,6}() out of get_dns() ddf7097 udp: Actually use host resolver to forward DNS queries https://passt.top/passt/log/?qt=range&q=2023_02_22.4ddbcb9..2023_02_27.c538… Packages: - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/5574807/ permanent mirror: https://passt.top/builds/copr/0^20230227.gc538ee8/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Ubuntu tracker: https://packages.ubuntu.com/lunar/passt - Static builds: - Package for other RPM-based distributions, x86_64 only: https://passt.top/builds/latest/x86_64/passt-gc538ee8-1.x86_64.rpm - x86_64 static binaries: https://passt.top/builds/latest/x86_64/ - Debian package, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt_0c538ee8-1_all.deb -- Stefano

1 0

[PATCH] util: Add own prototype for __clone2() on ia64
by Stefano Brivio 27 Feb '23

27 Feb '23

ia64 needs to use __clone2() as clone() is not available, but glibc doesn't export the prototype. Take it from clone(2) to avoid an implicit declaration: util.c: In function ‘do_clone’: util.c:512:16: warning: implicit declaration of function ‘__clone2’ [-Wimplicit-function-declaration] 512 | return __clone2(fn, stack_area + stack_size / 2, stack_size / 2, | ^~~~~~~~ Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- util.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/util.c b/util.c index c5ee1c0..799173f 100644 --- a/util.c +++ b/util.c @@ -495,6 +495,15 @@ int write_file(const char *path, const char *buf) return len == 0 ? 0 : -1; } +#ifdef __ia64__ +/* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(), + * use the description from clone(2). + */ +int __clone2(int (*fn)(void *), void *stack_base, size_t stack_size, int flags, + void *arg, ... /* pid_t *parent_tid, struct user_desc *tls, + pid_t *child_tid */ ); +#endif + /** * do_clone() - Wrapper of __clone2() for ia64, clone() for other architectures * @fn: Entry point for child -- 2.39.1

2 1

[PATCH 0/5] qrap: Fix a number of issues
by Andrea Bolognani 27 Feb '23

27 Feb '23

Andrea Bolognani (5): qrap: Fix limits for PCI addresses qrap: Fix support for pc machines qrap: Drop args in JSON format qrap: Introduce machine-specific PCI address base qrap: Generate -netdev as JSON qrap.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-) -- 2.39.2

3 14

[PATCH] contrib/apparmor: Split profile into abstractions, use them
by Stefano Brivio 27 Feb '23

27 Feb '23

One day, libvirt might actually support running passt to provide guest connectivity. Should libvirtd (or virtqemud) start passt, it will need to access socket and PID files in specific locations, and passt needs to accept SIGTERM in case QEMU fails to start after passt is already started. To make this more convenient, split the current profile into two abstractions, for passt and for pasta, so that external programmes can include the bits they need (and especially not include the pasta abstraction if they only need to start passt), plus whatever specific adaptation is needed. For stand-alone usage of passt and pasta, the 'passt' profile simply includes both abstractions, plus rules to create and access PID and capture files in default or reasonable ($HOME) locations. Tested on Debian with libvirt 9.0.0 together with a local fix to start passt as intended, namely libvirt commit c0efdbdb9f66 ("qemu_passt: Avoid double daemonizing passt"). This is an example of how the libvirtd profile (or virtqemud abstraction, or virtqemud profile) can use this: # support for passt network back-end /usr/bin/passt Cx -> passt, profile passt { /usr/bin/passt r, owner @{run}/user/[0-9]*/libvirt/qemu/run/passt/* rw, signal (receive) set=("term") peer=/usr/sbin/libvirtd, signal (receive) set=("term") peer=libvirtd, include if exists <abstractions/passt> } translated: - when executing /usr/bin/passt, switch to the subprofile "passt" (not the "discrete", i.e. stand-alone profile), described below. Scrub the environment (e.g. LD_PRELOAD is dropped) - in the "passt" subprofile: - allow reading the binary - allow read and write access to PID and socket files - make passt accept SIGTERM from /usr/sbin/libvirtd, and libvirtd peer names - include anything else that's needed by passt itself Suggested-by: Andrea Bolognani <abologna(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- contrib/apparmor/abstractions/passt | 47 ++++++++++++++++++++ contrib/apparmor/abstractions/pasta | 35 +++++++++++++++ contrib/apparmor/usr.bin.passt | 67 +++-------------------------- 3 files changed, 89 insertions(+), 60 deletions(-) create mode 100644 contrib/apparmor/abstractions/passt create mode 100644 contrib/apparmor/abstractions/pasta diff --git a/contrib/apparmor/abstractions/passt b/contrib/apparmor/abstractions/passt new file mode 100644 index 0000000..000e3db --- /dev/null +++ b/contrib/apparmor/abstractions/passt @@ -0,0 +1,47 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# +# PASST - Plug A Simple Socket Transport +# for qemu/UNIX domain socket mode +# +# PASTA - Pack A Subtle Tap Abstraction +# for network namespace/tap device mode +# +# contrib/apparmor/abstractions/passt - Abstraction for passt(1) +# +# Copyright (c) 2022 Red Hat GmbH +# Author: Stefano Brivio <sbrivio(a)redhat.com> + + abi <abi/3.0>, + + include <abstractions/base> + + # Alternatively: include <abstractions/nameservice> + @{etc_ro}/resolv.conf r, # get_dns(), conf.c + + capability net_bind_service, # isolation.c, conf.c + capability setuid, + capability setgid, + capability sys_admin, + capability setpcap, + capability net_admin, + capability sys_ptrace, + + / r, # isolate_prefork(), isolation.c + mount "" -> "/", + mount "" -> "/tmp/", + pivot_root "/tmp/" -> "/tmp/", + umount "/", + + network netlink raw, # nl_sock_init_do(), netlink.c + + network inet stream, # tcp.c + network inet6 stream, + + network inet dgram, # udp.c + network inet6 dgram, + + network unix stream, # tap.c + + network unix dgram, # __openlog(), log.c + + /usr/bin/passt.avx2 ix, # arch_avx2_exec(), arch.c diff --git a/contrib/apparmor/abstractions/pasta b/contrib/apparmor/abstractions/pasta new file mode 100644 index 0000000..7d802ac --- /dev/null +++ b/contrib/apparmor/abstractions/pasta @@ -0,0 +1,35 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# +# PASST - Plug A Simple Socket Transport +# for qemu/UNIX domain socket mode +# +# PASTA - Pack A Subtle Tap Abstraction +# for network namespace/tap device mode +# +# contrib/apparmor/abstractions/pasta - Abstraction for pasta(1) +# +# Copyright (c) 2022 Red Hat GmbH +# Author: Stefano Brivio <sbrivio(a)redhat.com> + + abi <abi/3.0>, + + include <abstractions/passt> + + @{PROC}/net/tcp r, # procfs_scan_listen(), util.c + @{PROC}/net/tcp6 r, + @{PROC}/net/udp r, + @{PROC}/net/udp6 r, + + @{run}/user/@{uid}/netns/* r, # pasta_open_ns(), pasta.c + + @{PROC}/[0-9]*/ns/net r, # pasta_wait_for_ns(), + @{PROC}/[0-9]*/ns/user r, # conf_pasta_ns() + + /dev/net/tun rw, # tap_ns_tun(), tap.c + + owner @{PROC}/@{pid}/gid_map w, # pasta_start_ns(), conf_ugid() + owner @{PROC}/@{pid}/setgroups w, + owner @{PROC}/@{pid}/uid_map rw, + + owner @{PROC}/sys/net/ipv4/ping_group_range w, # pasta_spawn_cmd(), pasta.c + /{usr/,}bin/** Ux, diff --git a/contrib/apparmor/usr.bin.passt b/contrib/apparmor/usr.bin.passt index 96b61ef..7433d01 100644 --- a/contrib/apparmor/usr.bin.passt +++ b/contrib/apparmor/usr.bin.passt @@ -16,73 +16,20 @@ abi <abi/3.0>, include <tunables/global> profile passt /usr/bin/passt{,.avx2} flags=(attach_disconnected) { - ### Common rules for passt and pasta - - include <abstractions/base> - - # Alternatively: include <abstractions/nameservice> - @{etc_ro}/resolv.conf r, # get_dns(), conf.c - - capability net_bind_service, # isolation.c, conf.c - capability setuid, - capability setgid, - capability sys_admin, - capability setpcap, - capability net_admin, - capability sys_ptrace, - - / r, # isolate_prefork(), isolation.c - mount "" -> "/", - mount "" -> "/tmp/", - pivot_root "/tmp/" -> "/tmp/", - umount "/", - - network netlink raw, # nl_sock_init_do(), netlink.c - - network inet stream, # tcp.c - network inet6 stream, - - network inet dgram, # udp.c - network inet6 dgram, - - network unix stream, # tap.c - - network unix dgram, # __openlog(), log.c - - # Alternatively: include <abstractions/user-tmp> - owner /tmp/** w, # tap_sock_unix_init(), pcap(), - # write_pidfile(), - # logfile_init() - - owner @{HOME}/** w, # pcap(), write_pidfile() - - /usr/bin/passt.avx2 ix, # arch_avx2_exec(), arch.c - - - ### Rules for pasta - ### ### TODO: AppArmor doesn't give us the chance to attach a separate profile ### depending on the executable symlink. That's possible with SELinux. Two ### alternatives: implement that in AppArmor, or consider aa_change_hat(2). ### With this, rules for passt(1) could be restricted significantly. Note that ### the attach_disconnected flag is not needed for passt(1). - @{PROC}/net/tcp r, # procfs_scan_listen(), util.c - @{PROC}/net/tcp6 r, - @{PROC}/net/udp r, - @{PROC}/net/udp6 r, - - @{run}/user/@{uid}/netns/* r, # pasta_open_ns(), pasta.c + include <abstractions/passt> - @{PROC}/[0-9]*/ns/net r, # pasta_wait_for_ns(), - @{PROC}/[0-9]*/ns/user r, # conf_pasta_ns() - - /dev/net/tun rw, # tap_ns_tun(), tap.c + # Alternatively: include <abstractions/user-tmp> + owner /tmp/** w, # tap_sock_unix_init(), pcap(), + # write_pidfile(), + # logfile_init() - owner @{PROC}/@{pid}/gid_map w, # pasta_start_ns(), conf_ugid() - owner @{PROC}/@{pid}/setgroups w, - owner @{PROC}/@{pid}/uid_map rw, + owner @{HOME}/** w, # pcap(), write_pidfile() - owner @{PROC}/sys/net/ipv4/ping_group_range w, # pasta_spawn_cmd(), pasta.c - /{usr/,}bin/** Ux, + include <abstractions/pasta> } -- 2.39.1

1 0

[PATCH 0/3] Allow non-127.0.0.1 loopback address as host resolver
by Stefano Brivio 27 Feb '23

27 Feb '23

This series fixes an issue with DNS resolution reported by Andrea: if both guest and host use systemd-resolved, reachable at 127.0.0.53, we need to advertise a reachable address as DNS to the guest, which we already do, but also to map UDP packets for that address and port 53 to the original host resolver address. Otherwise, DNS resolution doesn't work at all in the guest, with default options. Stefano Brivio (3): udp: Actually use host resolver to forward DNS queries conf: Split add_dns{4,6}() out of get_dns() conf, udp: Allow any loopback address to be used as resolver conf.c | 92 ++++++++++++++++++++++++++++++++++++---------------------- udp.c | 20 ++++++------- 2 files changed, 67 insertions(+), 45 deletions(-) -- 2.39.1

3 7

[PATCH] log, conf, tap: Define die() as err() plus exit(), drop cppcheck workarounds
by Stefano Brivio 27 Feb '23

27 Feb '23

If we define die() as a variadic macro, passing __VA_ARGS__ to err(), and calling exit() outside err() itself, we can drop the workarounds introduced in commit 36f0199f6ef4 ("conf, tap: Silence two false positive invalidFunctionArg from cppcheck"). Suggested-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 3 --- log.c | 14 +++++--------- log.h | 7 ++++++- tap.c | 6 +----- 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/conf.c b/conf.c index 37f25d6..0e512f4 100644 --- a/conf.c +++ b/conf.c @@ -1064,9 +1064,6 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) { die("Can't determine if we're in init namespace: %s", strerror(errno)); - - /* Silence cppcheck's invalidFunctionArg for 'fd' in read() */ - return; } if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) || diff --git a/log.c b/log.c index bfde6ce..d2f08fa 100644 --- a/log.c +++ b/log.c @@ -46,7 +46,7 @@ int log_trace; /* --trace mode enabled */ #define BEFORE_DAEMON (setlogmask(0) == LOG_MASK(LOG_EMERG)) -#define logfn(name, level, doexit) \ +#define logfn(name, level) \ void name(const char *format, ...) { \ struct timespec tp; \ va_list args; \ @@ -75,9 +75,6 @@ void name(const char *format, ...) { \ if (format[strlen(format)] != '\n') \ fprintf(stderr, "\n"); \ } \ - \ - if (doexit) \ - exit(EXIT_FAILURE); \ } /* Prefixes for log file messages, indexed by priority */ @@ -90,11 +87,10 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ }; -logfn(die, LOG_ERR, 1) -logfn(err, LOG_ERR, 0) -logfn(warn, LOG_WARNING, 0) -logfn(info, LOG_INFO, 0) -logfn(debug,LOG_DEBUG, 0) +logfn(err, LOG_ERR) +logfn(warn, LOG_WARNING) +logfn(info, LOG_INFO) +logfn(debug,LOG_DEBUG) /** * trace_init() - Set log_trace depending on trace (debug) mode diff --git a/log.h b/log.h index d4e9d85..d4ea141 100644 --- a/log.h +++ b/log.h @@ -10,12 +10,17 @@ #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE)) -void die(const char *format, ...); void err(const char *format, ...); void warn(const char *format, ...); void info(const char *format, ...); void debug(const char *format, ...); +#define die(...) \ + do { \ + err(__VA_ARGS__); \ + exit(EXIT_FAILURE); \ + } while (0) + extern int log_trace; void trace_init(int enable); #define trace(...) \ diff --git a/tap.c b/tap.c index d6f962e..88eed88 100644 --- a/tap.c +++ b/tap.c @@ -1037,13 +1037,9 @@ static void tap_sock_unix_init(struct ctx *c) snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0); - if (ex < 0) { + if (ex < 0) die("UNIX domain socket check: %s", strerror(errno)); - /* Silence cppcheck's invalidFunctionArg for 'ex' */ - return; - } - ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); if (!ret || (errno != ENOENT && errno != ECONNREFUSED && errno != EACCES)) { -- 2.39.1

2 1

[PATCH] doc/demo: Fix and suppress ShellCheck warnings
by Stefano Brivio 27 Feb '23

27 Feb '23

ShellCheck reports (SC2034) that __qemu_arch is not used. Use it, and silence the resulting SC2086 warning as we want word splitting on options we pass with it. While at it, silence SC2317 warnings for commands in cleanup() that appear to be unreachable: cleanup() is only called as trap. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- doc/demo.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/demo.sh b/doc/demo.sh index ed71ffb..5b05ddf 100755 --- a/doc/demo.sh +++ b/doc/demo.sh @@ -110,6 +110,7 @@ next() { } # cleanup() - Terminate pasta and passt, clean up, restore TTY settings +# shellcheck disable=SC2317 cleanup() { [ -f "${DEMO_DIR}/pasta.pid" ] && kill "$(cat "${DEMO_DIR}/pasta.pid")" [ -f "${DEMO_DIR}/passt.pid" ] && kill "$(cat "${DEMO_DIR}/passt.pid")" @@ -223,7 +224,8 @@ into_ns() { echo "Use ^C to terminate it." next - cmd qrap 5 qemu-system-x86_64 -M pc,accel=kvm:tcg \ + # shellcheck disable=SC2086 + cmd qrap 5 ${__qemu_arch} \ -smp "$(nproc)" -m 1024 \ -nographic -serial stdio -nodefaults -no-reboot -vga none \ -initrd "${DEMO_DIR}/demo.img" \ -- 2.39.1

2 1

[PATCH] Fix definitions of SOCKET_MAX, TCP_MAX_CONNS
by Stefano Brivio 27 Feb '23

27 Feb '23

...and, given that I keep getting this wrong, add a convenience macro, MAX_FROM_BITS(). Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.h | 4 ++-- tcp.h | 4 ++-- tcp_conn.h | 2 +- util.h | 2 ++ 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/passt.h b/passt.h index 3d7e567..e0383eb 100644 --- a/passt.h +++ b/passt.h @@ -42,7 +42,7 @@ union epoll_ref; /** * union epoll_ref - Breakdown of reference for epoll socket bookkeeping * @proto: IP protocol number - * @s: Socket number (implies 2^24 limit on number of descriptors) + * @s: Socket number (implies 2^24-1 limit on number of descriptors) * @tcp: TCP-specific reference part * @udp: UDP-specific reference part * @icmp: ICMP-specific reference part @@ -53,7 +53,7 @@ union epoll_ref { struct { int32_t proto:8, #define SOCKET_REF_BITS 24 -#define SOCKET_MAX (1 << SOCKET_REF_BITS) +#define SOCKET_MAX MAX_FROM_BITS(SOCKET_REF_BITS) s:SOCKET_REF_BITS; union { union tcp_epoll_ref tcp; diff --git a/tcp.h b/tcp.h index 5527c5b..36e5391 100644 --- a/tcp.h +++ b/tcp.h @@ -8,8 +8,8 @@ #define TCP_TIMER_INTERVAL 1000 /* ms */ -#define TCP_CONN_INDEX_BITS 17 /* 128k */ -#define TCP_MAX_CONNS (1 << TCP_CONN_INDEX_BITS) +#define TCP_CONN_INDEX_BITS 17 /* 128k - 1 */ +#define TCP_MAX_CONNS MAX_FROM_BITS(TCP_CONN_INDEX_BITS) struct ctx; diff --git a/tcp_conn.h b/tcp_conn.h index a499f34..c22632b 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -54,7 +54,7 @@ struct tcp_tap_conn { #define TCP_RETRANS_BITS 3 unsigned int retrans :TCP_RETRANS_BITS; -#define TCP_MAX_RETRANS ((1U << TCP_RETRANS_BITS) - 1) +#define TCP_MAX_RETRANS MAX_FROM_BITS(TCP_RETRANS_BITS) #define TCP_WS_BITS 4 /* RFC 7323 */ #define TCP_WS_MAX 14 diff --git a/util.h b/util.h index 6303c17..570094c 100644 --- a/util.h +++ b/util.h @@ -40,6 +40,8 @@ #define ROUND_DOWN(x, y) ((x) & ~((y) - 1)) #define ROUND_UP(x, y) (((x) + (y) - 1) & ~((y) - 1)) +#define MAX_FROM_BITS(n) ((int)((1U << (n)) - 1)) + #define BIT(n) (1UL << (n)) #define BITMAP_BIT(n) (BIT((n) % (sizeof(long) * 8))) #define BITMAP_WORD(n) (n / (sizeof(long) * 8)) -- 2.39.1

2 1

[PATCH 0/3] Avoid some warnings reported by Coverity
by Stefano Brivio 27 Feb '23

27 Feb '23

Stefano Brivio (3): tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls() tcp: Avoid false (but convoluted) positive Coverity CWE-476 warning tcp: Avoid (theoretical) resource leak (CWE-772) Coverity warning tcp.c | 17 ++++++++++++----- tcp_splice.c | 24 ++++++++++++++++-------- 2 files changed, 28 insertions(+), 13 deletions(-) -- 2.39.1

2 6

[PATCH] treewide: Disable gcc strict aliasing rules as needed, drop workarounds
by Stefano Brivio 27 Feb '23

27 Feb '23

Recently, commit 4ddbcb9c0c55 ("tcp: Disable optimisations for tcp_hash()") worked around yet another issue we hit with gcc 12 and '-flto -O2': some stores affecting the input data to siphash_20b() were omitted altogether, and tcp_hash() wouldn't get the correct hash for incoming connections. Digging further into this revealed that, at least according to gcc's interpretation of C99 aliasing rules, passing pointers to functions with different types compared to the effective type of the object (for example, a uint8_t pointer to an anonymous struct, as it happens in tcp_hash()), doesn't guarantee that stores are not reordered across the function call. This means that, in general, our checksum and hash functions might not see parts of input data that was intended to be provided by callers. Not even switching from uint8_t to character types, which should be appropriate here, according to C99 (ISO/IEC 9899, TC3, draft N1256), section 6.5, "Expressions", paragraph 7: An object shall have its stored value accessed only by an lvalue expression that has one of the following types: [...] — a character type. does the trick. I guess this is also subject to interpretation: casting passed pointers to character types, and then using those as different types, might still violate (dubious) aliasing rules. Disable gcc strict aliasing rules for potentially affected functions, which, in turn, disables gcc's Type-Based Alias Analysis (TBAA) optimisations based on those function arguments. Drop the existing workarounds. Also the (seemingly?) bogus 'maybe-uninitialized' warning on the tcp_tap_handler() > tcp_hash() > siphash_20b() path goes away with -fno-strict-aliasing on siphash_20b(). Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- Makefile | 21 --------------------- checksum.c | 13 ++++++++++--- siphash.c | 22 +++++++++++++++++++--- tcp.c | 6 ------ 4 files changed, 29 insertions(+), 33 deletions(-) diff --git a/Makefile b/Makefile index 080c748..667ddfb 100644 --- a/Makefile +++ b/Makefile @@ -56,27 +56,6 @@ PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \ tcp_splice.h udp.h util.h HEADERS = $(PASST_HEADERS) seccomp.h -# On gcc 11 and 12, with -O2 and -flto, tcp_hash() and siphash_20b(), if -# inlined, seem to be hitting something similar to: -# 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) $(CPPFLAGS))) -ifneq (,$(filter -O2,$(FLAGS) $(CFLAGS) $(CPPFLAGS))) - FLAGS += -DTCP_HASH_NOINLINE - FLAGS += -DSIPHASH_20B_NOINLINE - FLAGS += -DCSUM_UNALIGNED_NO_IPA -endif -endif -endif - C := \#include <linux/tcp.h>\nstruct tcp_info x = { .tcpi_snd_wnd = 0 }; ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) FLAGS += -DHAS_SND_WND diff --git a/checksum.c b/checksum.c index 29769d9..9631f91 100644 --- a/checksum.c +++ b/checksum.c @@ -69,6 +69,8 @@ * * Return: 32-bit sum of 16-bit words */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint32_t sum_16b(const void *buf, size_t len) { const uint16_t *p = buf; @@ -107,9 +109,8 @@ 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 +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init) { return (uint16_t)~csum_fold(sum_16b(buf, len) + init); @@ -245,6 +246,8 @@ void csum_icmp6(struct icmp6hdr *icmp6hr, * - sum_a/sum_b unpacking is interleaved and not sequential to reduce stalls * - coding style adaptation */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ static uint32_t csum_avx2(const void *buf, size_t len, uint32_t init) { __m256i a, b, sum256, sum_a_hi, sum_a_lo, sum_b_hi, sum_b_lo, c, d; @@ -391,6 +394,8 @@ less_than_128_bytes: * * Return: 16-bit folded, complemented checksum sum */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint16_t csum(const void *buf, size_t len, uint32_t init) { return (uint16_t)~csum_fold(csum_avx2(buf, len, init)); @@ -406,6 +411,8 @@ uint16_t csum(const void *buf, size_t len, uint32_t init) * * Return: 16-bit folded, complemented checksum */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint16_t csum(const void *buf, size_t len, uint32_t init) { return csum_unaligned(buf, len, init); diff --git a/siphash.c b/siphash.c index 811918b..e8b144d 100644 --- a/siphash.c +++ b/siphash.c @@ -104,6 +104,17 @@ * * Return: the 64-bit hash output */ +/* Type-Based Alias Analysis (TBAA) optimisation in gcc 11 and 12 (-flto -O2) + * makes these functions essentially useless by allowing reordering of stores of + * input data across function calls. Not even declaring @in as char pointer is + * enough: disable gcc's interpretation of strict aliasing altogether. See also: + * + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106706 + * https://stackoverflow.com/questions/2958633/gcc-strict-aliasing-and-horror-… + * https://lore.kernel.org/all/alpine.LFD.2.00.0901121128080.6528__33422.53280… + */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* cppcheck-suppress unusedFunction */ uint64_t siphash_8b(const uint8_t *in, const uint64_t *k) { @@ -123,6 +134,8 @@ uint64_t siphash_8b(const uint8_t *in, const uint64_t *k) * * Return: 32 bits obtained by XORing the two halves of the 64-bit hash output */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ /* cppcheck-suppress unusedFunction */ uint32_t siphash_12b(const uint8_t *in, const uint64_t *k) { @@ -148,9 +161,8 @@ uint32_t siphash_12b(const uint8_t *in, const uint64_t *k) * * Return: the 64-bit hash output */ -#if SIPHASH_20B_NOINLINE -__attribute__((__noinline__)) /* See comment in Makefile */ -#endif +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint64_t siphash_20b(const uint8_t *in, const uint64_t *k) { uint32_t *in32 = (uint32_t *)in; @@ -179,6 +191,8 @@ uint64_t siphash_20b(const uint8_t *in, const uint64_t *k) * * Return: the 64-bit hash output */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ /* cppcheck-suppress unusedFunction */ uint32_t siphash_32b(const uint8_t *in, const uint64_t *k) { @@ -205,6 +219,8 @@ uint32_t siphash_32b(const uint8_t *in, const uint64_t *k) * * Return: 32 bits obtained by XORing the two halves of the 64-bit hash output */ +/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ +__attribute__((optimize("-fno-strict-aliasing"))) /* See siphash_8b() */ uint32_t siphash_36b(const uint8_t *in, const uint64_t *k) { uint32_t *in32 = (uint32_t *)in; diff --git a/tcp.c b/tcp.c index 21c319d..cbd537e 100644 --- a/tcp.c +++ b/tcp.c @@ -1182,12 +1182,6 @@ static int tcp_hash_match(const struct tcp_tap_conn *conn, * * Return: hash value, already modulo size of the hash table */ -#if TCP_HASH_NOINLINE -__attribute__((__noinline__)) /* See comment in Makefile */ -#endif -__attribute__((optimize("O0"))) /* TODO: with -O2 and -flto on gcc 12.2, - * siphash_20b() doesn't see 'addr', why? - */ static unsigned int tcp_hash(const struct ctx *c, const union inany_addr *addr, in_port_t tap_port, in_port_t sock_port) { -- 2.39.1

2 1