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@redhat.com>
---
src/util/virprocess.c | 98 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index abce522bf3..e62ec379a6 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -757,6 +757,95 @@ 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;
+ }
+}
+
+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 len;
+ size_t i;
+
+ if (!(label = virProcessLimitResourceToLabel(resource))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown resource %d requested for process %lld"),
+ resource, (long long)pid);
+ return -1;
+ }
+
+ procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
+
+ if (!g_file_get_contents(procfile, &buf, &len, NULL))
+ 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 (!STRPREFIX(line, label))
+ continue;
+
+ line += strlen(label);
+
+ 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;
+}
+
static int
virProcessGetLimit(pid_t pid,
int resource,
@@ -768,6 +857,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 3/5/21 8:13 PM, Andrea Bolognani wrote:
> 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@redhat.com>
> ---
> src/util/virprocess.c | 98 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index abce522bf3..e62ec379a6 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -757,6 +757,95 @@ 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;
> + }
> +}
> +
> +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 len;
> + size_t i;
> +
> + if (!(label = virProcessLimitResourceToLabel(resource))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown resource %d requested for process %lld"),
> + resource, (long long)pid);
> + return -1;
> + }
> +
> + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> +
> + if (!g_file_get_contents(procfile, &buf, &len, NULL))
> + 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 (!STRPREFIX(line, label))
> + continue;
> +
> + line += strlen(label);
Or 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;
> +}
> +
> static int
> virProcessGetLimit(pid_t pid,
> int resource,
> @@ -768,6 +857,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;
>
>
Michal
On Mon, 2021-03-08 at 11:30 +0100, Michal Privoznik wrote: > On 3/5/21 8:13 PM, Andrea Bolognani wrote: > > + if (!STRPREFIX(line, label)) > > + continue; > > + > > + line += strlen(label); > > Or if (!(line = STRSKIP(line, label)) continue; Oh, I didn't know that existed! Nice suggestion :) -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 05, 2021 at 08:13:59PM +0100, Andrea Bolognani wrote:
> 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@redhat.com>
> ---
> src/util/virprocess.c | 98 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index abce522bf3..e62ec379a6 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -757,6 +757,95 @@ 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;
> + }
> +}
> +
> +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 len;
> + size_t i;
> +
> + if (!(label = virProcessLimitResourceToLabel(resource))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown resource %d requested for process %lld"),
> + resource, (long long)pid);
> + return -1;
Setting errors on -1
> + }
> +
> + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> +
> + if (!g_file_get_contents(procfile, &buf, &len, NULL))
> + return -1;
Not setting errors on -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 (!STRPREFIX(line, label))
> + continue;
> +
> + line += strlen(label);
> +
> + 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;
> +}
> +
> static int
> virProcessGetLimit(pid_t pid,
> int resource,
> @@ -768,6 +857,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;
This ought to be conditional for Linux only and error reporting needs
to be made consistent.
> +
> if (same_process && virProcessGetRLimit(resource, old_limit) == 0)
> return 0;
>
> --
> 2.26.2
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 2021-03-08 at 10:50 +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 05, 2021 at 08:13:59PM +0100, Andrea Bolognani wrote:
> > + if (!(label = virProcessLimitResourceToLabel(resource))) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Unknown resource %d requested for process %lld"),
> > + resource, (long long)pid);
> > + return -1;
>
> Setting errors on -1
This is only hit if virProcessGetLimitFromProc() has been asked to
obtain limits for a resource it doesn't know how to fetch, which
indicates a bug in libvirt and is thus reported as internal error.
> > + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> > +
> > + if (!g_file_get_contents(procfile, &buf, &len, NULL))
> > + return -1;
>
> Not setting errors on -1
This is simply "file couldn't be read", which would be the case on
FreeBSD for example.
> > + /* 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;
>
> This ought to be conditional for Linux only and error reporting needs
> to be made consistent.
The intent above was to have this fail quietly on non-Linux without
adding checks for it, but sure I can have an actual stub on other
platforms instead.
--
Andrea Bolognani / Red Hat / Virtualization
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
> 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@redhat.com>
> ---
> src/util/virprocess.c | 98 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
Sorry in advance for hijacking this thread.
> +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 len;
> + size_t i;
> +
> + if (!(label = virProcessLimitResourceToLabel(resource))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown resource %d requested for process %lld"),
> + resource, (long long)pid);
> + return -1;
> + }
> +
> + procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> +
> + if (!g_file_get_contents(procfile, &buf, &len, NULL))
> + return -1;
I did not spot this yesterday, but now I'm working on a something else
and have to read a contents of a file under /proc. I did not recall the
exact name but remembered where I saw it lately - here :-)
And now that I am thinking about it - and reading the docs - is this
function safe? I mean, it reads file without any limit - which may be
fine for /proc files, but I worry that if allowed in one func it may
sneak into others and read user provided files, or while its use in a
function X might be warranted for now, in the future after some refactor
the function X might be used to read user provided files.
Therefore, I think it should go onto the list of not-on-my-watch
functions and we ought stick with our fine crafted virFileRead*().
BTW: I think the same about g_get_host_name(), which does not reflect
hostname changes. Unfortunately, we have three places which slipped
through while I wasn't watching. I'll look into how to revert them.
Michal
On Tue, 2021-03-09 at 12:43 +0100, Michal Privoznik wrote: > On 3/5/21 8:13 PM, Andrea Bolognani wrote: > > + if (!g_file_get_contents(procfile, &buf, &len, NULL)) > > + return -1; > > I did not spot this yesterday, but now I'm working on a something else > and have to read a contents of a file under /proc. I did not recall the > exact name but remembered where I saw it lately - here :-) > > And now that I am thinking about it - and reading the docs - is this > function safe? I mean, it reads file without any limit - which may be > fine for /proc files, but I worry that if allowed in one func it may > sneak into others and read user provided files, or while its use in a > function X might be warranted for now, in the future after some refactor > the function X might be used to read user provided files. You're right. I used pure GLib functions initially because I was implementing this as a tiny stand-alone tool for faster iterative development, and I just forgot to change that specific function back to the libvirt equivalent when I was done :) > Therefore, I think it should go onto the list of not-on-my-watch > functions and we ought stick with our fine crafted virFileRead*(). > > BTW: I think the same about g_get_host_name(), which does not reflect > hostname changes. Unfortunately, we have three places which slipped > through while I wasn't watching. I'll look into how to revert them. Sounds good. -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2026 Red Hat, Inc.