[PATCH] Makefile: Hack for optimised-away store in ndp() before checksum calculation

[PATCH] Makefile: Hack for optimised-away store in ndp() before checksum calculation

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

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);
-- 
@@ -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