From 862ded47343a782d70f7d4421a6a2e4e33684e5e 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: #2017035

---

 src/basic/limits-util.c      | 50 ++++++++++++++++++++++++----------

 src/basic/procfs-util.c      | 53 +++++++++---------------------------

 src/basic/procfs-util.h      |  4 ++-

 src/test/test-process-util.c | 10 +++++--

 src/test/test-procfs-util.c  | 34 ++++++++++++++++-------

 5 files changed, 84 insertions(+), 67 deletions(-)

diff --git a/src/basic/limits-util.c b/src/basic/limits-util.c

index 9f8e26d46a..435a2a0efe 100644

--- a/src/basic/limits-util.c

+++ b/src/basic/limits-util.c

@@ -109,35 +109,57 @@ 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) 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

+         * a) kernel.threads-max sysctl: the maximum number of tasks (threads) the kernel allows.

          *

-         * And then pick the smallest of the three */

+         *    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.

+         *

+         *    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.

+         *

+         * 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)

                 log_debug_errno(r, "Failed to determine cgroup root path, ignoring: %m");

         else {

-                r = cg_get_attribute_as_uint64("pids", root, "pids.max", &b);

+                r = cg_get_attribute_as_uint64("pids", root, "pids.max", &c);

                 if (r < 0)

-                        log_debug_errno(r, "Failed to read pids.max attribute of cgroup root, ignoring: %m");

+                        log_debug_errno(r, "Failed to read pids.max attribute of root cgroup, 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/basic/procfs-util.c b/src/basic/procfs-util.c

index 9234ccaf85..a29e776a3a 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 61fa71d479..eb8c7738b1 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/test/test-process-util.c b/src/test/test-process-util.c

index 8c76392ae9..d89ce6e2db 100644

--- a/src/test/test-process-util.c

+++ b/src/test/test-process-util.c

@@ -850,8 +850,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 b2679e30fb..876ef40dfd 100644

--- a/src/test/test-procfs-util.c

+++ b/src/test/test-procfs-util.c

@@ -6,12 +6,13 @@

 #include "format-util.h"

 #include "log.h"

 #include "procfs-util.h"

+#include "process-util.h"

 #include "tests.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();

@@ -26,26 +27,39 @@ int main(int argc, char *argv[]) {

         assert_se(procfs_tasks_get_current(&v) >= 0);

         log_info("Current number of tasks: %" PRIu64, v);

-        r = procfs_tasks_get_limit(&v);

-        if (r == -ENOENT || ERRNO_IS_PRIVILEGE(r))

-                return log_tests_skipped("can't read /proc/sys/kernel/pid_max");

+        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))

+                return log_tests_skipped("can't set task limits");

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

         }