From 62678ec1aa02b53cb116b6f7dd72a54bf61153b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Tue, 2 Nov 2021 18:18:21 +0100
Subject: [PATCH] procfs-util: fix confusion wrt. quantity limit and maximum
 value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From packit/rawhide-arm64 logs:
Assertion 'limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH' failed at src/test/test-process-util.c:855, function test_get_process_ppid(). Aborting.
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

The kernel has a few different limits. In particular kernel.threads-max can be
set to some lower value, and kernel.pid_max can be set to a higher value. This
is nice because it reduces PID reuse, even if the number of threads that is
allowed is limited. But the tests assumed that we cannot have a thread with
PID above MIN(kernel.threads-max, kernel.pid_max-1), which is not valid.

So let's rework the whole thing: let's expose the helpers to read
kernel.threads-max and kernel.pid_max, and print what they return in tests.
procfs_tasks_get_limit() was something that is only used in tests, and wasn't
very well defined, so let's drop it.

Fixes #21193.

(cherry picked from commit c3dead53d50e334f2d072a2248256983d6dc9f8c)

Related: #1977569
---
 src/basic/procfs-util.c      | 53 +++++++++---------------------------
 src/basic/procfs-util.h      |  4 ++-
 src/basic/util.c             | 49 +++++++++++++++++++++++----------
 src/test/test-process-util.c | 10 +++++--
 src/test/test-procfs-util.c  | 37 +++++++++++++++++++------
 5 files changed, 88 insertions(+), 65 deletions(-)

diff --git a/src/basic/procfs-util.c b/src/basic/procfs-util.c
index 7aaf95bfce..fa5671dd72 100644
--- a/src/basic/procfs-util.c
+++ b/src/basic/procfs-util.c
@@ -12,54 +12,34 @@
 #include "stdio-util.h"
 #include "string-util.h"
 
