Calling prlimit() requires elevated privileges, specifically CAP_SYS_RESOURCE, and getrlimit() only works for the current process which is too limiting for our needs; /proc/$pid/limits, on the other hand, can be read by any process, so implement parsing that file as a fallback for when prlimit() fails. This is useful in containerized environments. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- src/util/virprocess.c | 102 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8428c91182..4fa854090d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -757,6 +757,99 @@ virProcessSetRLimit(int resource, #endif /* WITH_SETRLIMIT */ #if WITH_GETRLIMIT +static const char* +virProcessLimitResourceToLabel(int resource) +{ + switch (resource) { +# if defined(RLIMIT_MEMLOCK) + case RLIMIT_MEMLOCK: + return "Max locked memory"; +# endif /* defined(RLIMIT_MEMLOCK) */ + +# if defined(RLIMIT_NPROC) + case RLIMIT_NPROC: + return "Max processes"; +# endif /* defined(RLIMIT_NPROC) */ + +# if defined(RLIMIT_NOFILE) + case RLIMIT_NOFILE: + return "Max open files"; +# endif /* defined(RLIMIT_NOFILE) */ + +# if defined(RLIMIT_CORE) + case RLIMIT_CORE: + return "Max core file size"; +# endif /* defined(RLIMIT_CORE) */ + + default: + return NULL; + } +} + +# if defined(__linux__) +static int +virProcessGetLimitFromProc(pid_t pid, + int resource, + struct rlimit *limit) +{ + g_autofree char *procfile = NULL; + g_autofree char *buf = NULL; + g_auto(GStrv) lines = NULL; + const char *label; + size_t i; + + if (!(label = virProcessLimitResourceToLabel(resource))) { + return -1; + } + + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); + + if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) + return -1; + + if (!(lines = g_strsplit(buf, "\n", 0))) + return -1; + + for (i = 0; lines[i]; i++) { + g_autofree char *softLimit = NULL; + g_autofree char *hardLimit = NULL; + char *line = lines[i]; + unsigned long long tmp; + + if (!(line = STRSKIP(line, label))) + continue; + + if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) + return -1; + + if (STREQ(softLimit, "unlimited")) { + limit->rlim_cur = RLIM_INFINITY; + } else { + if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_cur = tmp; + } + if (STREQ(hardLimit, "unlimited")) { + limit->rlim_max = RLIM_INFINITY; + } else { + if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) + return -1; + limit->rlim_max = tmp; + } + } + + return 0; +} +# else /* !defined(__linux__) */ +static int +virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, + int resource G_GNUC_UNUSED, + struct rlimit *limit G_GNUC_UNUSED) +{ + return -1; +} +# endif /* !defined(__linux__) */ + static int virProcessGetLimit(pid_t pid, int resource, @@ -768,6 +861,15 @@ virProcessGetLimit(pid_t pid, if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0) return 0; + /* For whatever reason, using prlimit() on another process - even + * when it's just to obtain the current limit rather than changing + * it - requires CAP_SYS_RESOURCE, which we might not have in a + * containerized environment; on the other hand, no particular + * permission is needed to poke around /proc, so try that if going + * through the syscall didn't work */ + if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0) + return 0; + if (same_process && virProcessGetRLimit(resource, old_limit) == 0) return 0; -- 2.26.2

On Mon, 2021-03-15 at 10:21 +0100, Michal Privoznik wrote: Good catch! The changes you're suggesting are not trivial enough for me to feel comfortable simply applying them locally and then pushing right away. Would the diff below look reasonable squashed in? diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4fa854090d..cae0dabae3 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid, size_t i; if (!(label = virProcessLimitResourceToLabel(resource))) { + virReportSystemError(EINVAL, "%s", _("Invalid resource")); return -1; } procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid); if (virFileReadAllQuiet(procfile, 2048, &buf) < 0) - return -1; + goto error; if (!(lines = g_strsplit(buf, "\n", 0))) - return -1; + goto error; for (i = 0; lines[i]; i++) { g_autofree char *softLimit = NULL; @@ -820,25 +821,29 @@ virProcessGetLimitFromProc(pid_t pid, continue; if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2) - return -1; + goto error; if (STREQ(softLimit, "unlimited")) { limit->rlim_cur = RLIM_INFINITY; } else { if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_cur = tmp; } if (STREQ(hardLimit, "unlimited")) { limit->rlim_max = RLIM_INFINITY; } else { if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0) - return -1; + goto error; limit->rlim_max = tmp; } } return 0; + + error: + virReportSystemError(EIO, "%s", _("Input/output error")); + return -1; } # else /* !defined(__linux__) */ static int @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED, int resource G_GNUC_UNUSED, struct rlimit *limit G_GNUC_UNUSED) { + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; } # endif /* !defined(__linux__) */ -- Andrea Bolognani / Red Hat / Virtualization

On 3/15/21 12:22 PM, Andrea Bolognani wrote: virFileReadAllQuiet() sets errno, this would just overwrite it with something less specific. I think this check can be dropped. g_strsplit() doesn't ever return NULL really, does it? The rest looks good. Michal

Now that we've implemented a fallback for the function that obtains the information from /proc, there is no reason we would get a failure unless there's something seriously wrong with the environment we're running in, in which case we're better off reporting the issue to the user rather than pretending everything is fine. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 73e2473dce..1e9178764b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9246,11 +9246,10 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, if (bytes) { /* If this is the first time adjusting the limit, save the current * value so that we can restore it once memory locking is no longer - * required. Failing to obtain the current limit is not a critical - * failure, it just means we'll be unable to lower it later */ + * required */ if (!vm->originalMemlock) { if (virProcessGetMaxMemLock(vm->pid, &(vm->originalMemlock)) < 0) - vm->originalMemlock = 0; + return -1; } } else { /* Once memory locking is no longer required, we can restore the -- 2.26.2

Attempting to set the memlock limit might fail if we're running in a containerized environment where CAP_SYS_RESOURCE is not available, and if the limit is already high enough there's no point in trying to raise anyway. https://bugzilla.redhat.com/show_bug.cgi?id=1916346 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f8b0e1a62a..560c73b560 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9247,11 +9247,20 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm, return -1; if (desiredMemLock > 0) { - /* If this is the first time adjusting the limit, save the current - * value so that we can restore it once memory locking is no longer - * required */ - if (vm->originalMemlock == 0) { - vm->originalMemlock = currentMemLock; + if (currentMemLock < desiredMemLock) { + /* If this is the first time adjusting the limit, save the current + * value so that we can restore it once memory locking is no longer + * required */ + if (vm->originalMemlock == 0) { + vm->originalMemlock = currentMemLock; + } + } else { + /* If the limit is already high enough, we can assume + * that some external process is taking care of managing + * process limits and we shouldn't do anything ourselves: + * we're probably running in a containerized environment + * where we don't have enough privilege anyway */ + desiredMemLock = 0; } } else { /* Once memory locking is no longer required, we can restore the -- 2.26.2