[PATCH 0/8] Avoid running cppcheck on system headers

[PATCH 0/8] Avoid running cppcheck on system headers

[David Gibson]

It turns out cppcheck has inbuilt knowledge of the C library, and
isn't typically given the system headers.  Avoiding doing so reduces
the runtime to less than half of what it currently is.

For non-obvious reasons, this change also exposes some new warnings.
Some are real, one is a cppcheck bug.  Fix and/or workaround these
then make the change to the cppcheck options.

This is based on my earlier series with clangd configuration and
fixes.

David Gibson (8):
  linux_dep: Generalise tcp_info.h to handling Linux extension
    compatibility
  log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime
  linux_dep: Move close_range() conditional handling to linux_dep.h
  linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling
  ndp: Use const pointer for ndp_ns packet
  udp: Don't dereference uflow before NULL check in
    udp_reply_sock_handler()
  util: Work around cppcheck bug 6936
  cppcheck: Don't check the system headers

 Makefile                  | 17 ++---------------
 tcp_info.h => linux_dep.h | 32 ++++++++++++++++++++++++++++----
 log.c                     |  9 +--------
 ndp.c                     |  4 ++--
 tcp.c                     |  2 +-
 udp.c                     |  5 +++--
 util.h                    | 29 ++++++++++-------------------
 7 files changed, 47 insertions(+), 51 deletions(-)
 rename tcp_info.h => linux_dep.h (85%)

-- 
2.47.0

[PATCH 1/8] linux_dep: Generalise tcp_info.h to handling Linux extension compatibility

[David Gibson]

tcp_info.h exists just to contain a modern enough version of struct
tcp_info for our needs, removing compile time dependency on the version of
kernel headers.  There are several other cases where we can remove similar
compile time dependencies on kernel version.  Prepare for that by renaming
tcp_info.h to linux_dep.h.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp_info.h => linux_dep.h | 10 ++++++----
 tcp.c                     |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)
 rename tcp_info.h => linux_dep.h (97%)

diff --git a/tcp_info.h b/linux_dep.h
similarity index 97%
rename from tcp_info.h
rename to linux_dep.h
index 06ccb16..8921623 100644
--- a/tcp_info.h
+++ b/linux_dep.h
@@ -1,13 +1,15 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later
  * Copyright Red Hat
  *
- * Largely derived from include/linux/tcp.h in the Linux kernel
+ * Declarations for Linux specific dependencies
  */
 
-#ifndef TCP_INFO_H
-#define TCP_INFO_H
+#ifndef LINUX_DEP_H
+#define LINUX_DEP_H
 
 /* struct tcp_info_linux - Information from Linux TCP_INFO getsockopt()
+ *
+ * Largely derived from include/linux/tcp.h in the Linux kernel
  *
  * Some fields returned by TCP_INFO have been there for ages and are shared with
  * BSD.  struct tcp_info from netinet/tcp.h has only those fields.  There are
@@ -117,4 +119,4 @@ struct tcp_info_linux {
 						 */
 };
 
-#endif /* TCP_INFO_H */
+#endif /* LINUX_DEP_H */
diff --git a/tcp.c b/tcp.c
index 56ceba6..1bb122b 100644
--- a/tcp.c
+++ b/tcp.c
@@ -299,10 +299,10 @@
 #include "log.h"
 #include "inany.h"
 #include "flow.h"
+#include "linux_dep.h"
 
 #include "flow_table.h"
 #include "tcp_internal.h"
-#include "tcp_info.h"
 #include "tcp_buf.h"
 
 /* MSS rounding: see SET_MSS() */
-- 
@@ -299,10 +299,10 @@
 #include "log.h"
 #include "inany.h"
 #include "flow.h"
+#include "linux_dep.h"
 
 #include "flow_table.h"
 #include "tcp_internal.h"
-#include "tcp_info.h"
 #include "tcp_buf.h"
 
 /* MSS rounding: see SET_MSS() */
-- 
2.47.0

[PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

[David Gibson]

log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt
to use it if not defined.  But even if the value is defined at compile
time, it might not be available in the runtime kernel, so we need to check
for errors from a fallocate() call and fall back to other methods.

Simplify this to only need the runtime check by using linux_dep.h to define
FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile    | 5 -----
 linux_dep.h | 6 ++++++
 log.c       | 9 +--------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 56bf2e8..cb91535 100644
--- a/Makefile
+++ b/Makefile
@@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec
 	FLAGS += -fstack-protector-strong
 endif
 
-C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nint x = FALLOC_FL_COLLAPSE_RANGE;
-ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
-	EXTRA_SYSCALLS += fallocate
-endif
-
 prefix		?= /usr/local
 exec_prefix	?= $(prefix)
 bindir		?= $(exec_prefix)/bin
diff --git a/linux_dep.h b/linux_dep.h
index 8921623..eae9c3c 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -119,4 +119,10 @@ struct tcp_info_linux {
 						 */
 };
 
+#include <linux/falloc.h>
+
+#ifndef FALLOC_FL_COLLAPSE_RANGE
+#define FALLOC_FL_COLLAPSE_RANGE	0x08
+#endif
+
 #endif /* LINUX_DEP_H */
diff --git a/log.c b/log.c
index 19f1d98..3c1b39c 100644
--- a/log.c
+++ b/log.c
@@ -92,7 +92,6 @@ const char *logfile_prefix[] = {
 	"         ",		/* LOG_DEBUG */
 };
 
-#ifdef FALLOC_FL_COLLAPSE_RANGE
 /**
  * logfile_rotate_fallocate() - Write header, set log_written after fallocate()
  * @fd:		Log file descriptor
@@ -126,7 +125,6 @@ static void logfile_rotate_fallocate(int fd, const struct timespec *now)
 
 	log_written -= log_cut_size;
 }
-#endif /* FALLOC_FL_COLLAPSE_RANGE */
 
 /**
  * logfile_rotate_move() - Fallback: move recent entries toward start, then cut
@@ -198,21 +196,17 @@ out:
  *
  * Return: 0 on success, negative error code on failure
  *
- * #syscalls fcntl
- *
- * fallocate() passed as EXTRA_SYSCALL only if FALLOC_FL_COLLAPSE_RANGE is there
+ * #syscalls fcntl fallocate
  */
 static int logfile_rotate(int fd, const struct timespec *now)
 {
 	if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */))
 		return -errno;
 
-#ifdef FALLOC_FL_COLLAPSE_RANGE
 	/* Only for Linux >= 3.15, extent-based ext4 or XFS, glibc >= 2.18 */
 	if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size))
 		logfile_rotate_fallocate(fd, now);
 	else
-#endif
 		logfile_rotate_move(fd, now);
 
 	if (fcntl(fd, F_SETFL, O_RDWR | O_APPEND))
@@ -432,4 +426,3 @@ void logfile_init(const char *name, const char *path, size_t size)
 	/* For FALLOC_FL_COLLAPSE_RANGE: VFS block size can be up to one page */
 	log_cut_size = ROUND_UP(log_size * LOGFILE_CUT_RATIO / 100, PAGE_SIZE);
 }
-
-- 
2.47.0

[PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

[David Gibson]

util.h has some #ifdefs and weak definitions to handle compatibility with
various kernel versions.  Move this to linux_dep.h which handles several
other similar cases.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 linux_dep.h | 20 ++++++++++++++++++++
 util.h      | 19 -------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h
index eae9c3c..3a41e42 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -125,4 +125,24 @@ struct tcp_info_linux {
 #define FALLOC_FL_COLLAPSE_RANGE	0x08
 #endif
 
+#include <linux/close_range.h>
+
+#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
+/* glibc < 2.34 and musl as of 1.2.5 need these */
+#ifndef SYS_close_range
+#define SYS_close_range		436
+#endif
+__attribute__ ((weak))
+/* cppcheck-suppress funcArgNamesDifferent */
+int close_range(unsigned int first, unsigned int last, int flags) {
+	return syscall(SYS_close_range, first, last, flags);
+}
+#else
+/* No reasonable fallback option */
+/* cppcheck-suppress funcArgNamesDifferent */
+int close_range(unsigned int first, unsigned int last, int flags) {
+	return 0;
+}
+#endif
+
 #endif /* LINUX_DEP_H */
diff --git a/util.h b/util.h
index 2858b10..fdc3af8 100644
--- a/util.h
+++ b/util.h
@@ -17,7 +17,6 @@
 #include <arpa/inet.h>
 #include <unistd.h>
 #include <sys/syscall.h>
-#include <linux/close_range.h>
 
 #include "log.h"
 
@@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 
 struct ctx;
 
-#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
-/* glibc < 2.34 and musl as of 1.2.5 need these */
-#ifndef SYS_close_range
-#define SYS_close_range		436
-#endif
-__attribute__ ((weak))
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return syscall(SYS_close_range, first, last, flags);
-}
-#else
-/* No reasonable fallback option */
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return 0;
-}
-#endif
-
 int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 	       const void *sa, socklen_t sl,
 	       const char *ifname, bool v6only, uint32_t data);
-- 
@@ -17,7 +17,6 @@
 #include <arpa/inet.h>
 #include <unistd.h>
 #include <sys/syscall.h>
-#include <linux/close_range.h>
 
 #include "log.h"
 
@@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 
 struct ctx;
 
-#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
-/* glibc < 2.34 and musl as of 1.2.5 need these */
-#ifndef SYS_close_range
-#define SYS_close_range		436
-#endif
-__attribute__ ((weak))
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return syscall(SYS_close_range, first, last, flags);
-}
-#else
-/* No reasonable fallback option */
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return 0;
-}
-#endif
-
 int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 	       const void *sa, socklen_t sl,
 	       const char *ifname, bool v6only, uint32_t data);
-- 
2.47.0

[PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

[David Gibson]

If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of
close_range() which is a (successful) no-op.  This is broken in several
ways:
 * It doesn't actually fix compile if using old kernel headers, because
   the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE
   unprotected by ifdefs
 * Even if it did fix the compile, it means inconsistent behaviour between
   a compile time failure to find the value (we silently don't close files)
   and a runtime failure (we die with an error from close_range())
 * Silently not closing the files we intend to close for security reasons
   is probably not a good idea in any case

As bonus this fixes a cppcheck error I see with some different options I'm
looking to apply in future.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 linux_dep.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h
index 3a41e42..240f50a 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -127,22 +127,18 @@ struct tcp_info_linux {
 
 #include <linux/close_range.h>
 
-#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
 /* glibc < 2.34 and musl as of 1.2.5 need these */
 #ifndef SYS_close_range
 #define SYS_close_range		436
 #endif
+#ifndef CLOSE_RANGE_UNSHARE	/* Linux kernel < 5.9 */
+#define CLOSE_RANGE_UNSHARE	(1U << 1)
+#endif
+
 __attribute__ ((weak))
 /* cppcheck-suppress funcArgNamesDifferent */
 int close_range(unsigned int first, unsigned int last, int flags) {
 	return syscall(SYS_close_range, first, last, flags);
 }
-#else
-/* No reasonable fallback option */
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return 0;
-}
-#endif
 
 #endif /* LINUX_DEP_H */
-- 
@@ -127,22 +127,18 @@ struct tcp_info_linux {
 
 #include <linux/close_range.h>
 
-#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
 /* glibc < 2.34 and musl as of 1.2.5 need these */
 #ifndef SYS_close_range
 #define SYS_close_range		436
 #endif
+#ifndef CLOSE_RANGE_UNSHARE	/* Linux kernel < 5.9 */
+#define CLOSE_RANGE_UNSHARE	(1U << 1)
+#endif
+
 __attribute__ ((weak))
 /* cppcheck-suppress funcArgNamesDifferent */
 int close_range(unsigned int first, unsigned int last, int flags) {
 	return syscall(SYS_close_range, first, last, flags);
 }
-#else
-/* No reasonable fallback option */
-/* cppcheck-suppress funcArgNamesDifferent */
-int close_range(unsigned int first, unsigned int last, int flags) {
-	return 0;
-}
-#endif
 
 #endif /* LINUX_DEP_H */
-- 
2.47.0

[PATCH 5/8] ndp: Use const pointer for ndp_ns packet

[David Gibson]

We don't modify this structure at all.  For some reason cppcheck doesn't
catch this with our current options, but did when I was experimenting with
some different options.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ndp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ndp.c b/ndp.c
index a1ee834..faae408 100644
--- a/ndp.c
+++ b/ndp.c
@@ -234,8 +234,8 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr,
 		return 1;
 
 	if (ih->icmp6_type == NS) {
-		struct ndp_ns *ns = packet_get(p, 0, 0, sizeof(struct ndp_ns),
-					       NULL);
+		const struct ndp_ns *ns =
+			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
 
 		if (!ns)
 			return -1;
-- 
@@ -234,8 +234,8 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr,
 		return 1;
 
 	if (ih->icmp6_type == NS) {
-		struct ndp_ns *ns = packet_get(p, 0, 0, sizeof(struct ndp_ns),
-					       NULL);
+		const struct ndp_ns *ns =
+			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
 
 		if (!ns)
 			return -1;
-- 
2.47.0

[PATCH 6/8] udp: Don't dereference uflow before NULL check in udp_reply_sock_handler()

[David Gibson]

We have an ASSERT() verifying that we're able to look up the flow in
udp_reply_sock_handler().  However, we dereference uflow before that in
an initializer, rather defeating the point.  Rearrange to avoid that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/udp.c b/udp.c
index 0c01067..4be165f 100644
--- a/udp.c
+++ b/udp.c
@@ -644,12 +644,13 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref,
 	flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside);
 	const struct flowside *toside = flowside_at_sidx(tosidx);
 	struct udp_flow *uflow = udp_at_sidx(ref.flowside);
-	int from_s = uflow->s[ref.flowside.sidei];
 	uint8_t topif = pif_at_sidx(tosidx);
-	int n, i;
+	int n, i, from_s;
 
 	ASSERT(!c->no_udp && uflow);
 
+	from_s = uflow->s[ref.flowside.sidei];
+
 	if (udp_sock_errs(c, from_s, events) < 0) {
 		flow_err(uflow, "Unrecoverable error on reply socket");
 		flow_err_details(uflow);
-- 
@@ -644,12 +644,13 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref,
 	flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside);
 	const struct flowside *toside = flowside_at_sidx(tosidx);
 	struct udp_flow *uflow = udp_at_sidx(ref.flowside);
-	int from_s = uflow->s[ref.flowside.sidei];
 	uint8_t topif = pif_at_sidx(tosidx);
-	int n, i;
+	int n, i, from_s;
 
 	ASSERT(!c->no_udp && uflow);
 
+	from_s = uflow->s[ref.flowside.sidei];
+
 	if (udp_sock_errs(c, from_s, events) < 0) {
 		flow_err(uflow, "Unrecoverable error on reply socket");
 		flow_err_details(uflow);
-- 
2.47.0

[PATCH 7/8] util: Work around cppcheck bug 6936

[David Gibson]

While experimenting with cppcheck options, I hit several false positives
caused by this bug: https://trac.cppcheck.net/ticket/13227

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile |  2 +-
 util.h   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index cb91535..0ba85b4 100644
--- a/Makefile
+++ b/Makefile
@@ -183,5 +183,5 @@ cppcheck: $(PASST_SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
 	--suppress=unusedStructMember					\
-	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS))			\
+	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936  \
 	$(PASST_SRCS) $(HEADERS)
diff --git a/util.h b/util.h
index fdc3af8..1a4dfd4 100644
--- a/util.h
+++ b/util.h
@@ -67,6 +67,15 @@
 #define STRINGIFY(x)	#x
 #define STR(x)		STRINGIFY(x)
 
+#ifdef CPPCHECK_6936
+/* Some cppcheck versions get confused by aborts inside a loop, causing
+ * it to give false positive uninitialised variable warnings later in
+ * the function, because it doesn't realise the non-initialising path
+ * already exited.  See https://trac.cppcheck.net/ticket/13227
+ */
+#define ASSERT(expr)		\
+	((expr) ? (void)0 : abort())
+#else
 #define ASSERT(expr)							\
 	do {								\
 		if (!(expr)) {						\
@@ -78,6 +87,7 @@
 			abort();					\
 		}							\
 	} while (0)
+#endif
 
 #ifdef P_tmpdir
 #define TMPDIR		P_tmpdir
-- 
@@ -67,6 +67,15 @@
 #define STRINGIFY(x)	#x
 #define STR(x)		STRINGIFY(x)
 
+#ifdef CPPCHECK_6936
+/* Some cppcheck versions get confused by aborts inside a loop, causing
+ * it to give false positive uninitialised variable warnings later in
+ * the function, because it doesn't realise the non-initialising path
+ * already exited.  See https://trac.cppcheck.net/ticket/13227
+ */
+#define ASSERT(expr)		\
+	((expr) ? (void)0 : abort())
+#else
 #define ASSERT(expr)							\
 	do {								\
 		if (!(expr)) {						\
@@ -78,6 +87,7 @@
 			abort();					\
 		}							\
 	} while (0)
+#endif
 
 #ifdef P_tmpdir
 #define TMPDIR		P_tmpdir
-- 
2.47.0

[PATCH 8/8] cppcheck: Don't check the system headers

[David Gibson]

We pass -I options to cppcheck so that it will find the system headers.
Then we need to pass a bunch more options to suppress the zillions of
cppcheck errors found in those headers.

It turns out, however, that it's not recommended to give the system headers
to cppcheck anyway.  Instead it has built-in knowledge of the ANSI libc and
uses that as the basis of its checks.  We do need to suppress
missingIncludeSystem warnings instead though.

Not bothering with the system headers makes the cppcheck runtime go from
~37s to ~14s on my machine, which is a pretty nice win.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 0ba85b4..258d298 100644
--- a/Makefile
+++ b/Makefile
@@ -163,11 +163,6 @@ clang-tidy: $(PASST_SRCS) $(HEADERS)
 	clang-tidy $(PASST_SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
 	           -DCLANG_TIDY_58992
 
-SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
-ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
-VER := $(shell $(CC) -dumpversion)
-SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include
-endif
 cppcheck: $(PASST_SRCS) $(HEADERS)
 	if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \
 		CPPCHECK_EXHAUSTIVE="--check-level=exhaustive";		\
@@ -177,11 +172,8 @@ cppcheck: $(PASST_SRCS) $(HEADERS)
 	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
 	--inconclusive --library=posix --quiet				\
 	$${CPPCHECK_EXHAUSTIVE}						\
-	$(SYSTEM_INCLUDES:%=-I%)					\
-	$(SYSTEM_INCLUDES:%=--config-exclude=%)				\
-	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
-	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
+	--suppress=missingIncludeSystem \
 	--suppress=unusedStructMember					\
 	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936  \
 	$(PASST_SRCS) $(HEADERS)
-- 
@@ -163,11 +163,6 @@ clang-tidy: $(PASST_SRCS) $(HEADERS)
 	clang-tidy $(PASST_SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
 	           -DCLANG_TIDY_58992
 
-SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
-ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
-VER := $(shell $(CC) -dumpversion)
-SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include
-endif
 cppcheck: $(PASST_SRCS) $(HEADERS)
 	if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \
 		CPPCHECK_EXHAUSTIVE="--check-level=exhaustive";		\
@@ -177,11 +172,8 @@ cppcheck: $(PASST_SRCS) $(HEADERS)
 	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
 	--inconclusive --library=posix --quiet				\
 	$${CPPCHECK_EXHAUSTIVE}						\
-	$(SYSTEM_INCLUDES:%=-I%)					\
-	$(SYSTEM_INCLUDES:%=--config-exclude=%)				\
-	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
-	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
+	--suppress=missingIncludeSystem \
 	--suppress=unusedStructMember					\
 	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936  \
 	$(PASST_SRCS) $(HEADERS)
-- 
2.47.0

Re: [PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

[Stefano Brivio]

On Wed,  6 Nov 2024 17:54:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt
> to use it if not defined.  But even if the value is defined at compile
> time, it might not be available in the runtime kernel, so we need to check
> for errors from a fallocate() call and fall back to other methods.
> 
> Simplify this to only need the runtime check by using linux_dep.h to define
> FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile    | 5 -----
>  linux_dep.h | 6 ++++++
>  log.c       | 9 +--------
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 56bf2e8..cb91535 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec
>  	FLAGS += -fstack-protector-strong
>  endif
>  
> -C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nint x = FALLOC_FL_COLLAPSE_RANGE;
> -ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> -	EXTRA_SYSCALLS += fallocate
> -endif
> -
>  prefix		?= /usr/local
>  exec_prefix	?= $(prefix)
>  bindir		?= $(exec_prefix)/bin
> diff --git a/linux_dep.h b/linux_dep.h
> index 8921623..eae9c3c 100644
> --- a/linux_dep.h
> +++ b/linux_dep.h
> @@ -119,4 +119,10 @@ struct tcp_info_linux {
>  						 */
>  };
>  
> +#include <linux/falloc.h>
> +
> +#ifndef FALLOC_FL_COLLAPSE_RANGE
> +#define FALLOC_FL_COLLAPSE_RANGE	0x08
> +#endif
> +
>  #endif /* LINUX_DEP_H */
> diff --git a/log.c b/log.c
> index 19f1d98..3c1b39c 100644
> --- a/log.c
> +++ b/log.c
> @@ -92,7 +92,6 @@ const char *logfile_prefix[] = {
>  	"         ",		/* LOG_DEBUG */
>  };
>  
> -#ifdef FALLOC_FL_COLLAPSE_RANGE

This breaks the build on Alpine (and I suppose on Void Linux too, that
is, whenever we build against musl):

log.c: In function 'logfile_rotate':
log.c:207:28: error: 'FALLOC_FL_COLLAPSE_RANGE' undeclared (first use in this function)
  207 |         if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size))
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~
log.c:207:28: note: each undeclared identifier is reported only once for each function it appears in

and it's fixed by including linux_dep.h from log.c.

-- 
Stefano

Re: [PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

[Stefano Brivio]

On Wed,  6 Nov 2024 17:54:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> util.h has some #ifdefs and weak definitions to handle compatibility with
> various kernel versions.  Move this to linux_dep.h which handles several
> other similar cases.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  linux_dep.h | 20 ++++++++++++++++++++
>  util.h      | 19 -------------------
>  2 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/linux_dep.h b/linux_dep.h
> index eae9c3c..3a41e42 100644
> --- a/linux_dep.h
> +++ b/linux_dep.h
> @@ -125,4 +125,24 @@ struct tcp_info_linux {
>  #define FALLOC_FL_COLLAPSE_RANGE	0x08
>  #endif
>  
> +#include <linux/close_range.h>
> +
> +#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
> +/* glibc < 2.34 and musl as of 1.2.5 need these */
> +#ifndef SYS_close_range
> +#define SYS_close_range		436
> +#endif
> +__attribute__ ((weak))
> +/* cppcheck-suppress funcArgNamesDifferent */
> +int close_range(unsigned int first, unsigned int last, int flags) {
> +	return syscall(SYS_close_range, first, last, flags);
> +}
> +#else
> +/* No reasonable fallback option */
> +/* cppcheck-suppress funcArgNamesDifferent */
> +int close_range(unsigned int first, unsigned int last, int flags) {
> +	return 0;
> +}
> +#endif
> +
>  #endif /* LINUX_DEP_H */
> diff --git a/util.h b/util.h
> index 2858b10..fdc3af8 100644
> --- a/util.h
> +++ b/util.h
> @@ -17,7 +17,6 @@
>  #include <arpa/inet.h>
>  #include <unistd.h>
>  #include <sys/syscall.h>
> -#include <linux/close_range.h>
>  
>  #include "log.h"
>  
> @@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>  
>  struct ctx;
>  
> -#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
> -/* glibc < 2.34 and musl as of 1.2.5 need these */
> -#ifndef SYS_close_range
> -#define SYS_close_range		436
> -#endif
> -__attribute__ ((weak))
> -/* cppcheck-suppress funcArgNamesDifferent */
> -int close_range(unsigned int first, unsigned int last, int flags) {
> -	return syscall(SYS_close_range, first, last, flags);
> -}
> -#else
> -/* No reasonable fallback option */
> -/* cppcheck-suppress funcArgNamesDifferent */
> -int close_range(unsigned int first, unsigned int last, int flags) {
> -	return 0;
> -}
> -#endif
> -

This breaks the build on Alpine as well:

util.c: In function 'close_open_files':
util.c:729:22: error: implicit declaration of function 'close_range'; did you mean 'SYS_close_range'? [-Wimplicit-function-declaration]
  729 |                 rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
      |                      ^~~~~~~~~~~
      |                      SYS_close_range
util.c:729:58: error: 'CLOSE_RANGE_UNSHARE' undeclared (first use in this function)
  729 |                 rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
      |                                                          ^~~~~~~~~~~~~~~~~~~

and is fixed by including "linux_dep.h" from util.c.

-- 
Stefano

Re: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

[Stefano Brivio]

On Wed,  6 Nov 2024 17:54:17 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of
> close_range() which is a (successful) no-op.  This is broken in several
> ways:
>  * It doesn't actually fix compile if using old kernel headers, because
>    the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE
>    unprotected by ifdefs

For users using distribution packages, that is, pretty much everybody
not developing passt, this is generally not a concern, because build
environments typically ship kernel headers matching the C library and
the distribution version at hand.

>  * Even if it did fix the compile, it means inconsistent behaviour between
>    a compile time failure to find the value (we silently don't close files)
>    and a runtime failure (we die with an error from close_range())

Given that this is mostly relevant for stuff built against musl (and
running on a musl-based distribution), that's not really a problem in
practice. See 6e9ecf57410b ("util: Provide own version of close_range(),
and no-op fallback").

But sure, I'm fine with these changes in general, as they're strictly
speaking more correct than the current behaviour, minus the next point.

>  * Silently not closing the files we intend to close for security reasons
>    is probably not a good idea in any case

It's arguably even worse to force users to run containers or guests as
root. That's the reason for the no-op implementation.

I don't think that introducing a dependency on a >= 5.9 kernel is a
good idea.

If this bothers you or cppcheck, I'd rather call close_range()
(possibly the weakly aliased implementation), and log a warning on
ENOSYS instead of failing.

> As bonus this fixes a cppcheck error I see with some different options I'm
> looking to apply in future.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  linux_dep.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/linux_dep.h b/linux_dep.h
> index 3a41e42..240f50a 100644
> --- a/linux_dep.h
> +++ b/linux_dep.h
> @@ -127,22 +127,18 @@ struct tcp_info_linux {
>  
>  #include <linux/close_range.h>
>  
> -#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
>  /* glibc < 2.34 and musl as of 1.2.5 need these */
>  #ifndef SYS_close_range
>  #define SYS_close_range		436
>  #endif
> +#ifndef CLOSE_RANGE_UNSHARE	/* Linux kernel < 5.9 */
> +#define CLOSE_RANGE_UNSHARE	(1U << 1)
> +#endif
> +
>  __attribute__ ((weak))
>  /* cppcheck-suppress funcArgNamesDifferent */
>  int close_range(unsigned int first, unsigned int last, int flags) {
>  	return syscall(SYS_close_range, first, last, flags);
>  }
> -#else
> -/* No reasonable fallback option */
> -/* cppcheck-suppress funcArgNamesDifferent */
> -int close_range(unsigned int first, unsigned int last, int flags) {
> -	return 0;
> -}
> -#endif
>  
>  #endif /* LINUX_DEP_H */

-- 
Stefano

Re: [PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

[David Gibson]

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

On Wed, Nov 06, 2024 at 08:10:41PM +0100, Stefano Brivio wrote:
> On Wed,  6 Nov 2024 17:54:15 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt
> > to use it if not defined.  But even if the value is defined at compile
> > time, it might not be available in the runtime kernel, so we need to check
> > for errors from a fallocate() call and fall back to other methods.
> > 
> > Simplify this to only need the runtime check by using linux_dep.h to define
> > FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile    | 5 -----
> >  linux_dep.h | 6 ++++++
> >  log.c       | 9 +--------
> >  3 files changed, 7 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 56bf2e8..cb91535 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec
> >  	FLAGS += -fstack-protector-strong
> >  endif
> >  
> > -C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nint x = FALLOC_FL_COLLAPSE_RANGE;
> > -ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> > -	EXTRA_SYSCALLS += fallocate
> > -endif
> > -
> >  prefix		?= /usr/local
> >  exec_prefix	?= $(prefix)
> >  bindir		?= $(exec_prefix)/bin
> > diff --git a/linux_dep.h b/linux_dep.h
> > index 8921623..eae9c3c 100644
> > --- a/linux_dep.h
> > +++ b/linux_dep.h
> > @@ -119,4 +119,10 @@ struct tcp_info_linux {
> >  						 */
> >  };
> >  
> > +#include <linux/falloc.h>
> > +
> > +#ifndef FALLOC_FL_COLLAPSE_RANGE
> > +#define FALLOC_FL_COLLAPSE_RANGE	0x08
> > +#endif
> > +
> >  #endif /* LINUX_DEP_H */
> > diff --git a/log.c b/log.c
> > index 19f1d98..3c1b39c 100644
> > --- a/log.c
> > +++ b/log.c
> > @@ -92,7 +92,6 @@ const char *logfile_prefix[] = {
> >  	"         ",		/* LOG_DEBUG */
> >  };
> >  
> > -#ifdef FALLOC_FL_COLLAPSE_RANGE
> 
> This breaks the build on Alpine (and I suppose on Void Linux too, that
> is, whenever we build against musl):
> 
> log.c: In function 'logfile_rotate':
> log.c:207:28: error: 'FALLOC_FL_COLLAPSE_RANGE' undeclared (first use in this function)
>   207 |         if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size))
>       |                            ^~~~~~~~~~~~~~~~~~~~~~~~
> log.c:207:28: note: each undeclared identifier is reported only once for each function it appears in
> 
> and it's fixed by including linux_dep.h from log.c.

Oops, that was careless.  Fixed for the next spin.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

[David Gibson]

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

On Wed, Nov 06, 2024 at 08:10:53PM +0100, Stefano Brivio wrote:
> On Wed,  6 Nov 2024 17:54:16 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > util.h has some #ifdefs and weak definitions to handle compatibility with
> > various kernel versions.  Move this to linux_dep.h which handles several
> > other similar cases.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  linux_dep.h | 20 ++++++++++++++++++++
> >  util.h      | 19 -------------------
> >  2 files changed, 20 insertions(+), 19 deletions(-)
> > 
> > diff --git a/linux_dep.h b/linux_dep.h
> > index eae9c3c..3a41e42 100644
> > --- a/linux_dep.h
> > +++ b/linux_dep.h
> > @@ -125,4 +125,24 @@ struct tcp_info_linux {
> >  #define FALLOC_FL_COLLAPSE_RANGE	0x08
> >  #endif
> >  
> > +#include <linux/close_range.h>
> > +
> > +#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
> > +/* glibc < 2.34 and musl as of 1.2.5 need these */
> > +#ifndef SYS_close_range
> > +#define SYS_close_range		436
> > +#endif
> > +__attribute__ ((weak))
> > +/* cppcheck-suppress funcArgNamesDifferent */
> > +int close_range(unsigned int first, unsigned int last, int flags) {
> > +	return syscall(SYS_close_range, first, last, flags);
> > +}
> > +#else
> > +/* No reasonable fallback option */
> > +/* cppcheck-suppress funcArgNamesDifferent */
> > +int close_range(unsigned int first, unsigned int last, int flags) {
> > +	return 0;
> > +}
> > +#endif
> > +
> >  #endif /* LINUX_DEP_H */
> > diff --git a/util.h b/util.h
> > index 2858b10..fdc3af8 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -17,7 +17,6 @@
> >  #include <arpa/inet.h>
> >  #include <unistd.h>
> >  #include <sys/syscall.h>
> > -#include <linux/close_range.h>
> >  
> >  #include "log.h"
> >  
> > @@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
> >  
> >  struct ctx;
> >  
> > -#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
> > -/* glibc < 2.34 and musl as of 1.2.5 need these */
> > -#ifndef SYS_close_range
> > -#define SYS_close_range		436
> > -#endif
> > -__attribute__ ((weak))
> > -/* cppcheck-suppress funcArgNamesDifferent */
> > -int close_range(unsigned int first, unsigned int last, int flags) {
> > -	return syscall(SYS_close_range, first, last, flags);
> > -}
> > -#else
> > -/* No reasonable fallback option */
> > -/* cppcheck-suppress funcArgNamesDifferent */
> > -int close_range(unsigned int first, unsigned int last, int flags) {
> > -	return 0;
> > -}
> > -#endif
> > -
> 
> This breaks the build on Alpine as well:
> 
> util.c: In function 'close_open_files':
> util.c:729:22: error: implicit declaration of function 'close_range'; did you mean 'SYS_close_range'? [-Wimplicit-function-declaration]
>   729 |                 rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
>       |                      ^~~~~~~~~~~
>       |                      SYS_close_range
> util.c:729:58: error: 'CLOSE_RANGE_UNSHARE' undeclared (first use in this function)
>   729 |                 rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
>       |                                                          ^~~~~~~~~~~~~~~~~~~
> 
> and is fixed by including "linux_dep.h" from util.c.

Oops again, adjusted.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

[David Gibson]

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

On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:
> On Wed,  6 Nov 2024 17:54:17 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of
> > close_range() which is a (successful) no-op.  This is broken in several
> > ways:
> >  * It doesn't actually fix compile if using old kernel headers, because
> >    the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE
> >    unprotected by ifdefs
> 
> For users using distribution packages, that is, pretty much everybody
> not developing passt, this is generally not a concern, because build
> environments typically ship kernel headers matching the C library and
> the distribution version at hand.

Unlike some of the other things fixed recently, this one is not
related to compile time versus runtime checks.  This one is simply
broken compiling against an older kernel, regardless of the runtime
version.  Without this patch, close_open_files() directly uses
CLOSE_RANGE_UNSHARE unprotected by an #ifdef.

> >  * Even if it did fix the compile, it means inconsistent behaviour between
> >    a compile time failure to find the value (we silently don't close files)
> >    and a runtime failure (we die with an error from close_range())
> 
> Given that this is mostly relevant for stuff built against musl (and
> running on a musl-based distribution), that's not really a problem in
> practice. See 6e9ecf57410b ("util: Provide own version of close_range(),
> and no-op fallback").
> 
> But sure, I'm fine with these changes in general, as they're strictly
> speaking more correct than the current behaviour, minus the next point.
> 
> >  * Silently not closing the files we intend to close for security reasons
> >    is probably not a good idea in any case
> 
> It's arguably even worse to force users to run containers or guests as
> root. That's the reason for the no-op implementation.

Uh... what's the connection to running as root?

> I don't think that introducing a dependency on a >= 5.9 kernel is a
> good idea.

We already have a dependency on compiling against a >= 5.9 kernel, see
above.

> If this bothers you or cppcheck, I'd rather call close_range()
> (possibly the weakly aliased implementation), and log a warning on
> ENOSYS instead of failing.

We could do that.  We'd need to consider EINVAL as well as ENOSYS, for
the case that the syscall exists, but the CLOSE_RANGE_UNSHARE flag
isn't recognized.

> > As bonus this fixes a cppcheck error I see with some different options I'm
> > looking to apply in future.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  linux_dep.h | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/linux_dep.h b/linux_dep.h
> > index 3a41e42..240f50a 100644
> > --- a/linux_dep.h
> > +++ b/linux_dep.h
> > @@ -127,22 +127,18 @@ struct tcp_info_linux {
> >  
> >  #include <linux/close_range.h>
> >  
> > -#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
> >  /* glibc < 2.34 and musl as of 1.2.5 need these */
> >  #ifndef SYS_close_range
> >  #define SYS_close_range		436
> >  #endif
> > +#ifndef CLOSE_RANGE_UNSHARE	/* Linux kernel < 5.9 */
> > +#define CLOSE_RANGE_UNSHARE	(1U << 1)
> > +#endif
> > +
> >  __attribute__ ((weak))
> >  /* cppcheck-suppress funcArgNamesDifferent */
> >  int close_range(unsigned int first, unsigned int last, int flags) {
> >  	return syscall(SYS_close_range, first, last, flags);
> >  }
> > -#else
> > -/* No reasonable fallback option */
> > -/* cppcheck-suppress funcArgNamesDifferent */
> > -int close_range(unsigned int first, unsigned int last, int flags) {
> > -	return 0;
> > -}
> > -#endif
> >  
> >  #endif /* LINUX_DEP_H */
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

[Stefano Brivio]

On Thu, 7 Nov 2024 08:01:11 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:
> > On Wed,  6 Nov 2024 17:54:17 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of
> > > close_range() which is a (successful) no-op.  This is broken in several
> > > ways:
> > >  * It doesn't actually fix compile if using old kernel headers, because
> > >    the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE
> > >    unprotected by ifdefs  
> > 
> > For users using distribution packages, that is, pretty much everybody
> > not developing passt, this is generally not a concern, because build
> > environments typically ship kernel headers matching the C library and
> > the distribution version at hand.  
> 
> Unlike some of the other things fixed recently, this one is not
> related to compile time versus runtime checks.  This one is simply
> broken compiling against an older kernel, regardless of the runtime
> version.  Without this patch, close_open_files() directly uses
> CLOSE_RANGE_UNSHARE unprotected by an #ifdef.

Ah, right, it's a different case, indeed.

> > >  * Even if it did fix the compile, it means inconsistent behaviour between
> > >    a compile time failure to find the value (we silently don't close files)
> > >    and a runtime failure (we die with an error from close_range())  
> > 
> > Given that this is mostly relevant for stuff built against musl (and
> > running on a musl-based distribution), that's not really a problem in
> > practice. See 6e9ecf57410b ("util: Provide own version of close_range(),
> > and no-op fallback").
> > 
> > But sure, I'm fine with these changes in general, as they're strictly
> > speaking more correct than the current behaviour, minus the next point.
> >   
> > >  * Silently not closing the files we intend to close for security reasons
> > >    is probably not a good idea in any case  
> > 
> > It's arguably even worse to force users to run containers or guests as
> > root. That's the reason for the no-op implementation.  
> 
> Uh... what's the connection to running as root?

That you can't run pasta or passt altogether, and if you need some
features they provide, not covered by libslirp, you might as well need
to switch to run things as root.

> > I don't think that introducing a dependency on a >= 5.9 kernel is a
> > good idea.  
> 
> We already have a dependency on compiling against a >= 5.9 kernel, see
> above.

Yes, but that would be a trivial fix.

> > If this bothers you or cppcheck, I'd rather call close_range()
> > (possibly the weakly aliased implementation), and log a warning on
> > ENOSYS instead of failing.  
> 
> We could do that.  We'd need to consider EINVAL as well as ENOSYS, for
> the case that the syscall exists, but the CLOSE_RANGE_UNSHARE flag
> isn't recognized.

Right... I think we should do that.

-- 
Stefano

Re: [PATCH 0/8] Avoid running cppcheck on system headers

[Stefano Brivio]

On Wed,  6 Nov 2024 17:54:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> It turns out cppcheck has inbuilt knowledge of the C library, and
> isn't typically given the system headers.  Avoiding doing so reduces
> the runtime to less than half of what it currently is.
> 
> For non-obvious reasons, this change also exposes some new warnings.
> Some are real, one is a cppcheck bug.  Fix and/or workaround these
> then make the change to the cppcheck options.
> 
> This is based on my earlier series with clangd configuration and
> fixes.
> 
> David Gibson (8):
>   linux_dep: Generalise tcp_info.h to handling Linux extension
>     compatibility
>   log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime
>   linux_dep: Move close_range() conditional handling to linux_dep.h
>   linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling
>   ndp: Use const pointer for ndp_ns packet
>   udp: Don't dereference uflow before NULL check in
>     udp_reply_sock_handler()
>   util: Work around cppcheck bug 6936
>   cppcheck: Don't check the system headers

Applied, except for 2/8, 3/8, 4/8, and 8/8.

I had to skip 8/8 as well for the moment because, contrary to what I
reported earlier by mistake, it's actually the one leading to the new
cppcheck warning:

dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
 if (ia_type == OPT_IA_NA) {

...also on x86. The difference is not the architecture, rather the
cppcheck version: it happens with 2.16.x but not with 2.14.x.

I'm posting a patch for that soon.

-- 
Stefano

Re: [PATCH 0/8] Avoid running cppcheck on system headers

[David Gibson]

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

On Thu, Nov 07, 2024 at 03:55:16PM +0100, Stefano Brivio wrote:
> On Wed,  6 Nov 2024 17:54:13 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > It turns out cppcheck has inbuilt knowledge of the C library, and
> > isn't typically given the system headers.  Avoiding doing so reduces
> > the runtime to less than half of what it currently is.
> > 
> > For non-obvious reasons, this change also exposes some new warnings.
> > Some are real, one is a cppcheck bug.  Fix and/or workaround these
> > then make the change to the cppcheck options.
> > 
> > This is based on my earlier series with clangd configuration and
> > fixes.
> > 
> > David Gibson (8):
> >   linux_dep: Generalise tcp_info.h to handling Linux extension
> >     compatibility
> >   log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime
> >   linux_dep: Move close_range() conditional handling to linux_dep.h
> >   linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling
> >   ndp: Use const pointer for ndp_ns packet
> >   udp: Don't dereference uflow before NULL check in
> >     udp_reply_sock_handler()
> >   util: Work around cppcheck bug 6936
> >   cppcheck: Don't check the system headers
> 
> Applied, except for 2/8, 3/8, 4/8, and 8/8.
> 
> I had to skip 8/8 as well for the moment because, contrary to what I
> reported earlier by mistake, it's actually the one leading to the new
> cppcheck warning:
> 
> dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
>  if (ia_type == OPT_IA_NA) {
> 
> ...also on x86. The difference is not the architecture, rather the
> cppcheck version: it happens with 2.16.x but not with 2.14.x.
> 
> I'm posting a patch for that soon.

Oh, interesting.  8/8 exposed several new warnings for me too (hence
most of this series), but not that one.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]