-int procfs_tasks_get_limit(uint64_t *ret) {
+int procfs_get_pid_max(uint64_t *ret) {
         _cleanup_free_ char *value = NULL;
-        uint64_t pid_max, threads_max;
         int r;
 
         assert(ret);
 
-        /* So there are two sysctl files that control the system limit of processes:
-         *
-         * 1. kernel.threads-max: this is probably the sysctl that makes more sense, as it directly puts a limit on
-         *    concurrent tasks.
-         *
-         * 2. kernel.pid_max: this limits the numeric range PIDs can take, and thus indirectly also limits the number
-         *    of concurrent threads. AFAICS it's primarily a compatibility concept: some crappy old code used a signed
-         *    16bit type for PIDs, hence the kernel provides a way to ensure the PIDs never go beyond INT16_MAX by
-         *    default.
-         *
-         * By default #2 is set to much lower values than #1, hence the limit people come into contact with first, as
-         * it's the lowest boundary they need to bump when they want higher number of processes.
-         *
-         * Also note the weird definition of #2: PIDs assigned will be kept below this value, which means the number of
-         * tasks that can be created is one lower, as PID 0 is not a valid process ID. */
-
         r = read_one_line_file("/proc/sys/kernel/pid_max", &value);
         if (r < 0)
                 return r;
 
-        r = safe_atou64(value, &pid_max);
-        if (r < 0)
-                return r;
+        return safe_atou64(value, ret);
+}
 
-        value = mfree(value);
-        r = read_one_line_file("/proc/sys/kernel/threads-max", &value);
-        if (r < 0)
-                return r;
+int procfs_get_threads_max(uint64_t *ret) {
+        _cleanup_free_ char *value = NULL;
+        int r;
 
-        r = safe_atou64(value, &threads_max);
+        assert(ret);
+
+        r = read_one_line_file("/proc/sys/kernel/threads-max", &value);
         if (r < 0)
                 return r;
 
-        /* Subtract one from pid_max, since PID 0 is not a valid PID */
-        *ret = MIN(pid_max-1, threads_max);
-        return 0;
+        return safe_atou64(value, ret);
 }
 
 int procfs_tasks_set_limit(uint64_t limit) {
         char buffer[DECIMAL_STR_MAX(uint64_t)+1];
-        _cleanup_free_ char *value = NULL;
         uint64_t pid_max;
         int r;
 
@@ -74,10 +54,7 @@ int procfs_tasks_set_limit(uint64_t limit) {
          * set it to the maximum. */
         limit = CLAMP(limit, 20U, TASKS_MAX);
 
-        r = read_one_line_file("/proc/sys/kernel/pid_max", &value);
-        if (r < 0)
-                return r;
-        r = safe_atou64(value, &pid_max);
+        r = procfs_get_pid_max(&pid_max);
         if (r < 0)
                 return r;
 
@@ -98,14 +75,10 @@ int procfs_tasks_set_limit(uint64_t limit) {
                 /* Hmm, we couldn't write this? If so, maybe it was already set properly? In that case let's not
                  * generate an error */
 
-                value = mfree(value);
-                if (read_one_line_file("/proc/sys/kernel/threads-max", &value) < 0)
-                        return r; /* return original error */
-
-                if (safe_atou64(value, &threads_max) < 0)
+                if (procfs_get_threads_max(&threads_max) < 0)
                         return r; /* return original error */
 
-                if (MIN(pid_max-1, threads_max) != limit)
+                if (MIN(pid_max - 1, threads_max) != limit)
                         return r; /* return original error */
 
                 /* Yay! Value set already matches what we were trying to set, hence consider this a success. */
diff --git a/src/basic/procfs-util.h b/src/basic/procfs-util.h
index 5a44e9eff7..caaee8b0b6 100644
--- a/src/basic/procfs-util.h
+++ b/src/basic/procfs-util.h
@@ -5,7 +5,9 @@
 
 #include "time-util.h"
 
-int procfs_tasks_get_limit(uint64_t *ret);
+int procfs_get_pid_max(uint64_t *ret);
+int procfs_get_threads_max(uint64_t *ret);
+
 int procfs_tasks_set_limit(uint64_t limit);
 int procfs_tasks_get_current(uint64_t *ret);
 
diff --git a/src/basic/util.c b/src/basic/util.c
index 609f8c2f33..548e3652cc 100644
--- a/src/basic/util.c
+++ b/src/basic/util.c
@@ -527,23 +527,46 @@ uint64_t physical_memory_scale(uint64_t v, uint64_t max) {
 }
 
 uint64_t system_tasks_max(void) {
-
-        uint64_t a = TASKS_MAX, b = TASKS_MAX;
+        uint64_t a = TASKS_MAX, b = TASKS_MAX, c = TASKS_MAX;
         _cleanup_free_ char *root = NULL;
         int r;
 
-        /* Determine the maximum number of tasks that may run on this system. We check three sources to determine this
-         * limit:
+        /* Determine the maximum number of tasks that may run on this system. We check three sources to
+         * determine this limit:
+         *
+         * a) kernel.threads-max sysctl: the maximum number of tasks (threads) the kernel allows.
+         *
+         *    This puts a direct limit on the number of concurrent tasks.
+         *
+         * b) kernel.pid_max sysctl: the maximum PID value.
+         *
+         *    This limits the numeric range PIDs can take, and thus indirectly also limits the number of
+         *    concurrent threads. It's primarily a compatibility concept: some crappy old code used a signed
+         *    16bit type for PIDs, hence the kernel provides a way to ensure the PIDs never go beyond
+         *    INT16_MAX by default.
          *
-         * a) the maximum tasks value the kernel allows on this architecture
-         * b) the cgroups pids_max attribute for the system
-         * c) the kernel's configured maximum PID value
+         *    Also note the weird definition: PIDs assigned will be kept below this value, which means
+         *    the number of tasks that can be created is one lower, as PID 0 is not a valid process ID.
          *
-         * And then pick the smallest of the three */
+         * c) pids.max on the root cgroup: the kernel's configured maximum number of tasks.
+         *
+         * and then pick the smallest of the three.
+         *
+         * By default pid_max is set to much lower values than threads-max, hence the limit people come into
+         * contact with first, as it's the lowest boundary they need to bump when they want higher number of
+         * processes.
+         */
+
+        r = procfs_get_threads_max(&a);
+        if (r < 0)
+                log_debug_errno(r, "Failed to read kernel.threads-max, ignoring: %m");
 
-        r = procfs_tasks_get_limit(&a);
+        r = procfs_get_pid_max(&b);
         if (r < 0)
-                log_debug_errno(r, "Failed to read maximum number of tasks from /proc, ignoring: %m");
+                log_debug_errno(r, "Failed to read kernel.pid_max, ignoring: %m");
+        else if (b > 0)
+                /* Subtract one from pid_max, since PID 0 is not a valid PID */
+                b--;
 
         r = cg_get_root_path(&root);
         if (r < 0)
@@ -555,15 +578,13 @@ uint64_t system_tasks_max(void) {
                 if (r < 0)
                         log_debug_errno(r, "Failed to read pids.max attribute of cgroup root, ignoring: %m");
                 else if (!streq(value, "max")) {
-                        r = safe_atou64(value, &b);
+                        r = safe_atou64(value, &c);
                         if (r < 0)
                                 log_debug_errno(r, "Failed to parse pids.max attribute of cgroup root, ignoring: %m");
                 }
         }
 
-        return MIN3(TASKS_MAX,
-                    a <= 0 ? TASKS_MAX : a,
-                    b <= 0 ? TASKS_MAX : b);
+        return MIN3(a, b, c);
 }
 
 uint64_t system_tasks_max_scale(uint64_t v, uint64_t max) {
diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c
index 6b14ff592b..6295889b47 100644
--- a/src/test/test-process-util.c
+++ b/src/test/test-process-util.c
@@ -598,8 +598,14 @@ static void test_get_process_ppid(void) {
         assert_se(get_process_ppid(1, NULL) == -EADDRNOTAVAIL);
 
         /* the process with the PID above the global limit definitely doesn't exist. Verify that */
-        assert_se(procfs_tasks_get_limit(&limit) >= 0);
-        assert_se(limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH);
+        assert_se(procfs_get_pid_max(&limit) >= 0);
+        log_debug("kernel.pid_max = %"PRIu64, limit);
+
+        if (limit < INT_MAX) {
+                r = get_process_ppid(limit + 1, NULL);
+                log_debug_errno(r, "get_process_limit(%"PRIu64") → %d/%m", limit + 1, r);
+                assert(r == -ESRCH);
+        }
 
         for (pid_t pid = 0;;) {
                 _cleanup_free_ char *c1 = NULL, *c2 = NULL;
diff --git a/src/test/test-procfs-util.c b/src/test/test-procfs-util.c
index 1d0612985b..bb6943fed0 100644
--- a/src/test/test-procfs-util.c
+++ b/src/test/test-procfs-util.c
@@ -5,11 +5,13 @@
 #include "log.h"
 #include "parse-util.h"
 #include "procfs-util.h"
+#include "process-util.h"
+#include "util.h"
 
 int main(int argc, char *argv[]) {
         char buf[CONST_MAX(FORMAT_TIMESPAN_MAX, FORMAT_BYTES_MAX)];
         nsec_t nsec;
-        uint64_t v;
+        uint64_t v, w;
         int r;
 
         log_parse_environment();
@@ -24,22 +26,41 @@ int main(int argc, char *argv[]) {
         assert_se(procfs_tasks_get_current(&v) >= 0);
         log_info("Current number of tasks: %" PRIu64, v);
 
-        assert_se(procfs_tasks_get_limit(&v) >= 0);
+        v = TASKS_MAX;
+        r = procfs_get_pid_max(&v);
+        assert(r >= 0 || r == -ENOENT || ERRNO_IS_PRIVILEGE(r));
+        log_info("kernel.pid_max: %"PRIu64, v);
+
+        w = TASKS_MAX;
+        r = procfs_get_threads_max(&w);
+        assert(r >= 0 || r == -ENOENT || ERRNO_IS_PRIVILEGE(r));
+        log_info("kernel.threads-max: %"PRIu64, w);
+
+        v = MIN(v - (v > 0), w);
+
+        assert_se(r >= 0);
         log_info("Limit of tasks: %" PRIu64, v);
         assert_se(v > 0);
-        assert_se(procfs_tasks_set_limit(v) >= 0);
+        r = procfs_tasks_set_limit(v);
+        if (r == -ENOENT || ERRNO_IS_PRIVILEGE(r)) {
+                log_notice_errno(r, "Skipping test: can't set task limits");
+                return EXIT_TEST_SKIP;
+        }
+        assert(r >= 0);
 
         if (v > 100) {
-                uint64_t w;
+                log_info("Reducing limit by one to %"PRIu64"…", v-1);
+
                 r = procfs_tasks_set_limit(v-1);
-                assert_se(IN_SET(r, 0, -EPERM, -EACCES, -EROFS));
+                log_info_errno(r, "procfs_tasks_set_limit: %m");
+                assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r));
 
-                assert_se(procfs_tasks_get_limit(&w) >= 0);
-                assert_se((r == 0 && w == v - 1) || (r < 0 && w == v));
+                assert_se(procfs_get_threads_max(&w) >= 0);
+                assert_se(r >= 0 ? w == v - 1 : w == v);
 
                 assert_se(procfs_tasks_set_limit(v) >= 0);
 
-                assert_se(procfs_tasks_get_limit(&w) >= 0);
+                assert_se(procfs_get_threads_max(&w) >= 0);
                 assert_se(v == w);
         }