Userfaultfd is by default allowed only for privileged processes. Since
libvirt runs QEMU unprivileged, we need to enable unprivileged access to
userfaultfd before starting post-copy migration.

Rather than providing a static sysctl configuration file, we set the
sysctl knob in runtime once post-copy migration is requested. This way
unprivileged_userfaultfd is only enabled once actually used.

https://bugzilla.redhat.com/show_bug.cgi?id=1945420

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---

Notes:
    Version 2:
    - setting unprivileged_userfaultfd only when it is not already enabled
    - virReportSystemError replaced with VIR_WARN

 src/qemu/qemu_migration_params.c | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index dbc3219826..9ba4811242 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -804,6 +804,41 @@ qemuMigrationCapsToJSON(virBitmap *caps,
 }
 
 
+/**
+ * qemuMigrationParamsEnableUserfaultfd
+ *
+ * Try to enable unprivileged userfaultfd unless it's missing or already
+ * enabled. Only a warning is logged when we cannot enable it, QEMU will
+ * report an error when enabling post-copy migration capability.
+ */
+static void
+qemuMigrationParamsEnableUserfaultfd(void)
+{
+    const char *sysctl = "/proc/sys/vm/unprivileged_userfaultfd";
+    g_autofree char *buf = NULL;
+
+    if (!virFileExists(sysctl))
+        return;
+
+    if (virFileReadAll(sysctl, 10, &buf) < 0) {
+        VIR_WARN("Cannot read unprivileged userfaultfd state");
+        return;
+    }
+
+    if (STREQ(buf, "1\n")) {
+        VIR_DEBUG("Unprivileged userfaultfd already enabled");
+        return;
+    }
+
+    VIR_DEBUG("Enabling unprivileged userfaultfd for post-copy migration");
+
+    if (virFileWriteStr(sysctl, "1", 0) < 0) {
+        VIR_WARN("Failed to enable unprivileged userfaultfd: %s",
+                 g_strerror(errno));
+    }
+}
+
+
 /**
  * qemuMigrationParamsApply
  * @driver: qemu driver
@@ -839,6 +874,13 @@ qemuMigrationParamsApply(virQEMUDriver *driver,
             goto cleanup;
         }
     } else {
+        /* userfaultfd may only be enabled for privileged processes by default,
+         * we need to make sure QEMU can use it before enabling post-copy
+         * migration */
+        if (virBitmapIsBitSet(priv->migrationCaps, QEMU_MIGRATION_CAP_POSTCOPY) &&
+            virBitmapIsBitSet(migParams->caps, QEMU_MIGRATION_CAP_POSTCOPY))
+            qemuMigrationParamsEnableUserfaultfd();
+
         if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps)))
             goto cleanup;
 
-- 
2.34.1

On Tue, Dec 07, 2021 at 10:19:42 +0100, Jiri Denemark wrote:
> Userfaultfd is by default allowed only for privileged processes. Since
> libvirt runs QEMU unprivileged, we need to enable unprivileged access to
> userfaultfd before starting post-copy migration.
> 
> Rather than providing a static sysctl configuration file, we set the
> sysctl knob in runtime once post-copy migration is requested. This way
> unprivileged_userfaultfd is only enabled once actually used.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1945420
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
> 
> Notes:
>     Version 2:
>     - setting unprivileged_userfaultfd only when it is not already enabled
>     - virReportSystemError replaced with VIR_WARN
> 
>  src/qemu/qemu_migration_params.c | 42 ++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
> index dbc3219826..9ba4811242 100644
> --- a/src/qemu/qemu_migration_params.c
> +++ b/src/qemu/qemu_migration_params.c

[...]

> @@ -839,6 +874,13 @@ qemuMigrationParamsApply(virQEMUDriver *driver,
>              goto cleanup;
>          }
>      } else {
> +        /* userfaultfd may only be enabled for privileged processes by default,
> +         * we need to make sure QEMU can use it before enabling post-copy
> +         * migration */
> +        if (virBitmapIsBitSet(priv->migrationCaps, QEMU_MIGRATION_CAP_POSTCOPY) &&
> +            virBitmapIsBitSet(migParams->caps, QEMU_MIGRATION_CAP_POSTCOPY))
> +            qemuMigrationParamsEnableUserfaultfd();

I forgot to be grumpy about doing sysfs writes in a function which is
sending stuff to qemu. It feels really misplaced.

Since I don't have a better idea and don't feel like digging deeper:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Tue, Dec 07, 2021 at 10:19:42AM +0100, Jiri Denemark wrote:
> Userfaultfd is by default allowed only for privileged processes. Since
> libvirt runs QEMU unprivileged, we need to enable unprivileged access to
> userfaultfd before starting post-copy migration.
> 
> Rather than providing a static sysctl configuration file, we set the
> sysctl knob in runtime once post-copy migration is requested. This way
> unprivileged_userfaultfd is only enabled once actually used.

I'm really not a fan of silently changing sysctl knobs on the
fly like this, as it means the change is essentially invisible
to the host admin.

IIUC, the kernel change was made because of fear of risk this
feature exposes to the kernel when combined with other flaws.

Now I don't know how valid that fear is, but given that starting
point, I think if we're going to change it, then the change ought
to be visible to admins in a fairly obvious way.

IOW, we something ought to be droppping a file into /etc/sysctl.d/
that enables it. The downside then is that it applies to all installs,
even if they don't migrate. The flipside is that a default of 1 has
been the historical value since postcopy first arrived, so all QEMU
installs always had this behaviour.

If we drop in a file  50-qemu-postcopy.conf, someone else can drop
in a file 55-turn-it-off-again.conf to override our default.

Stil this all feels so awful every way I look at it :-(

> +/**
> + * qemuMigrationParamsEnableUserfaultfd
> + *
> + * Try to enable unprivileged userfaultfd unless it's missing or already
> + * enabled. Only a warning is logged when we cannot enable it, QEMU will
> + * report an error when enabling post-copy migration capability.
> + */
> +static void
> +qemuMigrationParamsEnableUserfaultfd(void)
> +{
> +    const char *sysctl = "/proc/sys/vm/unprivileged_userfaultfd";
> +    g_autofree char *buf = NULL;
> +
> +    if (!virFileExists(sysctl))
> +        return;
> +
> +    if (virFileReadAll(sysctl, 10, &buf) < 0) {
> +        VIR_WARN("Cannot read unprivileged userfaultfd state");
> +        return;
> +    }
> +
> +    if (STREQ(buf, "1\n")) {
> +        VIR_DEBUG("Unprivileged userfaultfd already enabled");
> +        return;
> +    }
> +
> +    VIR_DEBUG("Enabling unprivileged userfaultfd for post-copy migration");
> +
> +    if (virFileWriteStr(sysctl, "1", 0) < 0) {
> +        VIR_WARN("Failed to enable unprivileged userfaultfd: %s",
> +                 g_strerror(errno));

Why only a warning - surely we know it is going to fail at this
point, and QEMU will probably give an obcure EPERM error, while
we're in a position to tell the user exactly what's wrong with
the sysctl.


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 :|