[PATCH 0/3] qemu: fix validation of 'fd' passing for backup job and improve debugability of passed FDs
Peter Krempa (3): qemuBackupPrepare: Actually allow 'VIR_STORAGE_NET_HOST_TRANS_FD' docs: backup: Hint at proper selinux labelling of the FD-passed NBD socket qemu: fd: Log information about passed file descriptor docs/formatbackup.rst | 4 +++ src/qemu/qemu_backup.c | 4 ++- src/qemu/qemu_fd.c | 62 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> While I've actually implemented support for FD passing the NBD server socket in eb768a556db I managed to misplace the hunk allowing the 'FD' transport in the validation code, rendering the whole feature useless. Fix the validation logic to make the feature useful. Fixes: eb768a556db75040f7b518d198a18bd0f5d6faad Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index f6ee31dc2a..fb3558d280 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -86,8 +86,10 @@ qemuBackupPrepare(virDomainBackupDef *def) /* TODO: Do we need to mess with selinux? */ break; - case VIR_STORAGE_NET_HOST_TRANS_RDMA: case VIR_STORAGE_NET_HOST_TRANS_FD: + break; + + case VIR_STORAGE_NET_HOST_TRANS_RDMA: case VIR_STORAGE_NET_HOST_TRANS_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected transport in <domainbackup>")); -- 2.49.0
On a Monday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
While I've actually implemented support for FD passing the NBD server socket in eb768a556db I managed to misplace the hunk allowing the 'FD' transport in the validation code, rendering the whole feature useless.
Fix the validation logic to make the feature useful.
s/useful/usable/?
Fixes: eb768a556db75040f7b518d198a18bd0f5d6faad
The commit that misplaced the case is: commit ee49106dbf6f3c716f0e31c9ecc9bf528eb72bb2 conf: Introduce VIR_STORAGE_NET_HOST_TRANS_FD Not the one you mention (twice).
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, May 19, 2025 at 18:37:03 +0200, Ján Tomko wrote:
On a Monday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
While I've actually implemented support for FD passing the NBD server socket in eb768a556db I managed to misplace the hunk allowing the 'FD' transport in the validation code, rendering the whole feature useless.
Fix the validation logic to make the feature useful.
s/useful/usable/?
Fixes: eb768a556db75040f7b518d198a18bd0f5d6faad
The commit that misplaced the case is: commit ee49106dbf6f3c716f0e31c9ecc9bf528eb72bb2 conf: Introduce VIR_STORAGE_NET_HOST_TRANS_FD
Not the one you mention (twice).
That commit placed it correctly (at that point in time) because the implementation wasn't there. The commit I'm mentioning was supposed to allow it (I mentioned "misplacing hunk allowing the 'FD' transport').
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
From: Peter Krempa <pkrempa@redhat.com> In case selinux is used on the host the socket passed to qemu needs to be properly labelled. Add a hint to the example code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst index 155a45a22f..df6392e3bd 100644 --- a/docs/formatbackup.rst +++ b/docs/formatbackup.rst @@ -53,6 +53,10 @@ were supplied). The following child elements and attributes are supported: import socket import libvirt + import selinux + + # Optionally setup selinux context for the socket if the distro uses it + # selinux.setsockcreatecon_raw("system_u:object_r:svirt_t:s0") s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) s.bind("/path/to/socket") -- 2.49.0
On a Monday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In case selinux is used on the host the socket passed to qemu needs to be properly labelled. Add a hint to the example code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.rst | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst index 155a45a22f..df6392e3bd 100644 --- a/docs/formatbackup.rst +++ b/docs/formatbackup.rst @@ -53,6 +53,10 @@ were supplied). The following child elements and attributes are supported:
import socket import libvirt + import selinux + + # Optionally setup selinux context for the socket if the distro uses it + # selinux.setsockcreatecon_raw("system_u:object_r:svirt_t:s0")
Should this hint that the category should also match the domain, to be "properly" labelled? Either way: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) s.bind("/path/to/socket") -- 2.49.0
On Mon, May 19, 2025 at 18:41:45 +0200, Ján Tomko wrote:
On a Monday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
In case selinux is used on the host the socket passed to qemu needs to be properly labelled. Add a hint to the example code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.rst | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst index 155a45a22f..df6392e3bd 100644 --- a/docs/formatbackup.rst +++ b/docs/formatbackup.rst @@ -53,6 +53,10 @@ were supplied). The following child elements and attributes are supported:
import socket import libvirt + import selinux + + # Optionally setup selinux context for the socket if the distro uses it + # selinux.setsockcreatecon_raw("system_u:object_r:svirt_t:s0")
Should this hint that the category should also match the domain, to be "properly" labelled?
Well, possibly; but this is actually enough to make it work. I didn't care digging deeper.
From: Peter Krempa <pkrempa@redhat.com> Log information (type, label, etc) about FDs passed to qemu via APIs from this module. This does "spill" the selinux library code into this module, but acessing it via the security driver would require passing much more context to this module. Since it's for logging only it can be easily removed if necessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c index 333f9b128e..ef0a8d8127 100644 --- a/src/qemu/qemu_fd.c +++ b/src/qemu/qemu_fd.c @@ -25,6 +25,11 @@ #include "virfile.h" #include "virlog.h" +/* Used strictly for logging selinux context of passed FD */ +#ifdef WITH_SECDRIVER_SELINUX +# include <selinux/selinux.h> +#endif + #define VIR_FROM_THIS VIR_FROM_QEMU VIR_LOG_INIT("qemu.qemu_fd"); @@ -44,6 +49,56 @@ struct _qemuFDPass { }; +static void +qemuFDPassLogFDInfo(const char *name, + size_t idx, + int fd) +{ + struct stat st; + const char *type = "error"; + g_autofree char *selinux = NULL; + g_autofree char *tmp = NULL; + + if (fstat(fd, &st) == 0) { + switch (st.st_mode & S_IFMT) { + case S_IFBLK: + type = "block"; + break; + case S_IFCHR: + type = "char"; + break; + case S_IFDIR: + type = "directory"; + break; + case S_IFIFO: + type = "pipe"; + break; + case S_IFLNK: + type = "symlink"; + break; + case S_IFREG: + type = "file"; + break; + case S_IFSOCK: + type = "socket"; + break; + default: + type = tmp = g_strdup_printf("unknown:'0x%x')", st.st_mode & S_IFMT); + break; + } + } + +#ifdef WITH_SECDRIVER_SELINUX + ignore_value(fgetfilecon_raw(fd, &selinux)); +#else + selinux = g_strdup("N/A"); +#endif + + VIR_DEBUG("passing fd:'%i', name:'%s'(%zu) type:'%s' selinux:'%s'", + fd, name, idx, type, selinux); +} + + void qemuFDPassFree(qemuFDPass *fdpass) { @@ -234,6 +289,8 @@ qemuFDPassTransferCommand(qemuFDPass *fdpass, fdpass->fds[i].fd, fdpass->fds[i].opaque); + qemuFDPassLogFDInfo(fdpass->fds[i].opaque, i, fdpass->fds[i].fd); + virCommandPassFD(cmd, fdpass->fds[i].fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); fdpass->fds[i].fd = -1; virCommandAddArgList(cmd, "-add-fd", arg, NULL); @@ -274,6 +331,8 @@ qemuFDPassTransferMonitor(qemuFDPass *fdpass, } for (i = 0; i < fdpass->nfds; i++) { + qemuFDPassLogFDInfo(fdpass->fds[i].opaque, i, fdpass->fds[i].fd); + if (qemuMonitorAddFileHandleToSet(mon, fdpass->fds[i].fd, fdpass->fdSetID, @@ -381,6 +440,7 @@ qemuFDPassDirectTransferCommand(qemuFDPassDirect *fdpass, if (!fdpass) return; + qemuFDPassLogFDInfo(fdpass->name, 0, fdpass->fd); virCommandPassFD(cmd, fdpass->fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); g_free(fdpass->name); fdpass->name = g_strdup_printf("%d", fdpass->fd); @@ -403,6 +463,8 @@ qemuFDPassDirectTransferMonitor(qemuFDPassDirect *fdpass, if (!fdpass) return 0; + qemuFDPassLogFDInfo(fdpass->name, 0, fdpass->fd); + if (qemuMonitorSendFileHandle(mon, fdpass->name, fdpass->fd) < 0) return -1; -- 2.49.0
On a Monday in 2025, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Log information (type, label, etc) about FDs passed to qemu via APIs from this module.
This does "spill" the selinux library code into this module, but acessing it via the security driver would require passing much more context to this module. Since it's for logging only it can be easily removed if necessary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_fd.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa