1:13 p.m.
This feature has been requested by KubeVirt developers and will make
it possible for them to make some VFIO-related features, such as
migration and hotplug, work correctly.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
The first part of the series, especially the first 9 patches, is
preparation work: it addresses a few annoying issues with our APIs
that deal with process limits, and makes them all nice, consistent
and easy to reason about while moving policy code from the generic
code to the QEMU driver where it belongs.
Andrea Bolognani (17):
util: Document limit-related functions
util: Simplify stubs
util: Always pass a pid to virProcessSetMax*()
util: Introduce virProcess{Get,Set}Limit()
qemu: Make some minor tweaks
qemu: Set all limits at the same time
util: Have virCommand remember whether limits are set
qemu: Set limits only when explicitly asked to do so
util: Don't special-case setting a limit to zero
conf: Rename original_memlock -> originalMemlock
tests: Mock virProcessGetMaxMemLock()
util: Try to get limits from /proc
qemu: Don't ignore virProcessGetMaxMemLock() errors
qemu: Refactor qemuDomainAdjustMaxMemLock()
qemu: Add external_limit_manager config knob
qemu: Wire up external limit manager
news: Document external limit manager feature
NEWS.rst | 10 +
src/conf/domain_conf.h | 5 +-
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 12 +
src/qemu/qemu_command.c | 4 -
src/qemu/qemu_conf.c | 4 +
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_domain.c | 47 ++--
src/qemu/qemu_migration.c | 2 +
src/qemu/qemu_process.c | 30 ++-
src/qemu/test_libvirtd_qemu.aug.in | 1 +
src/util/vircommand.c | 21 +-
src/util/virprocess.c | 340 ++++++++++++++++++++---------
src/util/virprocess.h | 2 +-
tests/virprocessmock.c | 7 +
15 files changed, 354 insertions(+), 133 deletions(-)
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 01/17] util: Document limit-related functions
We're going to change their behavior, so it's good to have the
current one documented to serve as baseline.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 46 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 69d64e9466..9fe75d54d6 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -739,6 +739,15 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED,
#endif
#if WITH_SETRLIMIT && defined(RLIMIT_MEMLOCK)
+/**
+ * virProcessSetMaxMemLock:
+ * @pid: process to be changed (0 for the current process)
+ * @bytes: new limit (0 for no change)
+ *
+ * Sets a new limit on the amount of locked memory for a process.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
int
virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
{
@@ -791,6 +800,15 @@ virProcessSetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long
bytes)
#endif /* ! (WITH_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
#if WITH_GETRLIMIT && defined(RLIMIT_MEMLOCK)
+/**
+ * virProcessGetMaxMemLock:
+ * @pid: process to be queried (0 for the current process)
+ * @bytes: return location for the limit
+ *
+ * Obtain the current limit on the amount of locked memory for a process.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
int
virProcessGetMaxMemLock(pid_t pid,
unsigned long long *bytes)
@@ -843,6 +861,16 @@ virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED,
#endif /* ! (WITH_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
#if WITH_SETRLIMIT && defined(RLIMIT_NPROC)
+/**
+ * virProcessSetMaxProcesses:
+ * @pid: process to be changed (0 for the current process)
+ * @procs: new limit (0 for no change)
+ *
+ * Sets a new limit on the amount of processes for the user the
+ * process is running as.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
int
virProcessSetMaxProcesses(pid_t pid, unsigned int procs)
{
@@ -883,6 +911,15 @@ virProcessSetMaxProcesses(pid_t pid G_GNUC_UNUSED, unsigned int
procs)
#endif /* ! (WITH_SETRLIMIT && defined(RLIMIT_NPROC)) */
#if WITH_SETRLIMIT && defined(RLIMIT_NOFILE)
+/**
+ * virProcessSetMaxFiles:
+ * @pid: process to be changed (0 for the current process)
+ * @files: new limit (0 for no change)
+ *
+ * Sets a new limit on the number of opened files for a process.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
int
virProcessSetMaxFiles(pid_t pid, unsigned int files)
{
@@ -931,6 +968,15 @@ virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED, unsigned int files)
#endif /* ! (WITH_SETRLIMIT && defined(RLIMIT_NOFILE)) */
#if WITH_SETRLIMIT && defined(RLIMIT_CORE)
+/**
+ * virProcessSetMaxCoreSize:
+ * @pid: process to be changed (0 for the current process)
+ * @bytes: new limit (0 to disable core dumps)
+ *
+ * Sets a new limit on the size of core dumps for a process.
+ *
+ * Returns: 0 on success, <0 on failure.
+ */
int
virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes)
{
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 02/17] util: Simplify stubs
Calling a stub should always result in ENOSYS being raised,
regardless of what arguments are passed to it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 9fe75d54d6..e01ff25540 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -850,11 +850,8 @@ virProcessGetMaxMemLock(pid_t pid,
#else /* ! (WITH_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */
int
virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED,
- unsigned long long *bytes)
+ unsigned long long *bytes G_GNUC_UNUSED)
{
- if (!bytes)
- return 0;
-
virReportSystemError(ENOSYS, "%s", _("Not supported on this
platform"));
return -1;
}
@@ -900,11 +897,9 @@ virProcessSetMaxProcesses(pid_t pid, unsigned int procs)
}
#else /* ! (WITH_SETRLIMIT && defined(RLIMIT_NPROC)) */
int
-virProcessSetMaxProcesses(pid_t pid G_GNUC_UNUSED, unsigned int procs)
+virProcessSetMaxProcesses(pid_t pid G_GNUC_UNUSED,
+ unsigned int procs G_GNUC_UNUSED)
{
- if (procs == 0)
- return 0;
-
virReportSystemError(ENOSYS, "%s", _("Not supported on this
platform"));
return -1;
}
@@ -957,11 +952,9 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files)
}
#else /* ! (WITH_SETRLIMIT && defined(RLIMIT_NOFILE)) */
int
-virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED, unsigned int files)
+virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED,
+ unsigned int files G_GNUC_UNUSED)
{
- if (files == 0)
- return 0;
-
virReportSystemError(ENOSYS, "%s", _("Not supported on this
platform"));
return -1;
}
@@ -1004,11 +997,8 @@ virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes)
#else /* ! (WITH_SETRLIMIT && defined(RLIMIT_CORE)) */
int
virProcessSetMaxCoreSize(pid_t pid G_GNUC_UNUSED,
- unsigned long long bytes)
+ unsigned long long bytes G_GNUC_UNUSED)
{
- if (bytes == 0)
- return 0;
-
virReportSystemError(ENOSYS, "%s", _("Not supported on this
platform"));
return -1;
}
--
2.26.2
4:31 a.m.
New subject: [libvirt PATCH 02/17] util: Simplify stubs
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
Calling a stub should always result in ENOSYS being raised,
regardless of what arguments are passed to it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
Missed virProcessSetMaxMemLock().
Michal
7:23 a.m.
New subject: [libvirt PATCH 02/17] util: Simplify stubs
On Mon, 2021-03-08 at 11:31 +0100, Michal Privoznik wrote:
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
> Calling a stub should always result in ENOSYS being raised,
> regardless of what arguments are passed to it.
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> src/util/virprocess.c | 22 ++++++----------------
> 1 file changed, 6 insertions(+), 16 deletions(-)
Missed virProcessSetMaxMemLock().
Good catch - I must have misplaced it during a rebase O:-)
--
Andrea Bolognani / Red Hat / Virtualization
1:13 p.m.
New subject: [libvirt PATCH 03/17] util: Always pass a pid to virProcessSetMax*()
Currently, the functions accept either an explicit pid or zero,
in which case the current process should be modified: the latter
might sound like a convenient little feature, but in reality
obtaining the pid of the current process is a single additional
function call away, so it hardly makes a difference.
Removing the few cases in which we're passing zero will allow us
to simplify and improve the functions later.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/vircommand.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 1a4b77ea24..3eef0767bb 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -780,11 +780,13 @@ virExec(virCommandPtr cmd)
}
}
+ pid = getpid();
+
if (cmd->pidfile) {
int pidfilefd = -1;
char c;
- pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, getpid());
+ pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, pid);
if (pidfilefd < 0)
goto fork_error;
if (virSetInherit(pidfilefd, true) < 0) {
@@ -804,14 +806,14 @@ virExec(virCommandPtr cmd)
/* pidfilefd is intentionally leaked. */
}
- if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0)
+ if (virProcessSetMaxMemLock(pid, cmd->maxMemLock) < 0)
goto fork_error;
- if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0)
+ if (virProcessSetMaxProcesses(pid, cmd->maxProcesses) < 0)
goto fork_error;
- if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0)
+ if (virProcessSetMaxFiles(pid, cmd->maxFiles) < 0)
goto fork_error;
if (cmd->setMaxCore &&
- virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
+ virProcessSetMaxCoreSize(pid, cmd->maxCore) < 0)
goto fork_error;
if (cmd->hook) {
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 04/17] util: Introduce virProcess{Get,Set}Limit()
These functions abstract part of the existing logic, which is
the same in all virProcessSetMax*() functions, and changes it
so that which underlying syscall is used depends on their
availability rather than on the context in which they are
called: since prlimit() and {g,s}etrlimit() have slightly
different requirements, using the same one every single time
should make for a more consistent experience.
As part of the change, we also remove the special case for
passing zero to virProcessSetMax*() functions: we have removed
all callers that depended on that functionality in the previous
commit, so this is now safe to do and makes the semantics
simpler.
This commit is better viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 175 +++++++++++++++++++++++-------------------
1 file changed, 95 insertions(+), 80 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index e01ff25540..38b248217e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -738,10 +738,66 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED,
}
#endif
+#if WITH_GETRLIMIT
+static int
+virProcessGetRLimit(int resource,
+ struct rlimit *old_limit)
+{
+ return getrlimit(resource, old_limit);
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetRLimit(int resource,
+ const struct rlimit *new_limit)
+{
+ return setrlimit(resource, new_limit);
+}
+#endif /* WITH_SETRLIMIT */
+
+#if WITH_GETRLIMIT
+static int
+virProcessGetLimit(pid_t pid,
+ int resource,
+ struct rlimit *old_limit)
+{
+ pid_t current_pid = getpid();
+ bool same_process = (pid == current_pid);
+
+ if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0)
+ return 0;
+
+ if (same_process && virProcessGetRLimit(resource, old_limit) == 0)
+ return 0;
+
+ return -1;
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetLimit(pid_t pid,
+ int resource,
+ const struct rlimit *new_limit)
+{
+ pid_t current_pid = getpid();
+ bool same_process = (pid == current_pid);
+
+ if (virProcessPrLimit(pid, resource, new_limit, NULL) == 0)
+ return 0;
+
+ if (same_process && virProcessSetRLimit(resource, new_limit) == 0)
+ return 0;
+
+ return -1;
+}
+#endif /* WITH_SETRLIMIT */
+
#if WITH_SETRLIMIT && defined(RLIMIT_MEMLOCK)
/**
* virProcessSetMaxMemLock:
- * @pid: process to be changed (0 for the current process)
+ * @pid: process to be changed
* @bytes: new limit (0 for no change)
*
* Sets a new limit on the amount of locked memory for a process.
@@ -765,21 +821,11 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
else
rlim.rlim_cur = rlim.rlim_max = RLIM_INFINITY;
- if (pid == 0) {
- if (setrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
- virReportSystemError(errno,
- _("cannot limit locked memory to %llu"),
- bytes);
- return -1;
- }
- } else {
- if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, &rlim, NULL) < 0) {
- virReportSystemError(errno,
- _("cannot limit locked memory "
- "of process %lld to %llu"),
- (long long int)pid, bytes);
- return -1;
- }
+ if (virProcessSetLimit(pid, RLIMIT_MEMLOCK, &rlim) < 0) {
+ virReportSystemError(errno,
+ _("cannot limit locked memory "
+ "of process %lld to %llu"),
+ (long long int)pid, bytes);
}
VIR_DEBUG("Locked memory for process %lld limited to %llu bytes",
@@ -802,7 +848,7 @@ virProcessSetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long
bytes)
#if WITH_GETRLIMIT && defined(RLIMIT_MEMLOCK)
/**
* virProcessGetMaxMemLock:
- * @pid: process to be queried (0 for the current process)
+ * @pid: process to be queried
* @bytes: return location for the limit
*
* Obtain the current limit on the amount of locked memory for a process.
@@ -818,21 +864,12 @@ virProcessGetMaxMemLock(pid_t pid,
if (!bytes)
return 0;
- if (pid == 0) {
- if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
- virReportSystemError(errno,
- "%s",
- _("cannot get locked memory limit"));
- return -1;
- }
- } else {
- if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) {
- virReportSystemError(errno,
- _("cannot get locked memory limit "
- "of process %lld"),
- (long long int) pid);
- return -1;
- }
+ if (virProcessGetLimit(pid, RLIMIT_MEMLOCK, &rlim) < 0) {
+ virReportSystemError(errno,
+ _("cannot get locked memory limit "
+ "of process %lld"),
+ (long long int) pid);
+ return -1;
}
/* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the
@@ -860,7 +897,7 @@ virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED,
#if WITH_SETRLIMIT && defined(RLIMIT_NPROC)
/**
* virProcessSetMaxProcesses:
- * @pid: process to be changed (0 for the current process)
+ * @pid: process to be changed
* @procs: new limit (0 for no change)
*
* Sets a new limit on the amount of processes for the user the
@@ -877,21 +914,13 @@ virProcessSetMaxProcesses(pid_t pid, unsigned int procs)
return 0;
rlim.rlim_cur = rlim.rlim_max = procs;
- if (pid == 0) {
- if (setrlimit(RLIMIT_NPROC, &rlim) < 0) {
- virReportSystemError(errno,
- _("cannot limit number of subprocesses to
%u"),
- procs);
- return -1;
- }
- } else {
- if (virProcessPrLimit(pid, RLIMIT_NPROC, &rlim, NULL) < 0) {
- virReportSystemError(errno,
- _("cannot limit number of subprocesses "
- "of process %lld to %u"),
- (long long int)pid, procs);
- return -1;
- }
+
+ if (virProcessSetLimit(pid, RLIMIT_NPROC, &rlim) < 0) {
+ virReportSystemError(errno,
+ _("cannot limit number of subprocesses "
+ "of process %lld to %u"),
+ (long long int)pid, procs);
+ return -1;
}
return 0;
}
@@ -908,7 +937,7 @@ virProcessSetMaxProcesses(pid_t pid G_GNUC_UNUSED,
#if WITH_SETRLIMIT && defined(RLIMIT_NOFILE)
/**
* virProcessSetMaxFiles:
- * @pid: process to be changed (0 for the current process)
+ * @pid: process to be changed
* @files: new limit (0 for no change)
*
* Sets a new limit on the number of opened files for a process.
@@ -932,22 +961,15 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files)
* behavior.
*/
rlim.rlim_cur = rlim.rlim_max = files + 1;
- if (pid == 0) {
- if (setrlimit(RLIMIT_NOFILE, &rlim) < 0) {
- virReportSystemError(errno,
- _("cannot limit number of open files to %u"),
- files);
- return -1;
- }
- } else {
- if (virProcessPrLimit(pid, RLIMIT_NOFILE, &rlim, NULL) < 0) {
- virReportSystemError(errno,
- _("cannot limit number of open files "
- "of process %lld to %u"),
- (long long int)pid, files);
- return -1;
- }
+
+ if (virProcessSetLimit(pid, RLIMIT_NOFILE, &rlim) < 0) {
+ virReportSystemError(errno,
+ _("cannot limit number of open files "
+ "of process %lld to %u"),
+ (long long int)pid, files);
+ return -1;
}
+
return 0;
}
#else /* ! (WITH_SETRLIMIT && defined(RLIMIT_NOFILE)) */
@@ -963,7 +985,7 @@ virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED,
#if WITH_SETRLIMIT && defined(RLIMIT_CORE)
/**
* virProcessSetMaxCoreSize:
- * @pid: process to be changed (0 for the current process)
+ * @pid: process to be changed
* @bytes: new limit (0 to disable core dumps)
*
* Sets a new limit on the size of core dumps for a process.
@@ -976,22 +998,15 @@ virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes)
struct rlimit rlim;
rlim.rlim_cur = rlim.rlim_max = bytes;
- if (pid == 0) {
- if (setrlimit(RLIMIT_CORE, &rlim) < 0) {
- virReportSystemError(errno,
- _("cannot limit core file size to %llu"),
- bytes);
- return -1;
- }
- } else {
- if (virProcessPrLimit(pid, RLIMIT_CORE, &rlim, NULL) < 0) {
- virReportSystemError(errno,
- _("cannot limit core file size "
- "of process %lld to %llu"),
- (long long int)pid, bytes);
- return -1;
- }
+
+ if (virProcessSetLimit(pid, RLIMIT_CORE, &rlim) < 0) {
+ virReportSystemError(errno,
+ _("cannot limit core file size "
+ "of process %lld to %llu"),
+ (long long int)pid, bytes);
+ return -1;
}
+
return 0;
}
#else /* ! (WITH_SETRLIMIT && defined(RLIMIT_CORE)) */
--
2.26.2
4:31 a.m.
New subject: [libvirt PATCH 04/17] util: Introduce virProcess{Get,Set}Limit()
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
These functions abstract part of the existing logic, which is
the same in all virProcessSetMax*() functions, and changes it
so that which underlying syscall is used depends on their
availability rather than on the context in which they are
called: since prlimit() and {g,s}etrlimit() have slightly
different requirements, using the same one every single time
should make for a more consistent experience.
As part of the change, we also remove the special case for
passing zero to virProcessSetMax*() functions: we have removed
all callers that depended on that functionality in the previous
commit, so this is now safe to do and makes the semantics
simpler.
This commit is better viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 175 +++++++++++++++++++++++-------------------
1 file changed, 95 insertions(+), 80 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index e01ff25540..38b248217e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -738,10 +738,66 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED,
}
#endif
+#if WITH_GETRLIMIT
+static int
+virProcessGetRLimit(int resource,
+ struct rlimit *old_limit)
+{
+ return getrlimit(resource, old_limit);
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
+static int
+virProcessSetRLimit(int resource,
+ const struct rlimit *new_limit)
+{
+ return setrlimit(resource, new_limit);
+}
+#endif /* WITH_SETRLIMIT */
+
+#if WITH_GETRLIMIT
Question of preference. I'd rather have these two WITH_GETRLIMIT
sections joined together.... [1]
+static int
+virProcessGetLimit(pid_t pid,
+ int resource,
+ struct rlimit *old_limit)
+{
+ pid_t current_pid = getpid();
+ bool same_process = (pid == current_pid);
+
+ if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0)
+ return 0;
+
+ if (same_process && virProcessGetRLimit(resource, old_limit) == 0)
+ return 0;
+
+ return -1;
+}
+#endif /* WITH_GETRLIMIT */
+
+#if WITH_SETRLIMIT
1: ... just like these two WITH_SETRLIMIT. But it's matter of taste, I
agree.
+static int
+virProcessSetLimit(pid_t pid,
+ int resource,
+ const struct rlimit *new_limit)
+{
+ pid_t current_pid = getpid();
+ bool same_process = (pid == current_pid);
+
+ if (virProcessPrLimit(pid, resource, new_limit, NULL) == 0)
+ return 0;
+
+ if (same_process && virProcessSetRLimit(resource, new_limit) == 0)
+ return 0;
+
+ return -1;
+}
+#endif /* WITH_SETRLIMIT */
+
Michal
1:13 p.m.
New subject: [libvirt PATCH 05/17] qemu: Make some minor tweaks
Doing this now will make the next changes nicer.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d3208e5203..1e1df6da64 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7013,10 +7013,13 @@ qemuProcessLaunch(virConnectPtr conn,
goto cleanup;
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
+ virCommandSetUmask(cmd, 0x002);
+
+ VIR_DEBUG("Setting up process limits");
+
virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
virCommandSetMaxFiles(cmd, cfg->maxFiles);
virCommandSetMaxCoreSize(cmd, cfg->maxCore);
- virCommandSetUmask(cmd, 0x002);
VIR_DEBUG("Setting up security labelling");
if (qemuSecuritySetChildProcessLabel(driver->securityManager,
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 06/17] qemu: Set all limits at the same time
qemuProcessLaunch() is the correct place to set process limits,
and in fact is where we were dealing with almost all of them,
but the memory locking limit was handled in
qemuBuildCommandLine() instead for some reason.
The code is rewritten so that the desired limit is calculated
and applied in separated steps, which will help with further
changes, but this doesn't alter the behavior.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 4 ----
src/qemu/qemu_process.c | 6 ++++++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f255b0f881..348e7488f7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10121,10 +10121,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
qemuBuildVsockCommandLine(cmd, def, def->vsock, qemuCaps) < 0)
return NULL;
- /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
- * significant amount of memory, so we need to set the limit accordingly */
- virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def, false));
-
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MSG_TIMESTAMP) &&
cfg->logTimestamp)
virCommandAddArgList(cmd, "-msg", "timestamp=on", NULL);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1e1df6da64..92e33707c0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6920,6 +6920,7 @@ qemuProcessLaunch(virConnectPtr conn,
g_autoptr(virQEMUDriverConfig) cfg = NULL;
size_t nnicindexes = 0;
g_autofree int *nicindexes = NULL;
+ unsigned long long maxMemLock = 0;
VIR_DEBUG("conn=%p driver=%p vm=%p name=%s if=%d asyncJob=%d "
"incoming.launchURI=%s incoming.deferredURI=%s "
@@ -7017,6 +7018,11 @@ qemuProcessLaunch(virConnectPtr conn,
VIR_DEBUG("Setting up process limits");
+ /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
+ * significant amount of memory, so we need to set the limit accordingly */
+ maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
+
+ virCommandSetMaxMemLock(cmd, maxMemLock);
virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
virCommandSetMaxFiles(cmd, cfg->maxFiles);
virCommandSetMaxCoreSize(cmd, cfg->maxCore);
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 07/17] util: Have virCommand remember whether limits are set
Currently this only happens for the core size, but we want the
behavior to be consistent for other limits as well.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/vircommand.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 3eef0767bb..044c5e0500 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -132,8 +132,11 @@ struct _virCommand {
bool reap;
bool rawStatus;
+ bool setMaxMemLock;
unsigned long long maxMemLock;
+ bool setMaxProcesses;
unsigned int maxProcesses;
+ bool setMaxFiles;
unsigned int maxFiles;
bool setMaxCore;
unsigned long long maxCore;
@@ -806,11 +809,14 @@ virExec(virCommandPtr cmd)
/* pidfilefd is intentionally leaked. */
}
- if (virProcessSetMaxMemLock(pid, cmd->maxMemLock) < 0)
+ if (cmd->setMaxMemLock &&
+ virProcessSetMaxMemLock(pid, cmd->maxMemLock) < 0)
goto fork_error;
- if (virProcessSetMaxProcesses(pid, cmd->maxProcesses) < 0)
+ if (cmd->setMaxProcesses &&
+ virProcessSetMaxProcesses(pid, cmd->maxProcesses) < 0)
goto fork_error;
- if (virProcessSetMaxFiles(pid, cmd->maxFiles) < 0)
+ if (cmd->setMaxFiles &&
+ virProcessSetMaxFiles(pid, cmd->maxFiles) < 0)
goto fork_error;
if (cmd->setMaxCore &&
virProcessSetMaxCoreSize(pid, cmd->maxCore) < 0)
@@ -1149,6 +1155,7 @@ virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long
bytes)
return;
cmd->maxMemLock = bytes;
+ cmd->setMaxMemLock = true;
}
void
@@ -1158,6 +1165,7 @@ virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs)
return;
cmd->maxProcesses = procs;
+ cmd->setMaxProcesses = true;
}
void
@@ -1167,6 +1175,7 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files)
return;
cmd->maxFiles = files;
+ cmd->setMaxFiles = true;
}
void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes)
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 08/17] qemu: Set limits only when explicitly asked to do so
The current code is written under the assumption that, for all
limits except the core size, asking for the limit to be set to
zero is a no-op, and so the operation is performed
unconditionally.
While this is the behavior we want for the QEMU driver, the
virCommand and virProcess facilities are generic, and should not
implement this kind of policy: asking for a limit to be set to
zero should result in that limit being set to zero every single
time.
Add some checks in the QEMU driver, effectively moving the
policy where it belongs.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 5 +++--
src/qemu/qemu_migration.c | 2 ++
src/qemu/qemu_process.c | 15 ++++++++++++---
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 17fa71c21b..54d8bd0d3a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9259,9 +9259,10 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
vm->original_memlock = 0;
}
- /* Trying to set the memory locking limit to zero is a no-op */
- if (virProcessSetMaxMemLock(vm->pid, bytes) < 0)
+ if (bytes > 0 &&
+ virProcessSetMaxMemLock(vm->pid, bytes) < 0) {
return -1;
+ }
return 0;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d7231f68ae..e44931dcfa 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2950,6 +2950,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
}
if (STREQ_NULLABLE(protocol, "rdma") &&
+ vm->def->mem.hard_limit > 0 &&
virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10)
< 0) {
goto stopjob;
}
@@ -4199,6 +4200,7 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
switch (spec->destType) {
case MIGRATION_DEST_HOST:
if (STREQ(spec->dest.host.protocol, "rdma") &&
+ vm->def->mem.hard_limit > 0 &&
virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit <<
10) < 0) {
goto exit_monitor;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 92e33707c0..c05cbe3570 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7022,9 +7022,18 @@ qemuProcessLaunch(virConnectPtr conn,
* significant amount of memory, so we need to set the limit accordingly */
maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
- virCommandSetMaxMemLock(cmd, maxMemLock);
- virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
- virCommandSetMaxFiles(cmd, cfg->maxFiles);
+ /* For all these settings, zero indicates that the limit should
+ * not be set explicitly and the default/inherited limit should
+ * be applied instead */
+ if (maxMemLock > 0)
+ virCommandSetMaxMemLock(cmd, maxMemLock);
+ if (cfg->maxProcesses > 0)
+ virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
+ if (cfg->maxFiles > 0)
+ virCommandSetMaxFiles(cmd, cfg->maxFiles);
+
+ /* In this case, however, zero means that core dumps should be
+ * disabled, and so we always need to set the limit explicitly */
virCommandSetMaxCoreSize(cmd, cfg->maxCore);
VIR_DEBUG("Setting up security labelling");
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 09/17] util: Don't special-case setting a limit to zero
This behavior reflects the needs of the QEMU driver and has no
place in a generic module such as virProcess.
Thanks to the changes made with the previous commit, it is now
safe to remove these checks and make all virProcessSetMax*()
functions finally behave the same way.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 38b248217e..abce522bf3 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -798,7 +798,7 @@ virProcessSetLimit(pid_t pid,
/**
* virProcessSetMaxMemLock:
* @pid: process to be changed
- * @bytes: new limit (0 for no change)
+ * @bytes: new limit
*
* Sets a new limit on the amount of locked memory for a process.
*
@@ -809,9 +809,6 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes)
{
struct rlimit rlim;
- if (bytes == 0)
- return 0;
-
/* We use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED internally to represent
* unlimited memory amounts, but setrlimit() and prlimit() use
* RLIM_INFINITY for the same purpose, so we need to translate between
@@ -898,7 +895,7 @@ virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED,
/**
* virProcessSetMaxProcesses:
* @pid: process to be changed
- * @procs: new limit (0 for no change)
+ * @procs: new limit
*
* Sets a new limit on the amount of processes for the user the
* process is running as.
@@ -910,9 +907,6 @@ virProcessSetMaxProcesses(pid_t pid, unsigned int procs)
{
struct rlimit rlim;
- if (procs == 0)
- return 0;
-
rlim.rlim_cur = rlim.rlim_max = procs;
if (virProcessSetLimit(pid, RLIMIT_NPROC, &rlim) < 0) {
@@ -938,7 +932,7 @@ virProcessSetMaxProcesses(pid_t pid G_GNUC_UNUSED,
/**
* virProcessSetMaxFiles:
* @pid: process to be changed
- * @files: new limit (0 for no change)
+ * @files: new limit
*
* Sets a new limit on the number of opened files for a process.
*
@@ -949,9 +943,6 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files)
{
struct rlimit rlim;
- if (files == 0)
- return 0;
-
/* Max number of opened files is one greater than actual limit. See
* man setrlimit.
*
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 10/17] conf: Rename original_memlock -> originalMemlock
That's more consistent with our usual naming convention.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.h | 4 ++--
src/qemu/qemu_domain.c | 10 +++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 930eed60de..7d208d881c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2807,8 +2807,8 @@ struct _virDomainObj {
size_t ndeprecations;
char **deprecations;
- unsigned long long original_memlock; /* Original RLIMIT_MEMLOCK, zero if no
- * restore will be required later */
+ unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no
+ * restore will be required later */
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainObj, virObjectUnref);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 54d8bd0d3a..73e2473dce 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9248,15 +9248,15 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
* 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 */
- if (!vm->original_memlock) {
- if (virProcessGetMaxMemLock(vm->pid, &(vm->original_memlock)) <
0)
- vm->original_memlock = 0;
+ if (!vm->originalMemlock) {
+ if (virProcessGetMaxMemLock(vm->pid, &(vm->originalMemlock)) <
0)
+ vm->originalMemlock = 0;
}
} else {
/* Once memory locking is no longer required, we can restore the
* original, usually very low, limit */
- bytes = vm->original_memlock;
- vm->original_memlock = 0;
+ bytes = vm->originalMemlock;
+ vm->originalMemlock = 0;
}
if (bytes > 0 &&
--
2.26.2
1:13 p.m.
New subject: [libvirt PATCH 11/17] tests: Mock virProcessGetMaxMemLock()
Up until now we've implicitly relied on the fact that failures
reported from this function were simply ignored, but that's
about to change and so we need a proper mock.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.h | 2 +-
tests/virprocessmock.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 34210d6c9d..dbf4148e90 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -79,7 +79,7 @@ int virProcessSetMaxProcesses(pid_t pid, unsigned int procs);
int virProcessSetMaxFiles(pid_t pid, unsigned int files);
int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes);
-int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes);
+int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes) G_GNUC_NO_INLINE;
/* Callback to run code within the mount namespace tied to the given
* pid. This function must use only async-signal-safe functions, as
diff --git a/tests/virprocessmock.c b/tests/virprocessmock.c
index c9386d757a..0356ff2f70 100644
--- a/tests/virprocessmock.c
+++ b/tests/virprocessmock.c
@@ -21,6 +21,13 @@
#include <config.h>
#include "virprocess.h"
+int
+virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long *bytes
G_GNUC_UNUSED)
+{
+ *bytes = 0;
+ return 0;
+}
+
int
virProcessSetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long bytes G_GNUC_UNUSED)
{
--
2.26.2
4:31 a.m.
New subject: [libvirt PATCH 11/17] tests: Mock virProcessGetMaxMemLock()
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
Up until now we've implicitly relied on the fact that failures
reported from this function were simply ignored, but that's
about to change and so we need a proper mock.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virprocess.h | 2 +-
tests/virprocessmock.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 34210d6c9d..dbf4148e90 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -79,7 +79,7 @@ int virProcessSetMaxProcesses(pid_t pid, unsigned int procs);
int virProcessSetMaxFiles(pid_t pid, unsigned int files);
int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes);
-int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes);
+int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes) G_GNUC_NO_INLINE;
/* Callback to run code within the mount namespace tied to the given
* pid. This function must use only async-signal-safe functions, as
diff --git a/tests/virprocessmock.c b/tests/virprocessmock.c
index c9386d757a..0356ff2f70 100644
--- a/tests/virprocessmock.c
+++ b/tests/virprocessmock.c
@@ -21,6 +21,13 @@
#include <config.h>
#include "virprocess.h"
+int
+virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long *bytes
G_GNUC_UNUSED)
Ehm, probably coffee hadn't kicked in? Because I can see @bytes used ..
+{
+ *bytes = 0;
.. right here :-D
+ return 0;
+}
+
int
virProcessSetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long bytes
G_GNUC_UNUSED)
{
Michal
7:32 a.m.
New subject: [libvirt PATCH 11/17] tests: Mock virProcessGetMaxMemLock()
On Mon, 2021-03-08 at 11:31 +0100, Michal Privoznik wrote:
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
> +int
> +virProcessGetMaxMemLock(pid_t pid G_GNUC_UNUSED, unsigned long long *bytes
G_GNUC_UNUSED)
Ehm, probably coffee hadn't kicked in? Because I can see @bytes used ..
> +{
> + *bytes = 0;
.. right here :-D
More like it had been too long since the last coffee, but yeah, of
course you're absolutely right :)
--
Andrea Bolognani / Red Hat / Virtualization
1:13 p.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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>
---
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
4:30 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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(a)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
7:38 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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
4:50 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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(a)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 :|
9:21 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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
9:23 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
On Mon, Mar 08, 2021 at 04:21:16PM +0100, Andrea Bolognani wrote:
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.
The our coding standard is that we only call virReportError if the
caller is actually going to honour the errors.
> > + 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.
Right, but we *must* always be consistent in whether a method uses
virReportError or not. We have two code paths here returning -1,
and only one of which reports errors. Either all must report
errors, or none. Since the only caller is ignoring errors, then
it looks like none should report errors.
> > + /* 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
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 :|
5:43 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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(a)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
6:47 a.m.
New subject: [libvirt PATCH 12/17] util: Try to get limits from /proc
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
1:14 p.m.
New subject: [libvirt PATCH 13/17] qemu: Don't ignore virProcessGetMaxMemLock() errors
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>
---
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
1:14 p.m.
New subject: [libvirt PATCH 14/17] qemu: Refactor qemuDomainAdjustMaxMemLock()
Store the current memory locking limit and the desired one
separately, which will help with later changes.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1e9178764b..f8b0e1a62a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9239,27 +9239,29 @@ int
qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
bool forceVFIO)
{
- unsigned long long bytes = 0;
+ unsigned long long currentMemLock = 0;
+ unsigned long long desiredMemLock = 0;
- bytes = qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO);
+ desiredMemLock = qemuDomainGetMemLockLimitBytes(vm->def, forceVFIO);
+ if (virProcessGetMaxMemLock(vm->pid, ¤tMemLock) < 0)
+ return -1;
- if (bytes) {
+ 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) {
- if (virProcessGetMaxMemLock(vm->pid, &(vm->originalMemlock)) <
0)
- return -1;
+ if (vm->originalMemlock == 0) {
+ vm->originalMemlock = currentMemLock;
}
} else {
/* Once memory locking is no longer required, we can restore the
* original, usually very low, limit */
- bytes = vm->originalMemlock;
+ desiredMemLock = vm->originalMemlock;
vm->originalMemlock = 0;
}
- if (bytes > 0 &&
- virProcessSetMaxMemLock(vm->pid, bytes) < 0) {
+ if (desiredMemLock > 0 &&
+ virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
return -1;
}
--
2.26.2
1:14 p.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
This will be useful when libvirtd is running in a containerized
environment with limited capabilities, and in order to make
things like VFIO device assignment still work an external
privileged process changes the limits from outside of the
container. KubeVirt is an example of this setup.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 12 ++++++++++++
src/qemu/qemu_conf.c | 4 ++++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
5 files changed, 19 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 3c1045858b..f1b024a37f 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -104,6 +104,7 @@ module Libvirtd_qemu =
| str_entry "slirp_helper"
| str_entry "dbus_daemon"
| bool_entry "set_process_name"
+ | bool_entry "external_limit_manager"
| int_entry "max_processes"
| int_entry "max_files"
| limits_entry "max_core"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 0c1054f198..15cbc3ba38 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -662,6 +662,18 @@
#
#set_process_name = 1
+# If enabled, libvirt will not attempt to change process limits (as
+# configured with the max_processes, max_files and max_core settings
+# below) itself but will instead expect an external entity to perform
+# this task.
+#
+# This also applies to the memory locking limit, which cannot be
+# configured here and is instead calculated dynamically based on the
+# exact guest configuration: if an external limit manager is in use,
+# then libvirt will merely check that the limit has been set
+# appropriately.
+#
+#external_limit_manager = 1
# If max_processes is set to a positive integer, libvirt will use
# it to set the maximum number of processes that can be run by qemu
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2bbc75024c..ee95c124dd 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -673,6 +673,10 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg,
if (virConfGetValueBool(conf, "set_process_name",
&cfg->setProcessName) < 0)
return -1;
+
+ if (virConfGetValueBool(conf, "external_limit_manager",
&cfg->externalLimitManager) < 0)
+ return -1;
+
if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses)
< 0)
return -1;
if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7025b5222e..15e0353253 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -176,6 +176,7 @@ struct _virQEMUDriverConfig {
bool nogfxAllowHostAudio;
bool setProcessName;
+ bool externalLimitManager;
unsigned int maxProcesses;
unsigned int maxFiles;
unsigned int maxThreadsPerProc;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 9310dcec1c..73be55febe 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -77,6 +77,7 @@ module Test_libvirtd_qemu =
{ "hugetlbfs_mount" = "/dev/hugepages" }
{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
{ "set_process_name" = "1" }
+{ "external_limit_manager" = "1" }
{ "max_processes" = "0" }
{ "max_files" = "0" }
{ "max_threads_per_process" = "0" }
--
2.26.2
4:30 a.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
On 3/5/21 8:14 PM, Andrea Bolognani wrote:
This will be useful when libvirtd is running in a containerized
environment with limited capabilities, and in order to make
things like VFIO device assignment still work an external
privileged process changes the limits from outside of the
container. KubeVirt is an example of this setup.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 12 ++++++++++++
src/qemu/qemu_conf.c | 4 ++++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
5 files changed, 19 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 3c1045858b..f1b024a37f 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -104,6 +104,7 @@ module Libvirtd_qemu =
| str_entry "slirp_helper"
| str_entry "dbus_daemon"
| bool_entry "set_process_name"
+ | bool_entry "external_limit_manager"
| int_entry "max_processes"
| int_entry "max_files"
| limits_entry "max_core"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 0c1054f198..15cbc3ba38 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -662,6 +662,18 @@
#
#set_process_name = 1
+# If enabled, libvirt will not attempt to change process limits (as
+# configured with the max_processes, max_files and max_core settings
+# below) itself but will instead expect an external entity to perform
+# this task.
+#
+# This also applies to the memory locking limit, which cannot be
+# configured here and is instead calculated dynamically based on the
+# exact guest configuration: if an external limit manager is in use,
+# then libvirt will merely check that the limit has been set
+# appropriately.
+#
+#external_limit_manager = 1
# If max_processes is set to a positive integer, libvirt will use
# it to set the maximum number of processes that can be run by qemu
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2bbc75024c..ee95c124dd 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -673,6 +673,10 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg,
if (virConfGetValueBool(conf, "set_process_name",
&cfg->setProcessName) < 0)
return -1;
+
+ if (virConfGetValueBool(conf, "external_limit_manager",
&cfg->externalLimitManager) < 0)
+ return -1;
+
if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses)
< 0)
return -1;
if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0)
I think we could error out if external_limit_manager is set and one of
these max_processes, max_files, max_core is set also. It's not really a
combination we can make sense of, is it? It's perfectly okay to write
that in a separate patch though.
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7025b5222e..15e0353253 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -176,6 +176,7 @@ struct _virQEMUDriverConfig {
bool nogfxAllowHostAudio;
bool setProcessName;
+ bool externalLimitManager;
unsigned int maxProcesses;
unsigned int maxFiles;
unsigned int maxThreadsPerProc;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 9310dcec1c..73be55febe 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -77,6 +77,7 @@ module Test_libvirtd_qemu =
{ "hugetlbfs_mount" = "/dev/hugepages" }
{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
{ "set_process_name" = "1" }
+{ "external_limit_manager" = "1" }
{ "max_processes" = "0" }
{ "max_files" = "0" }
{ "max_threads_per_process" = "0" }
Michal
4:52 a.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
On Fri, Mar 05, 2021 at 08:14:02PM +0100, Andrea Bolognani wrote:
This will be useful when libvirtd is running in a containerized
environment with limited capabilities, and in order to make
things like VFIO device assignment still work an external
privileged process changes the limits from outside of the
container. KubeVirt is an example of this setup.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 12 ++++++++++++
src/qemu/qemu_conf.c | 4 ++++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
5 files changed, 19 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 3c1045858b..f1b024a37f 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -104,6 +104,7 @@ module Libvirtd_qemu =
| str_entry "slirp_helper"
| str_entry "dbus_daemon"
| bool_entry "set_process_name"
+ | bool_entry "external_limit_manager"
| int_entry "max_processes"
| int_entry "max_files"
| limits_entry "max_core"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 0c1054f198..15cbc3ba38 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -662,6 +662,18 @@
#
#set_process_name = 1
+# If enabled, libvirt will not attempt to change process limits (as
+# configured with the max_processes, max_files and max_core settings
+# below) itself but will instead expect an external entity to perform
+# this task.
Can't users simply not set max_core, max_files, etc already ?
I think it is preferrable to have flags tailored specifically to
the individual limits, not a global flag. Otherwise you can end
up in a case where you want to disable the memory limits, but
keep the other limits set which is impossible with this global
flag.
+#
+# This also applies to the memory locking limit, which cannot be
+# configured here and is instead calculated dynamically based on the
+# exact guest configuration: if an external limit manager is in use,
+# then libvirt will merely check that the limit has been set
+# appropriately.
+#
+#external_limit_manager = 1
# If max_processes is set to a positive integer, libvirt will use
# it to set the maximum number of processes that can be run by qemu
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2bbc75024c..ee95c124dd 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -673,6 +673,10 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg,
if (virConfGetValueBool(conf, "set_process_name",
&cfg->setProcessName) < 0)
return -1;
+
+ if (virConfGetValueBool(conf, "external_limit_manager",
&cfg->externalLimitManager) < 0)
+ return -1;
+
if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses)
< 0)
return -1;
if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7025b5222e..15e0353253 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -176,6 +176,7 @@ struct _virQEMUDriverConfig {
bool nogfxAllowHostAudio;
bool setProcessName;
+ bool externalLimitManager;
unsigned int maxProcesses;
unsigned int maxFiles;
unsigned int maxThreadsPerProc;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 9310dcec1c..73be55febe 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -77,6 +77,7 @@ module Test_libvirtd_qemu =
{ "hugetlbfs_mount" = "/dev/hugepages" }
{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
{ "set_process_name" = "1" }
+{ "external_limit_manager" = "1" }
{ "max_processes" = "0" }
{ "max_files" = "0" }
{ "max_threads_per_process" = "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 :|
6:56 a.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
On Mon, 2021-03-08 at 10:52 +0000, Daniel P. Berrangé wrote:
On Fri, Mar 05, 2021 at 08:14:02PM +0100, Andrea Bolognani wrote:
> +# If enabled, libvirt will not attempt to change process limits (as
> +# configured with the max_processes, max_files and max_core settings
> +# below) itself but will instead expect an external entity to perform
> +# this task.
Can't users simply not set max_core, max_files, etc already ?
That works for things that are static and have a corresponding
configuration option in qemu.conf, but the memory locking limit is
dynamic, per-VM and needs to change as devices are added and removed
from the guest.
I think it is preferrable to have flags tailored specifically to
the individual limits, not a global flag. Otherwise you can end
up in a case where you want to disable the memory limits, but
keep the other limits set which is impossible with this global
flag.
Since what I'm interested in is the memory locking limit, I guess I
could turn this into
max_memlock_external = 1
or even
max_memlock = "external"
with "dynamic" being the other accepted value, which would be the
default and would behave as libvirt does today.
Do you think that would work better?
--
Andrea Bolognani / Red Hat / Virtualization
7:01 a.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
On Mon, Mar 08, 2021 at 01:56:15PM +0100, Andrea Bolognani wrote:
On Mon, 2021-03-08 at 10:52 +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 05, 2021 at 08:14:02PM +0100, Andrea Bolognani wrote:
> > +# If enabled, libvirt will not attempt to change process limits (as
> > +# configured with the max_processes, max_files and max_core settings
> > +# below) itself but will instead expect an external entity to perform
> > +# this task.
>
> Can't users simply not set max_core, max_files, etc already ?
That works for things that are static and have a corresponding
configuration option in qemu.conf, but the memory locking limit is
dynamic, per-VM and needs to change as devices are added and removed
from the guest.
> I think it is preferrable to have flags tailored specifically to
> the individual limits, not a global flag. Otherwise you can end
> up in a case where you want to disable the memory limits, but
> keep the other limits set which is impossible with this global
> flag.
Since what I'm interested in is the memory locking limit, I guess I
could turn this into
max_memlock_external = 1
or even
max_memlock = "external"
with "dynamic" being the other accepted value, which would be the
default and would behave as libvirt does today.
Do you think that would work better?
I think that would be better, as it has clearly defined scope which we
can maintain more accurately long term.
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 :|
7:48 a.m.
New subject: [libvirt PATCH 15/17] qemu: Add external_limit_manager config knob
On 3/8/21 11:52 AM, Daniel P. Berrangé wrote:
On Fri, Mar 05, 2021 at 08:14:02PM +0100, Andrea Bolognani wrote:
> This will be useful when libvirtd is running in a containerized
> environment with limited capabilities, and in order to make
> things like VFIO device assignment still work an external
> privileged process changes the limits from outside of the
> container. KubeVirt is an example of this setup.
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf | 12 ++++++++++++
> src/qemu/qemu_conf.c | 4 ++++
> src/qemu/qemu_conf.h | 1 +
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 5 files changed, 19 insertions(+)
>
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 3c1045858b..f1b024a37f 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -104,6 +104,7 @@ module Libvirtd_qemu =
> | str_entry "slirp_helper"
> | str_entry "dbus_daemon"
> | bool_entry "set_process_name"
> + | bool_entry "external_limit_manager"
> | int_entry "max_processes"
> | int_entry "max_files"
> | limits_entry "max_core"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 0c1054f198..15cbc3ba38 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -662,6 +662,18 @@
> #
> #set_process_name = 1
>
> +# If enabled, libvirt will not attempt to change process limits (as
> +# configured with the max_processes, max_files and max_core settings
> +# below) itself but will instead expect an external entity to perform
> +# this task.
Can't users simply not set max_core, max_files, etc already ?
These two yes, mem lock no.
Michal
1:14 p.m.
New subject: [libvirt PATCH 16/17] qemu: Wire up external limit manager
When the config knob is enabled, we simply skip the part where
limits are set; for the memory locking limit, which can change
dynamically over the lifetime of the guest, we still make sure
that the external process has set it correctly and error out
if that turns out not to be the case
This commit is better viewed with 'git show -w'.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++--------------
src/qemu/qemu_process.c | 44 +++++++++++++++++++++++------------------
3 files changed, 50 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7d208d881c..d1333020e1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2807,6 +2807,7 @@ struct _virDomainObj {
size_t ndeprecations;
char **deprecations;
+ bool externalLimitManager; /* Whether process limits are handled outside of libvirt
*/
unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no
* restore will be required later */
};
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f8b0e1a62a..0d9adb2f9c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9246,23 +9246,32 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
if (virProcessGetMaxMemLock(vm->pid, ¤tMemLock) < 0)
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 (!vm->externalLimitManager) {
+ 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;
+ }
+ } else {
+ /* Once memory locking is no longer required, we can restore the
+ * original, usually very low, limit */
+ desiredMemLock = vm->originalMemlock;
+ vm->originalMemlock = 0;
}
- } else {
- /* Once memory locking is no longer required, we can restore the
- * original, usually very low, limit */
- desiredMemLock = vm->originalMemlock;
- vm->originalMemlock = 0;
- }
- if (desiredMemLock > 0 &&
- virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
- return -1;
+ if (desiredMemLock > 0 &&
+ virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
+ return -1;
+ }
+ } else {
+ if (currentMemLock < desiredMemLock) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("insufficient memlock limit (%llu < %llu)"),
+ currentMemLock, desiredMemLock);
+ return -1;
+ }
}
return 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c05cbe3570..2eac3934c7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7016,25 +7016,31 @@ qemuProcessLaunch(virConnectPtr conn,
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
virCommandSetUmask(cmd, 0x002);
- VIR_DEBUG("Setting up process limits");
-
- /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
- * significant amount of memory, so we need to set the limit accordingly */
- maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
-
- /* For all these settings, zero indicates that the limit should
- * not be set explicitly and the default/inherited limit should
- * be applied instead */
- if (maxMemLock > 0)
- virCommandSetMaxMemLock(cmd, maxMemLock);
- if (cfg->maxProcesses > 0)
- virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
- if (cfg->maxFiles > 0)
- virCommandSetMaxFiles(cmd, cfg->maxFiles);
-
- /* In this case, however, zero means that core dumps should be
- * disabled, and so we always need to set the limit explicitly */
- virCommandSetMaxCoreSize(cmd, cfg->maxCore);
+ if (cfg->externalLimitManager) {
+ VIR_DEBUG("Not setting up process limits (handled externally)");
+
+ vm->externalLimitManager = true;
+ } else {
+ VIR_DEBUG("Setting up process limits");
+
+ /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
+ * significant amount of memory, so we need to set the limit accordingly */
+ maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
+
+ /* For all these settings, zero indicates that the limit should
+ * not be set explicitly and the default/inherited limit should
+ * be applied instead */
+ if (maxMemLock > 0)
+ virCommandSetMaxMemLock(cmd, maxMemLock);
+ if (cfg->maxProcesses > 0)
+ virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
+ if (cfg->maxFiles > 0)
+ virCommandSetMaxFiles(cmd, cfg->maxFiles);
+
+ /* In this case, however, zero means that core dumps should be
+ * disabled, and so we always need to set the limit explicitly */
+ virCommandSetMaxCoreSize(cmd, cfg->maxCore);
+ }
VIR_DEBUG("Setting up security labelling");
if (qemuSecuritySetChildProcessLabel(driver->securityManager,
--
2.26.2
4:30 a.m.
New subject: [libvirt PATCH 16/17] qemu: Wire up external limit manager
On 3/5/21 8:14 PM, Andrea Bolognani wrote:
When the config knob is enabled, we simply skip the part where
limits are set; for the memory locking limit, which can change
dynamically over the lifetime of the guest, we still make sure
that the external process has set it correctly and error out
if that turns out not to be the case
This commit is better viewed with 'git show -w'.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++--------------
src/qemu/qemu_process.c | 44 +++++++++++++++++++++++------------------
3 files changed, 50 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7d208d881c..d1333020e1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2807,6 +2807,7 @@ struct _virDomainObj {
size_t ndeprecations;
char **deprecations;
+ bool externalLimitManager; /* Whether process limits are handled outside of libvirt
*/
unsigned long long originalMemlock; /* Original RLIMIT_MEMLOCK, zero if no
* restore will be required later */
};
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f8b0e1a62a..0d9adb2f9c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9246,23 +9246,32 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm,
if (virProcessGetMaxMemLock(vm->pid, ¤tMemLock) < 0)
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 (!vm->externalLimitManager) {
+ 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;
+ }
+ } else {
+ /* Once memory locking is no longer required, we can restore the
+ * original, usually very low, limit */
+ desiredMemLock = vm->originalMemlock;
+ vm->originalMemlock = 0;
}
- } else {
- /* Once memory locking is no longer required, we can restore the
- * original, usually very low, limit */
- desiredMemLock = vm->originalMemlock;
- vm->originalMemlock = 0;
- }
- if (desiredMemLock > 0 &&
- virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
- return -1;
+ if (desiredMemLock > 0 &&
+ virProcessSetMaxMemLock(vm->pid, desiredMemLock) < 0) {
+ return -1;
+ }
+ } else {
+ if (currentMemLock < desiredMemLock) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("insufficient memlock limit (%llu < %llu)"),
+ currentMemLock, desiredMemLock);
Should we go this way? Our limit computation is nothing but a guess
(even though it might be more educated than others). I think we should
reduce this to a warning. User had chosen not to set any limits and
might have tracked down (e.g. via strace) the actual value needed which
may be smaller than our guess.
Another reason is that if domain is started with a VFIO/mdev device then
no memlock is set at all but here, during hotplug it would be.
+ return -1;
+ }
}
return 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c05cbe3570..2eac3934c7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7016,25 +7016,31 @@ qemuProcessLaunch(virConnectPtr conn,
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
virCommandSetUmask(cmd, 0x002);
- VIR_DEBUG("Setting up process limits");
-
- /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
- * significant amount of memory, so we need to set the limit accordingly */
- maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
-
- /* For all these settings, zero indicates that the limit should
- * not be set explicitly and the default/inherited limit should
- * be applied instead */
- if (maxMemLock > 0)
- virCommandSetMaxMemLock(cmd, maxMemLock);
- if (cfg->maxProcesses > 0)
- virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
- if (cfg->maxFiles > 0)
- virCommandSetMaxFiles(cmd, cfg->maxFiles);
-
- /* In this case, however, zero means that core dumps should be
- * disabled, and so we always need to set the limit explicitly */
- virCommandSetMaxCoreSize(cmd, cfg->maxCore);
+ if (cfg->externalLimitManager) {
+ VIR_DEBUG("Not setting up process limits (handled externally)");
+
+ vm->externalLimitManager = true;
+ } else {
+ VIR_DEBUG("Setting up process limits");
+
+ /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
+ * significant amount of memory, so we need to set the limit accordingly */
+ maxMemLock = qemuDomainGetMemLockLimitBytes(vm->def, false);
+
+ /* For all these settings, zero indicates that the limit should
+ * not be set explicitly and the default/inherited limit should
+ * be applied instead */
+ if (maxMemLock > 0)
+ virCommandSetMaxMemLock(cmd, maxMemLock);
+ if (cfg->maxProcesses > 0)
+ virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
+ if (cfg->maxFiles > 0)
+ virCommandSetMaxFiles(cmd, cfg->maxFiles);
+
+ /* In this case, however, zero means that core dumps should be
+ * disabled, and so we always need to set the limit explicitly */
+ virCommandSetMaxCoreSize(cmd, cfg->maxCore);
+ }
VIR_DEBUG("Setting up security labelling");
if (qemuSecuritySetChildProcessLabel(driver->securityManager,
Michal
1:14 p.m.
New subject: [libvirt PATCH 17/17] news: Document external limit manager feature
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
NEWS.rst | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst
index bd40373a80..3a3e3962c2 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -13,6 +13,16 @@ v7.2.0 (unreleased)
* **New features**
+ * qemu: Allow process limits to be managed by an external process
+
+ Whereas in traditional virtualization scenarios libvirt takes full
+ reponsibility of setting up the environment for a QEMU process, when it's
+ used as part of things like KubeVirt some of these tasks are delegated to
+ external, more privileged processes.
+
+ The ``external_limit_manager`` feature can now be enabled in ``qemu.conf``
+ to tell libvirt that it shouldn't attempt to set process limits itself.
+
* **Improvements**
* **Bug fixes**
--
2.26.2
4:30 a.m.
On 3/5/21 8:13 PM, Andrea Bolognani wrote:
This feature has been requested by KubeVirt developers and will make
it possible for them to make some VFIO-related features, such as
migration and hotplug, work correctly.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
The first part of the series, especially the first 9 patches, is
preparation work: it addresses a few annoying issues with our APIs
that deal with process limits, and makes them all nice, consistent
and easy to reason about while moving policy code from the generic
code to the QEMU driver where it belongs.
Andrea Bolognani (17):
util: Document limit-related functions
util: Simplify stubs
util: Always pass a pid to virProcessSetMax*()
util: Introduce virProcess{Get,Set}Limit()
qemu: Make some minor tweaks
qemu: Set all limits at the same time
util: Have virCommand remember whether limits are set
qemu: Set limits only when explicitly asked to do so
util: Don't special-case setting a limit to zero
conf: Rename original_memlock -> originalMemlock
tests: Mock virProcessGetMaxMemLock()
util: Try to get limits from /proc
qemu: Don't ignore virProcessGetMaxMemLock() errors
qemu: Refactor qemuDomainAdjustMaxMemLock()
qemu: Add external_limit_manager config knob
qemu: Wire up external limit manager
news: Document external limit manager feature
NEWS.rst | 10 +
src/conf/domain_conf.h | 5 +-
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 12 +
src/qemu/qemu_command.c | 4 -
src/qemu/qemu_conf.c | 4 +
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_domain.c | 47 ++--
src/qemu/qemu_migration.c | 2 +
src/qemu/qemu_process.c | 30 ++-
src/qemu/test_libvirtd_qemu.aug.in | 1 +
src/util/vircommand.c | 21 +-
src/util/virprocess.c | 340 ++++++++++++++++++++---------
src/util/virprocess.h | 2 +-
tests/virprocessmock.c | 7 +
15 files changed, 354 insertions(+), 133 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
4:54 a.m.
On Fri, Mar 05, 2021 at 08:13:47PM +0100, Andrea Bolognani wrote:
This feature has been requested by KubeVirt developers and will make
it possible for them to make some VFIO-related features, such as
migration and hotplug, work correctly.
https://bugzilla.redhat.com/show_bug.cgi?id=1916346
The first part of the series, especially the first 9 patches, is
preparation work: it addresses a few annoying issues with our APIs
that deal with process limits, and makes them all nice, consistent
and easy to reason about while moving policy code from the generic
code to the QEMU driver where it belongs.
IIUC, the code was already supposed to attempt to set the limits
and gracefully carry on if it was unable todo so. Can you outline
the actual problem being solved, as wading through the bug comments
to understand the precise detail of the problem is not very clear.
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 :|
7:11 a.m.
On Mon, 2021-03-08 at 10:54 +0000, Daniel P. Berrangé wrote:
On Fri, Mar 05, 2021 at 08:13:47PM +0100, Andrea Bolognani wrote:
> This feature has been requested by KubeVirt developers and will make
> it possible for them to make some VFIO-related features, such as
> migration and hotplug, work correctly.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1916346
>
> The first part of the series, especially the first 9 patches, is
> preparation work: it addresses a few annoying issues with our APIs
> that deal with process limits, and makes them all nice, consistent
> and easy to reason about while moving policy code from the generic
> code to the QEMU driver where it belongs.
IIUC, the code was already supposed to attempt to set the limits
and gracefully carry on if it was unable todo so. Can you outline
the actual problem being solved, as wading through the bug comments
to understand the precise detail of the problem is not very clear.
Not being able to set the memory locking limit shouldn't be a soft
failure, because that might cause QEMU to start up apparently fine
and then error out later on during the lifetime of the VM, which is
much worse than not starting up at all. We're actually doing this
correctly for the most part, which is why hotplug requests for VFIO
devices in KubeVirt result in errors under certain conditions.
The reason why VFIO device assignment is currently not completely
broken in KubeVirt is that, when the QEMU process is initially
started, we set the memory locking limit after fork(), so we can do
that using setrlimit() which doesn't require additional capabilities,
but in the hotplug scenario libvirtd needs to change the limits of a
different process: in that case we are forced to use prlimit(), which
fails due to the lack of CAP_SYS_RESOURCE.
--
Andrea Bolognani / Red Hat / Virtualization
7:17 a.m.
On Mon, Mar 08, 2021 at 02:11:56PM +0100, Andrea Bolognani wrote:
On Mon, 2021-03-08 at 10:54 +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 05, 2021 at 08:13:47PM +0100, Andrea Bolognani wrote:
> > This feature has been requested by KubeVirt developers and will make
> > it possible for them to make some VFIO-related features, such as
> > migration and hotplug, work correctly.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1916346
> >
> > The first part of the series, especially the first 9 patches, is
> > preparation work: it addresses a few annoying issues with our APIs
> > that deal with process limits, and makes them all nice, consistent
> > and easy to reason about while moving policy code from the generic
> > code to the QEMU driver where it belongs.
>
> IIUC, the code was already supposed to attempt to set the limits
> and gracefully carry on if it was unable todo so. Can you outline
> the actual problem being solved, as wading through the bug comments
> to understand the precise detail of the problem is not very clear.
Not being able to set the memory locking limit shouldn't be a soft
failure, because that might cause QEMU to start up apparently fine
and then error out later on during the lifetime of the VM, which is
much worse than not starting up at all. We're actually doing this
correctly for the most part, which is why hotplug requests for VFIO
devices in KubeVirt result in errors under certain conditions.
Yep, I agree we shoudln't have it be a mere warning. IIUC, we would
try to set the limit, and if we fail, then we query the current limit
to see if its satisfactory and report a fatal error if not.
The reason why VFIO device assignment is currently not completely
broken in KubeVirt is that, when the QEMU process is initially
started, we set the memory locking limit after fork(), so we can do
that using setrlimit() which doesn't require additional capabilities,
but in the hotplug scenario libvirtd needs to change the limits of a
different process: in that case we are forced to use prlimit(), which
fails due to the lack of CAP_SYS_RESOURCE.
Since you added code to parse existing limits from /proc, I'm wondering
if we can just do without the config option. Simply try to use prlimit
and if it fails, query existing limits to determine if we sould treat
the prlimit as fatal or ignore it. Overall I'd prefer libvirt to
"just work" out of the box rather than requiring people to know about
setting a "make-vfio-hotplug-work=yes" flag in the config file.
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 :|
9:32 a.m.
On Mon, 2021-03-08 at 13:17 +0000, Daniel P. Berrangé wrote:
On Mon, Mar 08, 2021 at 02:11:56PM +0100, Andrea Bolognani wrote:
> The reason why VFIO device assignment is currently not completely
> broken in KubeVirt is that, when the QEMU process is initially
> started, we set the memory locking limit after fork(), so we can do
> that using setrlimit() which doesn't require additional capabilities,
> but in the hotplug scenario libvirtd needs to change the limits of a
> different process: in that case we are forced to use prlimit(), which
> fails due to the lack of CAP_SYS_RESOURCE.
Since you added code to parse existing limits from /proc, I'm wondering
if we can just do without the config option. Simply try to use prlimit
and if it fails, query existing limits to determine if we sould treat
the prlimit as fatal or ignore it. Overall I'd prefer libvirt to
"just work" out of the box rather than requiring people to know about
setting a "make-vfio-hotplug-work=yes" flag in the config file.
The problem with that approach is what to do when *lowering* the
limit, for example as a consequence of hot-unplugging the last VFIO
device from the VM.
If we're controlling the memory locking limit ourselves, then failure
to lower it should be an error, because leaving the limit much higher
than necessary creates potential for DoS by a compromised QEMU; on
the other hand, if the limit is controlled by an external process,
all we can really do is assume they will do the right thing after
hot-unplugging has happened.
I don't think discoverability is too much of an issue, as anyone who
needs to use this option will already have needed to figure out a lot
more in order to effectively take over memory locking limit
management responsibilities from libvirt...
--
Andrea Bolognani / Red Hat / Virtualization
9:57 a.m.
On Mon, Mar 08, 2021 at 04:32:26PM +0100, Andrea Bolognani wrote:
On Mon, 2021-03-08 at 13:17 +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 08, 2021 at 02:11:56PM +0100, Andrea Bolognani wrote:
> > The reason why VFIO device assignment is currently not completely
> > broken in KubeVirt is that, when the QEMU process is initially
> > started, we set the memory locking limit after fork(), so we can do
> > that using setrlimit() which doesn't require additional capabilities,
> > but in the hotplug scenario libvirtd needs to change the limits of a
> > different process: in that case we are forced to use prlimit(), which
> > fails due to the lack of CAP_SYS_RESOURCE.
>
> Since you added code to parse existing limits from /proc, I'm wondering
> if we can just do without the config option. Simply try to use prlimit
> and if it fails, query existing limits to determine if we sould treat
> the prlimit as fatal or ignore it. Overall I'd prefer libvirt to
> "just work" out of the box rather than requiring people to know about
> setting a "make-vfio-hotplug-work=yes" flag in the config file.
The problem with that approach is what to do when *lowering* the
limit, for example as a consequence of hot-unplugging the last VFIO
device from the VM.
If we're controlling the memory locking limit ourselves, then failure
to lower it should be an error, because leaving the limit much higher
than necessary creates potential for DoS by a compromised QEMU; on
the other hand, if the limit is controlled by an external process,
all we can really do is assume they will do the right thing after
hot-unplugging has happened.
IMHO once QEMU vCPUs start running, immediately assume QEMU is
compromised / hostile. IOW, the DoS risk arrived the moment it
was given the higher limit. We're just failing to close off the
existing risk we've already accepted, which doesn't worry me much.
On unplug the only thing we actually do when memory lock reduce
fails is to log a warning message, it is never treated as a
fatal error.
So the only difference is whether we skip the warning message
when we get EPERM from prlimit(), or always emit the warning.
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 :|
11:28 a.m.
On Mon, 2021-03-08 at 15:57 +0000, Daniel P. Berrangé wrote:
On Mon, Mar 08, 2021 at 04:32:26PM +0100, Andrea Bolognani wrote:
> On Mon, 2021-03-08 at 13:17 +0000, Daniel P. Berrangé wrote:
> > Since you added code to parse existing limits from /proc, I'm wondering
> > if we can just do without the config option. Simply try to use prlimit
> > and if it fails, query existing limits to determine if we sould treat
> > the prlimit as fatal or ignore it. Overall I'd prefer libvirt to
> > "just work" out of the box rather than requiring people to know
about
> > setting a "make-vfio-hotplug-work=yes" flag in the config file.
>
> The problem with that approach is what to do when *lowering* the
> limit, for example as a consequence of hot-unplugging the last VFIO
> device from the VM.
>
> If we're controlling the memory locking limit ourselves, then failure
> to lower it should be an error, because leaving the limit much higher
> than necessary creates potential for DoS by a compromised QEMU; on
> the other hand, if the limit is controlled by an external process,
> all we can really do is assume they will do the right thing after
> hot-unplugging has happened.
IMHO once QEMU vCPUs start running, immediately assume QEMU is
compromised / hostile. IOW, the DoS risk arrived the moment it
was given the higher limit. We're just failing to close off the
existing risk we've already accepted, which doesn't worry me much.
On unplug the only thing we actually do when memory lock reduce
fails is to log a warning message, it is never treated as a
fatal error.
So the only difference is whether we skip the warning message
when we get EPERM from prlimit(), or always emit the warning.
You're right, we're currently just soft-failing when we can't lower
the memlock limit on unplug. Given this and your assessment of the
security implications, which I trust, we should indeed be able to
avoid introducing the qemu.conf knob and just behave sanely in all
scenarios out of the box. I'll give it a try.
--
Andrea Bolognani / Red Hat / Virtualization
1354
days inactive
1358
days old
42 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik