[PATCH] migration: cpr socket permissions fix

[PATCH] migration: cpr socket permissions fix

[Ben Chaney]

Fix an issue where cpr transfer fails when running with the
-run-with user=$USERID with a permission denied error. This issue
occurs because the destination host creates the transfer sockets before
it drops permissions while the source host tries to connect after
dropping permissions. Fix this by changing the ownership of the cpr
socket to the lower level so it is accessible to both parties.

Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
Signed-off-by: Ben Chaney <bchaney@akamai.com>
---
 include/system/os-posix.h |  1 +
 include/system/os-wasm.h  |  1 +
 include/system/os-win32.h |  1 +
 os-posix.c                | 12 ++++++++++++
 stubs/meson.build         |  1 +
 stubs/os-file.c           |  6 ++++++
 util/qemu-sockets.c       |  6 ++++++
 7 files changed, 28 insertions(+)
 create mode 100644 stubs/os-file.c

diff --git a/include/system/os-posix.h b/include/system/os-posix.h
index ce5b3bccf8..e4b69fc316 100644
--- a/include/system/os-posix.h
+++ b/include/system/os-posix.h
@@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
 void os_setup_limits(void);
 void os_setup_post(void);
 int os_mlock(bool on_fault);
+void os_set_socket_permissions(const char *path);
 
 /**
  * qemu_alloc_stack:
diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
index 3abb3aaa03..eeac092183 100644
--- a/include/system/os-wasm.h
+++ b/include/system/os-wasm.h
@@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
 };
 bool is_daemonized(void);
 static inline void os_daemonize(void) {}
+static inline void os_set_socket_permissions(const char *dummy) {};
 
 /**
  * qemu_alloc_stack:
diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index 22d72babdf..79e42ec297 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
 static inline void os_set_proc_name(const char *dummy) {}
 void os_set_line_buffering(void);
 void os_setup_early_signal_handling(void);
+static inline void os_set_socket_permissions(const char *dummy) {};
 
 int getpagesize(void);
 
diff --git a/os-posix.c b/os-posix.c
index 52925c23d3..bbd17ff2b9 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
 static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
 static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
 
+void os_set_socket_permissions(const char *path)
+{
+    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
+    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
+
+    if (chown(path, uid, gid) < 0) {
+        error_report("Failed to chown socket %s: %s", path, strerror(errno));
+        exit(1);
+    }
+}
+
+
 /*
  * Prepare to change user ID. user_id can be one of 3 forms:
  *   - a username, in which case user ID will be changed to its uid,
diff --git a/stubs/meson.build b/stubs/meson.build
index 0b2778c568..4a4342344b 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -24,6 +24,7 @@ if have_block
   stub_ss.add(files('ram-block.c'))
   stub_ss.add(files('runstate-check.c'))
   stub_ss.add(files('uuid.c'))
+  stub_ss.add(files('os-file.c'))
 endif
 
 if have_block or have_ga
diff --git a/stubs/os-file.c b/stubs/os-file.c
new file mode 100644
index 0000000000..c32cbc7efa
--- /dev/null
+++ b/stubs/os-file.c
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include "qemu/osdep.h"
+
+void os_set_socket_permissions(const char *dummy)
+{
+}
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4773755fd5..77b2d9287b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
         goto err;
     }
+    /*
+     * Change the permissions on the socket to match the permission of the
+     * counterparty process
+     */
+    os_set_socket_permissions(un.sun_path);
+
     if (listen(sock, num) < 0) {
         error_setg_errno(errp, errno, "Failed to listen on socket");
         goto err;
-- 
2.34.1

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Thu, Nov 20, 2025 at 01:57:33PM -0500, Ben Chaney wrote:
> Fix an issue where cpr transfer fails when running with the
> -run-with user=$USERID with a permission denied error. This issue
> occurs because the destination host creates the transfer sockets before
> it drops permissions while the source host tries to connect after
> dropping permissions. Fix this by changing the ownership of the cpr
> socket to the lower level so it is accessible to both parties.
> 
> Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
> Signed-off-by: Ben Chaney <bchaney@akamai.com>
> ---
>  include/system/os-posix.h |  1 +
>  include/system/os-wasm.h  |  1 +
>  include/system/os-win32.h |  1 +
>  os-posix.c                | 12 ++++++++++++
>  stubs/meson.build         |  1 +
>  stubs/os-file.c           |  6 ++++++
>  util/qemu-sockets.c       |  6 ++++++
>  7 files changed, 28 insertions(+)
>  create mode 100644 stubs/os-file.c
> 
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index ce5b3bccf8..e4b69fc316 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
>  void os_setup_limits(void);
>  void os_setup_post(void);
>  int os_mlock(bool on_fault);
> +void os_set_socket_permissions(const char *path);
>  
>  /**
>   * qemu_alloc_stack:
> diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
> index 3abb3aaa03..eeac092183 100644
> --- a/include/system/os-wasm.h
> +++ b/include/system/os-wasm.h
> @@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
>  };
>  bool is_daemonized(void);
>  static inline void os_daemonize(void) {}
> +static inline void os_set_socket_permissions(const char *dummy) {};
>  
>  /**
>   * qemu_alloc_stack:
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index 22d72babdf..79e42ec297 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
>  static inline void os_set_proc_name(const char *dummy) {}
>  void os_set_line_buffering(void);
>  void os_setup_early_signal_handling(void);
> +static inline void os_set_socket_permissions(const char *dummy) {};
>  
>  int getpagesize(void);
>  
> diff --git a/os-posix.c b/os-posix.c
> index 52925c23d3..bbd17ff2b9 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
>  static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
>  static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
>  
> +void os_set_socket_permissions(const char *path)
> +{
> +    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
> +    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
> +
> +    if (chown(path, uid, gid) < 0) {
> +        error_report("Failed to chown socket %s: %s", path, strerror(errno));
> +        exit(1);
> +    }
> +}
> +
> +
>  /*
>   * Prepare to change user ID. user_id can be one of 3 forms:
>   *   - a username, in which case user ID will be changed to its uid,
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 0b2778c568..4a4342344b 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -24,6 +24,7 @@ if have_block
>    stub_ss.add(files('ram-block.c'))
>    stub_ss.add(files('runstate-check.c'))
>    stub_ss.add(files('uuid.c'))
> +  stub_ss.add(files('os-file.c'))
>  endif
>  
>  if have_block or have_ga
> diff --git a/stubs/os-file.c b/stubs/os-file.c
> new file mode 100644
> index 0000000000..c32cbc7efa
> --- /dev/null
> +++ b/stubs/os-file.c
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include "qemu/osdep.h"
> +
> +void os_set_socket_permissions(const char *dummy)
> +{
> +}
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 4773755fd5..77b2d9287b 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
>          goto err;
>      }
> +    /*
> +     * Change the permissions on the socket to match the permission of the
> +     * counterparty process
> +     */
> +    os_set_socket_permissions(un.sun_path);

QEMU's -run-with described user= as:

  user=username or user=uid:gid can be used to drop root privileges before
  starting guest execution. QEMU will use the setuid and setgid system
  calls to switch to the specified identity...

So it explicitly mentioned "before starting guest execution", hence at
least from that doc it's hard to say if unix socket should be created with
the privilege or after dropped..  Here I believe cpr socket should be the
case for latter, however when it's a generic change to unix listening
ports, I wonder if there's other unwanted side effects.

Considering unix socket itself doesn't really have a UID attached to it,
it's only the unix path that needs a chmod(), meanwhile the mgmt of course
knows both the right UID (as specified in -run-with) and the path, would it
make sense if the mgmt chmod() after it starts dest QEMU?  That'll reduce
the scope of impact to minimum.

Thanks,

> +
>      if (listen(sock, num) < 0) {
>          error_setg_errno(errp, errno, "Failed to listen on socket");
>          goto err;
> -- 
> 2.34.1
> 

-- 
Peter Xu

Re: [PATCH] migration: cpr socket permissions fix

[Daniel P. Berrangé]

On Fri, Nov 21, 2025 at 11:07:48AM -0500, Peter Xu wrote:
> On Thu, Nov 20, 2025 at 01:57:33PM -0500, Ben Chaney wrote:
> > Fix an issue where cpr transfer fails when running with the
> > -run-with user=$USERID with a permission denied error. This issue
> > occurs because the destination host creates the transfer sockets before
> > it drops permissions while the source host tries to connect after
> > dropping permissions. Fix this by changing the ownership of the cpr
> > socket to the lower level so it is accessible to both parties.
> > 
> > Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
> > Signed-off-by: Ben Chaney <bchaney@akamai.com>
> > ---
> >  include/system/os-posix.h |  1 +
> >  include/system/os-wasm.h  |  1 +
> >  include/system/os-win32.h |  1 +
> >  os-posix.c                | 12 ++++++++++++
> >  stubs/meson.build         |  1 +
> >  stubs/os-file.c           |  6 ++++++
> >  util/qemu-sockets.c       |  6 ++++++
> >  7 files changed, 28 insertions(+)
> >  create mode 100644 stubs/os-file.c
> > 
> > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > index ce5b3bccf8..e4b69fc316 100644
> > --- a/include/system/os-posix.h
> > +++ b/include/system/os-posix.h
> > @@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
> >  void os_setup_limits(void);
> >  void os_setup_post(void);
> >  int os_mlock(bool on_fault);
> > +void os_set_socket_permissions(const char *path);
> >  
> >  /**
> >   * qemu_alloc_stack:
> > diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
> > index 3abb3aaa03..eeac092183 100644
> > --- a/include/system/os-wasm.h
> > +++ b/include/system/os-wasm.h
> > @@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
> >  };
> >  bool is_daemonized(void);
> >  static inline void os_daemonize(void) {}
> > +static inline void os_set_socket_permissions(const char *dummy) {};
> >  
> >  /**
> >   * qemu_alloc_stack:
> > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > index 22d72babdf..79e42ec297 100644
> > --- a/include/system/os-win32.h
> > +++ b/include/system/os-win32.h
> > @@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
> >  static inline void os_set_proc_name(const char *dummy) {}
> >  void os_set_line_buffering(void);
> >  void os_setup_early_signal_handling(void);
> > +static inline void os_set_socket_permissions(const char *dummy) {};
> >  
> >  int getpagesize(void);
> >  
> > diff --git a/os-posix.c b/os-posix.c
> > index 52925c23d3..bbd17ff2b9 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
> >  static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
> >  static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
> >  
> > +void os_set_socket_permissions(const char *path)
> > +{
> > +    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
> > +    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
> > +
> > +    if (chown(path, uid, gid) < 0) {
> > +        error_report("Failed to chown socket %s: %s", path, strerror(errno));
> > +        exit(1);
> > +    }
> > +}
> > +
> > +
> >  /*
> >   * Prepare to change user ID. user_id can be one of 3 forms:
> >   *   - a username, in which case user ID will be changed to its uid,
> > diff --git a/stubs/meson.build b/stubs/meson.build
> > index 0b2778c568..4a4342344b 100644
> > --- a/stubs/meson.build
> > +++ b/stubs/meson.build
> > @@ -24,6 +24,7 @@ if have_block
> >    stub_ss.add(files('ram-block.c'))
> >    stub_ss.add(files('runstate-check.c'))
> >    stub_ss.add(files('uuid.c'))
> > +  stub_ss.add(files('os-file.c'))
> >  endif
> >  
> >  if have_block or have_ga
> > diff --git a/stubs/os-file.c b/stubs/os-file.c
> > new file mode 100644
> > index 0000000000..c32cbc7efa
> > --- /dev/null
> > +++ b/stubs/os-file.c
> > @@ -0,0 +1,6 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#include "qemu/osdep.h"
> > +
> > +void os_set_socket_permissions(const char *dummy)
> > +{
> > +}
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 4773755fd5..77b2d9287b 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
> >          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> >          goto err;
> >      }
> > +    /*
> > +     * Change the permissions on the socket to match the permission of the
> > +     * counterparty process
> > +     */
> > +    os_set_socket_permissions(un.sun_path);
> 
> QEMU's -run-with described user= as:
> 
>   user=username or user=uid:gid can be used to drop root privileges before
>   starting guest execution. QEMU will use the setuid and setgid system
>   calls to switch to the specified identity...
> 
> So it explicitly mentioned "before starting guest execution", hence at
> least from that doc it's hard to say if unix socket should be created with
> the privilege or after dropped..  Here I believe cpr socket should be the
> case for latter, however when it's a generic change to unix listening
> ports, I wonder if there's other unwanted side effects.
> 
> Considering unix socket itself doesn't really have a UID attached to it,
> it's only the unix path that needs a chmod(), meanwhile the mgmt of course
> knows both the right UID (as specified in -run-with) and the path, would it
> make sense if the mgmt chmod() after it starts dest QEMU?  That'll reduce
> the scope of impact to minimum.

My guiding principal is that the behaviour of cold-plugging a backend
on the CLI and hot-plugging a backend via QMP should be the same, since
we want to eventually get to a point where everything can be configured
via QMP and zero CLI except for basic process related things.

hot-plugging will honour -run-with setting, therefore cold-plugging
should do the same.

Having said that, the run-with setting impl is very clearcut however
that it is intended to run *after* all cold-plug setup is complete.
IOW, it is inherantly intending to have differnt behaviour for cold
plug vs hotplug.

There's a very real risk we'll cause regressions if we change how
permissions are handled implicitly on any backends.

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

Re: [PATCH] migration: cpr socket permissions fix

[Chaney, Ben]

> Considering unix socket itself doesn't really have a UID attached to it,
> it's only the unix path that needs a chmod(), meanwhile the mgmt of course
> knows both the right UID (as specified in -run-with) and the path, would it
> make sense if the mgmt chmod() after it starts dest QEMU? That'll reduce
> the scope of impact to minimum.


I like this solution, but it runs into the issue that the main channel socket is not
created ahead of time, so there isn't an opportunity for the management layer
to change it's permissions. The CPR socket is created ahead of time, so we could
use this solution there. I'm not familiar with the history here. Do you know why
the sockets are created at different times?

Thanks,
        Ben

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Thu, Dec 04, 2025 at 08:05:59PM +0000, Chaney, Ben wrote:
> > Considering unix socket itself doesn't really have a UID attached to it,
> > it's only the unix path that needs a chmod(), meanwhile the mgmt of course
> > knows both the right UID (as specified in -run-with) and the path, would it
> > make sense if the mgmt chmod() after it starts dest QEMU? That'll reduce
> > the scope of impact to minimum.
> 
> 
> I like this solution, but it runs into the issue that the main channel socket is not
> created ahead of time, so there isn't an opportunity for the management layer
> to change it's permissions. The CPR socket is created ahead of time, so we could
> use this solution there. I'm not familiar with the history here. Do you know why
> the sockets are created at different times?

IIUC the cpr channel isn't created ahead of time either, it's still created
a while after QEMU process start to run.  It's just that I believe CPR
needs to synchronously wait for the client to connect first and send data,
before it can reach other part of QEMU logic to further create the main
channel.

It should look like this:

qemu_init
  cpr_state_load
    cpr_transfer_input
      qio_net_listener_wait_client [1]
  qmp_x_exit_preconfig
    qmp_migrate_incoming [2]
  os_setup_post
    change_process_uid [3]

So IIUC you're looking for [2] creating the other unix socket.

Maybe you can stick with -incoming defer, then it'll be after step [3],
which will inherit the modified uid, and mgmt doesn't need to bother
monitoring.

Thanks,

-- 
Peter Xu

Re: [PATCH] migration: cpr socket permissions fix

[Chaney, Ben]

On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:


> Maybe you can stick with -incoming defer, then it'll be after step [3],
> which will inherit the modified uid, and mgmt doesn't need to bother
> monitoring.

I tried this approach, but It doesn't look like it is possible to create the
cprsocket later with -incoming defer.

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Mon, Dec 08, 2025 at 07:32:41PM +0000, Chaney, Ben wrote:
> 
> On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:
> 
> 
> > Maybe you can stick with -incoming defer, then it'll be after step [3],
> > which will inherit the modified uid, and mgmt doesn't need to bother
> > monitoring.
> 
> I tried this approach, but It doesn't look like it is possible to create the
> cprsocket later with -incoming defer.

You'll still need to chmod for the cpr socket.  "defer" will still help the
main channel to be created with the uid provided.

-- 
Peter Xu

Re: [PATCH] migration: cpr socket permissions fix

[Chaney, Ben]

On 12/9/25, 1:55 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:

> On Mon, Dec 08, 2025 at 07:32:41PM +0000, Chaney, Ben wrote:
> >
> > On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>> wrote:
> >
> >
> > > Maybe you can stick with -incoming defer, then it'll be after step [3],
> > > which will inherit the modified uid, and mgmt doesn't need to bother
> > > monitoring.
> >
> > I tried this approach, but It doesn't look like it is possible to create the
> > cprsocket later with -incoming defer.
>
>
> You'll still need to chmod for the cpr socket. "defer" will still help the
> main channel to be created with the uid provided.

Thanks for the pointers. I was able to get the incoming defer method
working, but it has much worse performance than the other method.

Would you be open to a solution where we chown only the migration
sockets, or would that run into similar concerns?

Thanks,
        Ben

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Thu, Dec 11, 2025 at 06:42:05PM +0000, Chaney, Ben wrote:
> 
> 
> On 12/9/25, 1:55 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:
> 
> > On Mon, Dec 08, 2025 at 07:32:41PM +0000, Chaney, Ben wrote:
> > >
> > > On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>> wrote:
> > >
> > >
> > > > Maybe you can stick with -incoming defer, then it'll be after step [3],
> > > > which will inherit the modified uid, and mgmt doesn't need to bother
> > > > monitoring.
> > >
> > > I tried this approach, but It doesn't look like it is possible to create the
> > > cprsocket later with -incoming defer.
> >
> >
> > You'll still need to chmod for the cpr socket. "defer" will still help the
> > main channel to be created with the uid provided.
> 
> Thanks for the pointers. I was able to get the incoming defer method
> working, but it has much worse performance than the other method.
> 
> Would you be open to a solution where we chown only the migration
> sockets, or would that run into similar concerns?

We can evaluate, but before that, could you explain your current solution
first?

And, what is the performance you mentioned here that is worse?

I at least didn't expect it to be downtime, because IIUC what your mgmt
needs to do is to chmod on the cpr channel first (during which migration
hasn't started), then chmod once more on the main channel after CPR channel
migrated and before main channel migration happens (during which VM should
be running on src), hence it should have nothing to do with downtime.

Thanks,

-- 
Peter Xu

Re: [PATCH] migration: cpr socket permissions fix

[Chaney, Ben]

> On 12/11/25, 2:13 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:

> On Thu, Dec 11, 2025 at 06:42:05PM +0000, Chaney, Ben wrote:
> >
> >
> > On 12/9/25, 1:55 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>> wrote:
> > >
> > > On Mon, Dec 08, 2025 at 07:32:41PM +0000, Chaney, Ben wrote:
> > >
> > > > On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>> <mailto:peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>>> wrote:
> > > >
> > > >
> > > > > Maybe you can stick with -incoming defer, then it'll be after step [3],
> > > > > which will inherit the modified uid, and mgmt doesn't need to bother
> > > > > monitoring.
> > > >
> > > > I tried this approach, but It doesn't look like it is possible to create the
> > > > cprsocket later with -incoming defer.
> > >
> > >
> > > You'll still need to chmod for the cpr socket. "defer" will still help the
> > > main channel to be created with the uid provided.
> >
> > Thanks for the pointers. I was able to get the incoming defer method
> > working, but it has much worse performance than the other method.
> >
> > Would you be open to a solution where we chown only the migration
> > sockets, or would that run into similar concerns?
>
>
> We can evaluate, but before that, could you explain your current solution
> first?
>
>
> And, what is the performance you mentioned here that is worse?
>
>
> I at least didn't expect it to be downtime, because IIUC what your mgmt
> needs to do is to chmod on the cpr channel first (during which migration
> hasn't started), then chmod once more on the main channel after CPR channel
> migrated and before main channel migration happens (during which VM should
> be running on src), hence it should have nothing to do with downtime.

I wouldn't have expected this to affect downtime either, but it does increase the
downtime by about 3.5 seconds (700-800ms to just over 4s). I am using the
following setup to defer the creation of the main socket:

qemu-system-x86_64 ... -incoming defer -incoming \
'{"channel-type": "cpr", "addr": { "transport": "socket", "type": "unix", "path": "cpr.sock"}}'

chown $UID:$GID cpr.sock

        echo '{"execute":"qmp_capabilities"}
{"execute": "query-status"}
{"execute":"migrate-set-parameters",
   "arguments":{"mode":"cpr-transfer"}}
{"execute": "migrate", "arguments": { "channels": [
    {"channel-type": "main", "addr": { "transport": "socket", "type": "unix",
               "path": "main.sock"}},
{"channel-type": "cpr",
     "addr": { "transport": "socket", "type": "unix",
               "path": "cpr.sock"}}]}}

{"execute": "query-status"}

{"execute": "query-migrate"}
' | $SSH_COMMAND socat STDIO unix-connect:qemu_src.monitor

echo '{"execute":"qmp_capabilities"}
{"execute": "migrate-incoming", "arguments": { "channels": [
    {"channel-type": "main", "addr": { "transport": "socket", "type": "unix",
               "path": "main.sock"}}]}}
{"execute": "query-status"}
{"execute": "query-migrate"}
' | $SSH_COMMAND socat STDIO unix-connect:qemu_dst.monitor

The migration finishes as soon as the migrate-incoming command is issued.
There is no opportunity to chown the main socket, but because it is being
hot plugged it gets created with the appropriate permissions.

I should also note that I am testing this in combination with the patch set for
 cpr transfer for tap devices, which makes the issue more pronounced in terms
of network interruption, however the reported downtime increases by 3.5s
regardless of if that patch set is applied or not.

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Thu, Dec 11, 2025 at 08:44:27PM +0000, Chaney, Ben wrote:
> 
> 
> > On 12/11/25, 2:13 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com>> wrote:
> 
> > On Thu, Dec 11, 2025 at 06:42:05PM +0000, Chaney, Ben wrote:
> > >
> > >
> > > On 12/9/25, 1:55 PM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>> wrote:
> > > >
> > > > On Mon, Dec 08, 2025 at 07:32:41PM +0000, Chaney, Ben wrote:
> > > >
> > > > > On 12/5/25, 10:13 AM, "Peter Xu" <peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>> <mailto:peterx@redhat.com <mailto:peterx@redhat.com> <mailto:peterx@redhat.com <mailto:peterx@redhat.com>>>> wrote:
> > > > >
> > > > >
> > > > > > Maybe you can stick with -incoming defer, then it'll be after step [3],
> > > > > > which will inherit the modified uid, and mgmt doesn't need to bother
> > > > > > monitoring.
> > > > >
> > > > > I tried this approach, but It doesn't look like it is possible to create the
> > > > > cprsocket later with -incoming defer.
> > > >
> > > >
> > > > You'll still need to chmod for the cpr socket. "defer" will still help the
> > > > main channel to be created with the uid provided.
> > >
> > > Thanks for the pointers. I was able to get the incoming defer method
> > > working, but it has much worse performance than the other method.
> > >
> > > Would you be open to a solution where we chown only the migration
> > > sockets, or would that run into similar concerns?
> >
> >
> > We can evaluate, but before that, could you explain your current solution
> > first?
> >
> >
> > And, what is the performance you mentioned here that is worse?
> >
> >
> > I at least didn't expect it to be downtime, because IIUC what your mgmt
> > needs to do is to chmod on the cpr channel first (during which migration
> > hasn't started), then chmod once more on the main channel after CPR channel
> > migrated and before main channel migration happens (during which VM should
> > be running on src), hence it should have nothing to do with downtime.
> 
> I wouldn't have expected this to affect downtime either, but it does increase the
> downtime by about 3.5 seconds (700-800ms to just over 4s). I am using the
> following setup to defer the creation of the main socket:
> 
> qemu-system-x86_64 ... -incoming defer -incoming \
> '{"channel-type": "cpr", "addr": { "transport": "socket", "type": "unix", "path": "cpr.sock"}}'
> 
> chown $UID:$GID cpr.sock
> 
>         echo '{"execute":"qmp_capabilities"}
> {"execute": "query-status"}
> {"execute":"migrate-set-parameters",
>    "arguments":{"mode":"cpr-transfer"}}
> {"execute": "migrate", "arguments": { "channels": [
>     {"channel-type": "main", "addr": { "transport": "socket", "type": "unix",
>                "path": "main.sock"}},
> {"channel-type": "cpr",
>      "addr": { "transport": "socket", "type": "unix",
>                "path": "cpr.sock"}}]}}
> 
> {"execute": "query-status"}
> 
> {"execute": "query-migrate"}
> ' | $SSH_COMMAND socat STDIO unix-connect:qemu_src.monitor
> 
> echo '{"execute":"qmp_capabilities"}
> {"execute": "migrate-incoming", "arguments": { "channels": [
>     {"channel-type": "main", "addr": { "transport": "socket", "type": "unix",
>                "path": "main.sock"}}]}}
> {"execute": "query-status"}
> {"execute": "query-migrate"}
> ' | $SSH_COMMAND socat STDIO unix-connect:qemu_dst.monitor
> 
> The migration finishes as soon as the migrate-incoming command is issued.

This really sounds weird, because this window should be the maximum
downtime..  if it finished so fast, something is wrong.

Could you spend some time investigate this problem?  IMHO something was
very off, a few seconds of downtime shouldn't be hard to chase.

If we need to justify a chmod on top of migration channels, we still need
to know why it's needed.

Thanks,

> There is no opportunity to chown the main socket, but because it is being
> hot plugged it gets created with the appropriate permissions.
> 
> I should also note that I am testing this in combination with the patch set for
>  cpr transfer for tap devices, which makes the issue more pronounced in terms
> of network interruption, however the reported downtime increases by 3.5s
> regardless of if that patch set is applied or not.

-- 
Peter Xu