5:31 a.m.
I'm trying to fix SGX patches I reviewed earlier [1] and these would
simplify my attempts.
1: https://listman.redhat.com/archives/libvir-list/2022-July/232679.html
Michal Prívozník (2):
qemu_cgroup: Avoid ternary operator when setting @deviceACL
qemu_cgroup: Introduce qemuCgroupAllowDevicesPaths()
src/qemu/qemu_cgroup.c | 54 +++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 17 deletions(-)
--
2.35.1
5:31 a.m.
New subject: [PATCH 1/2] qemu_cgroup: Avoid ternary operator when setting @deviceACL
Inside of the qemuSetupDevicesCgroup() there's @deviceACL
variable, which points to a string list of devices that are
allowed in devices controller by default. This list can either
come from qemu.conf (cfg->cgroupDeviceACL) or from a builtin
@defaultDeviceACL. However, a multiline ternary operator is used
when setting the variable which is against our coding style.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 2cc16a69d3..e012ba92c0 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -652,7 +652,7 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
{
qemuDomainObjPrivate *priv = vm->privateData;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
- const char *const *deviceACL = NULL;
+ const char *const *deviceACL = (const char *const *) cfg->cgroupDeviceACL;
int rv = -1;
size_t i;
@@ -686,9 +686,8 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
if (rv < 0)
return -1;
- deviceACL = cfg->cgroupDeviceACL ?
- (const char *const *)cfg->cgroupDeviceACL :
- defaultDeviceACL;
+ if (!deviceACL)
+ deviceACL = defaultDeviceACL;
if (vm->def->nsounds &&
((!vm->def->ngraphics && cfg->nogfxAllowHostAudio) ||
--
2.35.1
7:40 a.m.
New subject: [PATCH 1/2] qemu_cgroup: Avoid ternary operator when setting @deviceACL
On Thu, Jul 21, 2022 at 12:31:41PM +0200, Michal Privoznik wrote:
Inside of the qemuSetupDevicesCgroup() there's @deviceACL
variable, which points to a string list of devices that are
allowed in devices controller by default. This list can either
come from qemu.conf (cfg->cgroupDeviceACL) or from a builtin
@defaultDeviceACL. However, a multiline ternary operator is used
when setting the variable which is against our coding style.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
5:31 a.m.
New subject: [PATCH 2/2] qemu_cgroup: Introduce qemuCgroupAllowDevicesPaths()
We have qemuCgroupAllowDevicePath() which sets up devices
controller for just one path. And if we have more paths we have
to call it in a loop. So far, we have just one such place, but
soon we'll have another one (for SGX memory). Separate the loop
into its own function so that it can be reused.
And while at it, move setting the default set of devices as the
first thing, right after all devices are disallowed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 51 +++++++++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e012ba92c0..8339caeb53 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -67,6 +67,32 @@ qemuCgroupAllowDevicePath(virDomainObj *vm,
}
+static int
+qemuCgroupAllowDevicesPaths(virDomainObj *vm,
+ const char *const *deviceACL,
+ int perms,
+ bool ignoreEacces)
+{
+ size_t i;
+
+ for (i = 0; deviceACL[i] != NULL; i++) {
+ int rv;
+
+ if (!virFileExists(deviceACL[i])) {
+ VIR_DEBUG("Ignoring non-existent device %s", deviceACL[i]);
+ continue;
+ }
+
+ rv = qemuCgroupAllowDevicePath(vm, deviceACL[i], perms, ignoreEacces);
+ if (rv < 0 &&
+ !virLastErrorIsSystemErrno(ENOENT))
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
qemuCgroupDenyDevicePath(virDomainObj *vm,
const char *path,
@@ -659,6 +685,10 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
+ deviceACL = cfg->cgroupDeviceACL ?
+ (const char *const *)cfg->cgroupDeviceACL :
+ defaultDeviceACL;
+
rv = virCgroupDenyAllDevices(priv->cgroup);
virDomainAuditCgroup(vm, priv->cgroup, "deny", "all", rv ==
0);
if (rv < 0) {
@@ -671,6 +701,12 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
return -1;
}
+ if (!deviceACL)
+ deviceACL = defaultDeviceACL;
+
+ if (qemuCgroupAllowDevicesPaths(vm, deviceACL, VIR_CGROUP_DEVICE_RW, false) < 0)
+ return -1;
+
if (qemuSetupFirmwareCgroup(vm) < 0)
return -1;
@@ -686,9 +722,6 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
if (rv < 0)
return -1;
- if (!deviceACL)
- deviceACL = defaultDeviceACL;
-
if (vm->def->nsounds &&
((!vm->def->ngraphics && cfg->nogfxAllowHostAudio) ||
(vm->def->graphics &&
@@ -703,18 +736,6 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
return -1;
}
- for (i = 0; deviceACL[i] != NULL; i++) {
- if (!virFileExists(deviceACL[i])) {
- VIR_DEBUG("Ignoring non-existent device %s", deviceACL[i]);
- continue;
- }
-
- rv = qemuCgroupAllowDevicePath(vm, deviceACL[i], VIR_CGROUP_DEVICE_RW, false);
- if (rv < 0 &&
- !virLastErrorIsSystemErrno(ENOENT))
- return -1;
- }
-
if (virDomainChrDefForeach(vm->def,
true,
qemuSetupChardevCgroupCB,
--
2.35.1
5:45 a.m.
New subject: [PATCH 2/2] qemu_cgroup: Introduce qemuCgroupAllowDevicesPaths()
On 7/21/22 12:31, Michal Privoznik wrote:
We have qemuCgroupAllowDevicePath() which sets up devices
controller for just one path. And if we have more paths we have
to call it in a loop. So far, we have just one such place, but
soon we'll have another one (for SGX memory). Separate the loop
into its own function so that it can be reused.
And while at it, move setting the default set of devices as the
first thing, right after all devices are disallowed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 51 +++++++++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e012ba92c0..8339caeb53 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -67,6 +67,32 @@ qemuCgroupAllowDevicePath(virDomainObj *vm,
}
+static int
+qemuCgroupAllowDevicesPaths(virDomainObj *vm,
+ const char *const *deviceACL,
+ int perms,
+ bool ignoreEacces)
+{
+ size_t i;
+
+ for (i = 0; deviceACL[i] != NULL; i++) {
+ int rv;
+
+ if (!virFileExists(deviceACL[i])) {
+ VIR_DEBUG("Ignoring non-existent device %s", deviceACL[i]);
+ continue;
+ }
+
+ rv = qemuCgroupAllowDevicePath(vm, deviceACL[i], perms, ignoreEacces);
+ if (rv < 0 &&
+ !virLastErrorIsSystemErrno(ENOENT))
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
qemuCgroupDenyDevicePath(virDomainObj *vm,
const char *path,
@@ -659,6 +685,10 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
+ deviceACL = cfg->cgroupDeviceACL ?
+ (const char *const *)cfg->cgroupDeviceACL :
+ defaultDeviceACL;
+
OOOps, this hunk does not belong here. I've screwed up conflict
resolution. Consider fixed locally.
Michal
7:40 a.m.
New subject: [PATCH 2/2] qemu_cgroup: Introduce qemuCgroupAllowDevicesPaths()
On Thu, Jul 21, 2022 at 12:45:29PM +0200, Michal Prívozník wrote:
On 7/21/22 12:31, Michal Privoznik wrote:
> We have qemuCgroupAllowDevicePath() which sets up devices
> controller for just one path. And if we have more paths we have
> to call it in a loop. So far, we have just one such place, but
> soon we'll have another one (for SGX memory). Separate the loop
> into its own function so that it can be reused.
>
> And while at it, move setting the default set of devices as the
> first thing, right after all devices are disallowed.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_cgroup.c | 51 +++++++++++++++++++++++++++++-------------
> 1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index e012ba92c0..8339caeb53 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -67,6 +67,32 @@ qemuCgroupAllowDevicePath(virDomainObj *vm,
> }
>
>
> +static int
> +qemuCgroupAllowDevicesPaths(virDomainObj *vm,
> + const char *const *deviceACL,
> + int perms,
> + bool ignoreEacces)
> +{
> + size_t i;
> +
> + for (i = 0; deviceACL[i] != NULL; i++) {
> + int rv;
> +
> + if (!virFileExists(deviceACL[i])) {
> + VIR_DEBUG("Ignoring non-existent device %s", deviceACL[i]);
> + continue;
> + }
> +
> + rv = qemuCgroupAllowDevicePath(vm, deviceACL[i], perms, ignoreEacces);
> + if (rv < 0 &&
> + !virLastErrorIsSystemErrno(ENOENT))
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> qemuCgroupDenyDevicePath(virDomainObj *vm,
> const char *path,
> @@ -659,6 +685,10 @@ qemuSetupDevicesCgroup(virDomainObj *vm)
> if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
> return 0;
>
> + deviceACL = cfg->cgroupDeviceACL ?
> + (const char *const *)cfg->cgroupDeviceACL :
> + defaultDeviceACL;
> +
OOOps, this hunk does not belong here. I've screwed up conflict
resolution. Consider fixed locally.
With this hunk removed
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
890
days inactive
890
days old
5 comments
3 participants
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Michal Prívozník