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. v2: * Dropped patches already merged * Rebased on Stefano's series of lint fixes * Add a missing #include in 1/4 and 2/4 * Adjust 3/4 not to die if close_range() is unavailable David Gibson (4): 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 cppcheck: Don't check the system headers Makefile | 15 +-------------- linux_dep.h | 22 ++++++++++++++++++++++ log.c | 10 ++-------- util.c | 17 +++++++++++++++-- util.h | 19 ------------------- 5 files changed, 40 insertions(+), 43 deletions(-) -- 2.47.0
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(a)gibson.dropbear.id.au>
---
Makefile | 5 -----
linux_dep.h | 6 ++++++
log.c | 10 ++--------
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/Makefile b/Makefile
index 3c82f50..0ba85b4 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..239c8ce 100644
--- a/log.c
+++ b/log.c
@@ -26,6 +26,7 @@
#include <stdarg.h>
#include <sys/socket.h>
+#include "linux_dep.h"
#include "log.h"
#include "util.h"
#include "passt.h"
@@ -92,7 +93,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 +126,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 +197,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 +427,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
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(a)gibson.dropbear.id.au>
---
linux_dep.h | 20 ++++++++++++++++++++
util.c | 1 +
util.h | 19 -------------------
3 files changed, 21 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.c b/util.c
index 3448f30..913f34b 100644
--- a/util.c
+++ b/util.c
@@ -28,6 +28,7 @@
#include <linux/errqueue.h>
#include <getopt.h>
+#include "linux_dep.h"
#include "util.h"
#include "iov.h"
#include "passt.h"
diff --git a/util.h b/util.h
index 963f57b..3616515 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"
@@ -171,24 +170,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
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
We don't want to simply error if close_range() or CLOSE_RANGE_UNSHARE isn't
available, because that would require running on kernel >= 5.9. On the
other hand there's not really any other way to flush all possible fds
leaked by the parent (close() in a loop takes over a minute). So in this
case print a warning and carry on.
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(a)gibson.dropbear.id.au>
---
linux_dep.h | 12 ++++--------
util.c | 16 ++++++++++++++--
2 files changed, 18 insertions(+), 10 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 */
diff --git a/util.c b/util.c
index 913f34b..126dedb 100644
--- a/util.c
+++ b/util.c
@@ -738,8 +738,20 @@ void close_open_files(int argc, char **argv)
rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
}
- if (rc)
- die_perror("Failed to close files leaked by parent");
+ if (rc) {
+ if (errno == ENOSYS || errno == EINVAL) {
+ /* This probably means close_range() or the
+ * CLOSE_RANGE_UNSHARE flag is not supported by the
+ * kernel. Not much we can do here except carry on and
+ * hope for the best.
+ */
+ warn(
+"Can't use close_range() to ensure no files leaked by parent");
+ } else {
+ die_perror("Failed to close files leaked by parent");
+ }
+ }
+
}
/**
--
2.47.0
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(a)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)
--
2.47.0
On Fri, 8 Nov 2024 13:53:26 +1100 David Gibson <david(a)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.Applied. -- Stefano