m4/virt-driver-qemu.m4 | 5 ++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 ++ src/qemu/qemu_capabilities.c | 6 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 38 +++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 7 ++- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 11 ++++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_hotplug.c | 5 +- src/qemu/qemu_interface.c | 80 ++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 6 +++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 15 files changed, 161 insertions(+), 10 deletions(-)
From: Marc-André Lureau <marcandre.lureau@redhat.com>
I am throwing this away for discussions, and early feedback.
With the upcoming release of libslirp[1], we have an opportunity to
run SLIRP networking in a separate process. This will allow for
stricter security policies for both qemu & slirp, as slirp is
notoriously not very safe (discussed on ML, various CVEs, and even the
code says so explicitly in the comments), yet people rely on it regularly.
For network type "user", libvirt could spawn an helper process (an
experimental version is [2]). It would setup a socket pair between
qemu and the helper and use -net socket. qemu could then be built
without libslirp! (imho, qemu should deprecate built-in -net user
support, and doesn't need to grow its own sub-process handling)
This libvirt patch is a bit rough, I am not sure where things should
go. In particular, how to manage the subprocesses properly (security
aspects, and lifecycle etc), and how to deal with migration (prevent
migrating from non-helper to helper version etc).
It seems guestfwd has been non-functional for a while. This is also
something that the current experimental helper doesn't support
atm. Doing all the various "channel" types that libvirt/qemu support
would be tedious, and probably unnecessary. But the underlying
libslirp library support redirections the same way qemu does today, so
it could be added if necessary.
At this point, the slirp-helper doesn't handle migration. But is it
really worth it? From a guest POV, it would look like packet lost or
disconnection. Adding migration handling is possible, to avoid a
disconnection event. How should it be done? via a libvirt provided
socket for storage? via its own file transferred by libvirt? via qemu
somehow (qemu wouldn't really know about the external process but
would copy around the migration bits?).
Does the helper need to have a "standard" & capabilities, similar to
what is proposed for vhost-user [3]? This would allow for easier
competing implementation to emerge (I have plans in mind, not using
libslirp).
Thanks for the feedback!
[1] https://gitlab.freedesktop.org/slirp/libslirp
[2] https://github.com/elmarco/libslirp-rs
[3] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/vhost-user.json
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
m4/virt-driver-qemu.m4 | 5 ++
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 3 ++
src/qemu/qemu_capabilities.c | 6 +++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 38 +++++++++++---
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_conf.c | 7 ++-
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_domain.c | 11 ++++
src/qemu/qemu_domain.h | 3 ++
src/qemu/qemu_hotplug.c | 5 +-
src/qemu/qemu_interface.c | 80 ++++++++++++++++++++++++++++++
src/qemu/qemu_interface.h | 6 +++
src/qemu/test_libvirtd_qemu.aug.in | 1 +
15 files changed, 161 insertions(+), 10 deletions(-)
diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
index a1d05bbd7f..705b1f592b 100644
--- a/m4/virt-driver-qemu.m4
+++ b/m4/virt-driver-qemu.m4
@@ -105,6 +105,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
[/usr/bin:/usr/libexec])
AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"],
[QEMU PR helper])
+ AC_PATH_PROG([SLIRP_HELPER], [slirp-helper],
+ [/usr/bin/slirp-helper],
+ [/usr/bin:/usr/libexec])
+ AC_DEFINE_UNQUOTED([SLIRP_HELPER], ["$SLIRP_HELPER"],
+ [slirp helper])
])
AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index b311f02da6..15206454e0 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -88,6 +88,7 @@ module Libvirtd_qemu =
| bool_entry "clear_emulator_capabilities"
| str_entry "bridge_helper"
| str_entry "pr_helper"
+ | str_entry "slirp_helper"
| bool_entry "set_process_name"
| int_entry "max_processes"
| int_entry "max_files"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 334b4cd4ee..725ae791c5 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -810,6 +810,9 @@
# used whenever <reservations/> are enabled for SCSI LUN devices.
#pr_helper = "/usr/bin/qemu-pr-helper"
+# Path to the SLIRP networking helper.
+#slirp_helper = "/usr/bin/slirp-helper"
+
# User for the swtpm TPM Emulator
#
# Default is 'tss'; this is the same user that tcsd (TrouSerS) installs
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f8ea66b577..86cef7b9ab 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -524,6 +524,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"scsi-disk.device_id",
"virtio-pci-non-transitional",
"overcommit",
+ "net-socket-dgram",
);
@@ -4212,6 +4213,11 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps)
ARCH_IS_PPC64(qemuCaps->arch)) {
virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT);
}
+
+ /* -net socket,fd= with dgram socket (for ex, with slirp helper) */
+ if (qemuCaps->version >= 3001092) {
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM);
+ }
}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 23ecef8c63..9db4c1bf2d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -506,6 +506,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
QEMU_CAPS_SCSI_DISK_DEVICE_ID, /* 'device_id' property of scsi disk */
QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL, /* virtio *-pci-{non-}transitional devices */
QEMU_CAPS_OVERCOMMIT, /* -overcommit */
+ QEMU_CAPS_NET_SOCKET_DGRAM,
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9df7b7e8ea..6a047fa8f9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4066,7 +4066,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
char **tapfd,
size_t tapfdSize,
char **vhostfd,
- size_t vhostfdSize)
+ size_t vhostfdSize,
+ char *slirpfd)
{
bool is_tap = false;
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -4137,6 +4138,12 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
break;
case VIR_DOMAIN_NET_TYPE_USER:
+ if (slirpfd) {
+ virBufferAsprintf(&buf, "socket,fd=%s,",
+ slirpfd);
+ break;
+ }
+
virBufferAddLit(&buf, "user,");
for (i = 0; i < net->guestIP.nips; i++) {
const virNetDevIPAddr *ip = net->guestIP.ips[i];
@@ -8721,10 +8728,10 @@ qemuInterfaceVhostuserConnect(virQEMUDriverPtr driver,
static int
qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
virLogManagerPtr logManager,
virSecurityManagerPtr secManager,
virCommandPtr cmd,
- virDomainDefPtr def,
virDomainNetDefPtr net,
virQEMUCapsPtr qemuCaps,
unsigned int bootindex,
@@ -8733,6 +8740,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
size_t *nnicindexes,
int **nicindexes)
{
+ virDomainDefPtr def = vm->def;
int ret = -1;
char *nic = NULL;
char *host = NULL;
@@ -8743,12 +8751,13 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
size_t vhostfdSize = 0;
char **tapfdName = NULL;
char **vhostfdName = NULL;
+ int slirpfd = -1;
+ char *slirpfdName = NULL;
virDomainNetType actualType = virDomainNetGetActualType(net);
virNetDevBandwidthPtr actualBandwidth;
bool requireNicdev = false;
size_t i;
-
if (!bootindex)
bootindex = net->info.bootIndex;
@@ -8971,6 +8980,18 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
goto cleanup;
}
+ if (actualType == VIR_DOMAIN_NET_TYPE_USER &&
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM) &&
+ !standalone) {
+ if (qemuInterfaceOpenSlirp(driver, vm, net, &slirpfd) < 0) {
+ goto cleanup;
+ }
+ virCommandPassFD(cmd, slirpfd,
+ VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+ if (virAsprintf(&slirpfdName, "%d", slirpfd) < 0)
+ goto cleanup;
+ }
+
for (i = 0; i < tapfdSize; i++) {
if (qemuSecuritySetTapFDLabel(driver->securityManager,
def, tapfd[i]) < 0)
@@ -8993,7 +9014,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
if (!(host = qemuBuildHostNetStr(net, driver,
tapfdName, tapfdSize,
- vhostfdName, vhostfdSize)))
+ vhostfdName, vhostfdSize,
+ slirpfdName)))
goto cleanup;
virCommandAddArgList(cmd, "-netdev", host, NULL);
@@ -9032,6 +9054,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
virSetError(saved_err);
virFreeError(saved_err);
}
+ VIR_FREE(slirpfdName);
for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
if (ret < 0)
VIR_FORCE_CLOSE(vhostfd[i]);
@@ -9061,10 +9084,10 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
*/
static int
qemuBuildNetCommandLine(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
virLogManagerPtr logManager,
virSecurityManagerPtr secManager,
virCommandPtr cmd,
- virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virNetDevVPortProfileOp vmop,
bool standalone,
@@ -9075,6 +9098,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
size_t i;
int last_good_net = -1;
virErrorPtr originalError = NULL;
+ virDomainDefPtr def = vm->def;
if (def->nnets) {
unsigned int bootNet = 0;
@@ -9090,7 +9114,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (qemuBuildInterfaceCommandLine(driver, logManager, secManager, cmd, def, net,
+ if (qemuBuildInterfaceCommandLine(driver, vm, logManager, secManager, cmd, net,
qemuCaps, bootNet, vmop,
standalone, nnicindexes,
nicindexes) < 0)
@@ -10818,7 +10842,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
goto error;
- if (qemuBuildNetCommandLine(driver, logManager, secManager, cmd, def,
+ if (qemuBuildNetCommandLine(driver, vm, logManager, secManager, cmd,
qemuCaps, vmop, standalone,
nnicindexes, nicindexes, &bootHostdevNet) < 0)
goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 9565a7a377..93696cb8d0 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -88,7 +88,8 @@ char *qemuBuildHostNetStr(virDomainNetDefPtr net,
char **tapfd,
size_t tapfdSize,
char **vhostfd,
- size_t vhostfdSize);
+ size_t vhostfdSize,
+ char *slirpfd);
/* Current, best practice */
char *qemuBuildNicDevStr(virDomainDefPtr def,
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index daea11dacb..e3cc0921a6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -297,7 +297,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
}
if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 ||
- VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
+ VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0 ||
+ VIR_STRDUP(cfg->slirpHelperName, SLIRP_HELPER) < 0)
goto error;
cfg->clearEmulatorCapabilities = true;
@@ -387,6 +388,7 @@ static void virQEMUDriverConfigDispose(void *obj)
VIR_FREE(cfg->hugetlbfs);
VIR_FREE(cfg->bridgeHelperName);
VIR_FREE(cfg->prHelperName);
+ VIR_FREE(cfg->slirpHelperName);
VIR_FREE(cfg->saveImageFormat);
VIR_FREE(cfg->dumpImageFormat);
@@ -681,6 +683,9 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg,
if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0)
return -1;
+ if (virConfGetValueString(conf, "slirp_helper", &cfg->slirpHelperName) < 0)
+ return -1;
+
if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0)
return -1;
if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 983e74a3cf..7ae09f8f8c 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -160,6 +160,7 @@ struct _virQEMUDriverConfig {
char *bridgeHelperName;
char *prHelperName;
+ char *slirpHelperName;
bool macFilter;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f9f5ffc22b..626702c3ed 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2345,6 +2345,15 @@ qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf,
}
+static void
+qemuDomainObjPrivateXMLFormatSlirp(virBufferPtr buf,
+ qemuDomainObjPrivatePtr priv)
+{
+ if (priv->slirpPid)
+ virBufferAddLit(buf, "<Slirp/>\n");
+}
+
+
static int
qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf,
virStorageSourcePtr src,
@@ -2555,6 +2564,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
qemuDomainObjPrivateXMLFormatPR(buf, priv);
+ qemuDomainObjPrivateXMLFormatSlirp(buf, priv);
+
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
virBufferAsprintf(buf, "<nodename index='%llu'/>\n", priv->nodenameindex);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 06640a9510..f7937bf436 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -365,6 +365,9 @@ struct _qemuDomainObjPrivate {
/* true if qemu-pr-helper process is running for the domain */
bool prDaemonRunning;
+ /* todo: list of running slirp processes */
+ pid_t slirpPid;
+
/* counter for generating node names for qemu disks */
unsigned long long nodenameindex;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a4f7d111b1..f6ee9815ab 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1355,6 +1355,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_NET, { .net = net } };
virErrorPtr originalError = NULL;
+ char *slirpfdName = NULL;
char **tapfdName = NULL;
int *tapfd = NULL;
size_t tapfdSize = 0;
@@ -1592,7 +1593,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
if (!(netstr = qemuBuildHostNetStr(net, driver,
tapfdName, tapfdSize,
- vhostfdName, vhostfdSize)))
+ vhostfdName, vhostfdSize,
+ slirpfdName)))
goto cleanup;
qemuDomainObjEnterMonitor(driver, vm);
@@ -1720,6 +1722,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
VIR_FREE(vhostfdName);
VIR_FREE(charDevAlias);
virObjectUnref(conn);
+ VIR_FREE(slirpfdName);
virDomainCCWAddressSetFree(ccwaddrs);
return ret;
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index c8effa68f4..55423d6895 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -603,6 +603,86 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
}
+/**
+ * qemuInterfaceOpenSlirp:
+ * @net: network definition
+ * @slirpfd: slirp connection
+ *
+ * Returns: 0 on success
+ * -1 on failure
+ */
+int
+qemuInterfaceOpenSlirp(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainNetDefPtr net,
+ int *slirpfd)
+{
+ int i, pair[2] = { -1, -1 };
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ VIR_AUTOFREE(char *) cmdstr = NULL;
+ VIR_AUTOFREE(char *) addr = NULL;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
+ if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair) < 0) {
+ virReportSystemError(errno, "%s", _("failed to create socket"));
+ goto error;
+ }
+
+ cmd = virCommandNew(cfg->slirpHelperName);
+ virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
+ virCommandPassFD(cmd, pair[1],
+ VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+
+ for (i = 0; i < net->guestIP.nips; i++) {
+ const virNetDevIPAddr *ip = net->guestIP.ips[i];
+ const char *opt = "";
+
+ if (!(addr = virSocketAddrFormat(&ip->address)))
+ goto error;
+
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET))
+ opt = "--net";
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6))
+ opt = "--prefix-ipv6";
+
+ virCommandAddArgFormat(cmd, "%s=%s", opt, addr);
+ VIR_FREE(addr);
+
+ if (ip->prefix) {
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) {
+ virSocketAddr netmask;
+ VIR_AUTOFREE(char *) netmaskStr = NULL;
+
+ if (virSocketAddrPrefixToNetmask(ip->prefix, &netmask, AF_INET) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to translate prefix %d to netmask"),
+ ip->prefix);
+ goto error;
+ }
+ if (!(netmaskStr = virSocketAddrFormat(&netmask)))
+ goto error;
+ virCommandAddArgFormat(cmd, "--mask=%s", netmaskStr);
+ }
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6))
+ virCommandAddArgFormat(cmd, "--prefix-length-ipv6=%u", ip->prefix);
+ }
+ }
+
+ virCommandClearCaps(cmd);
+ if (virCommandRunAsync(cmd, &priv->slirpPid) < 0) {
+ goto error;
+ }
+
+ *slirpfd = pair[0];
+ return 0;
+
+error:
+ VIR_FORCE_CLOSE(pair[0]);
+ return -1;
+}
+
+
/**
* qemuInterfaceOpenVhostNet:
* @def: domain definition
diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
index f3ec540eda..de6195462b 100644
--- a/src/qemu/qemu_interface.h
+++ b/src/qemu/qemu_interface.h
@@ -55,4 +55,10 @@ int qemuInterfaceOpenVhostNet(virDomainDefPtr def,
virDomainNetDefPtr net,
int *vhostfd,
size_t *vhostfdSize);
+
+int qemuInterfaceOpenSlirp(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainNetDefPtr net,
+ int *slirpfd);
+
#endif /* LIBVIRT_QEMU_INTERFACE_H */
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index fea1d308b7..5796e5d1eb 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -102,5 +102,6 @@ module Test_libvirtd_qemu =
}
{ "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
{ "pr_helper" = "/usr/bin/qemu-pr-helper" }
+{ "slirp_helper" = "/usr/bin/slirp-helper" }
{ "swtpm_user" = "tss" }
{ "swtpm_group" = "tss" }
--
2.21.0.313.ge35b8cb8e2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 4/18/19 3:24 PM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > I am throwing this away for discussions, and early feedback. > > With the upcoming release of libslirp[1], we have an opportunity to > run SLIRP networking in a separate process. This will allow for > stricter security policies for both qemu & slirp, as slirp is > notoriously not very safe (discussed on ML, various CVEs, and even the > code says so explicitly in the comments), yet people rely on it regularly. Do they? I mean, SLIRP is broken in libvirt for quite some time and we haven't noticed nor seen a bug about it. What is the typical use anyway? I can see some potential in combining -netdev user + -chardev where one could see/inject packets into a guest (if I got that correctly). But that can't work currently, because libvirt doesn't allow setting all the interesting bits (like subnet mask). > > For network type "user", libvirt could spawn an helper process (an > experimental version is [2]). It would setup a socket pair between > qemu and the helper and use -net socket. qemu could then be built > without libslirp! (imho, qemu should deprecate built-in -net user > support, and doesn't need to grow its own sub-process handling) > > This libvirt patch is a bit rough, I am not sure where things should > go. In particular, how to manage the subprocesses properly (security > aspects, and lifecycle etc), and how to deal with migration (prevent > migrating from non-helper to helper version etc). > > It seems guestfwd has been non-functional for a while. This is also > something that the current experimental helper doesn't support > atm. Doing all the various "channel" types that libvirt/qemu support > would be tedious, and probably unnecessary. But the underlying > libslirp library support redirections the same way qemu does today, so > it could be added if necessary. > > At this point, the slirp-helper doesn't handle migration. But is it > really worth it? From a guest POV, it would look like packet lost or > disconnection. Adding migration handling is possible, to avoid a > disconnection event. How should it be done? via a libvirt provided > socket for storage? via its own file transferred by libvirt? via qemu > somehow (qemu wouldn't really know about the external process but > would copy around the migration bits?). > > Does the helper need to have a "standard" & capabilities, similar to > what is proposed for vhost-user [3]? This would allow for easier > competing implementation to emerge (I have plans in mind, not using > libslirp). > > Thanks for the feedback! > > [1] https://gitlab.freedesktop.org/slirp/libslirp > [2] https://github.com/elmarco/libslirp-rs > [3] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/vhost-user.json > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > m4/virt-driver-qemu.m4 | 5 ++ > src/qemu/libvirtd_qemu.aug | 1 + > src/qemu/qemu.conf | 3 ++ > src/qemu/qemu_capabilities.c | 6 +++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 38 +++++++++++--- > src/qemu/qemu_command.h | 3 +- > src/qemu/qemu_conf.c | 7 ++- > src/qemu/qemu_conf.h | 1 + > src/qemu/qemu_domain.c | 11 ++++ > src/qemu/qemu_domain.h | 3 ++ > src/qemu/qemu_hotplug.c | 5 +- > src/qemu/qemu_interface.c | 80 ++++++++++++++++++++++++++++++ > src/qemu/qemu_interface.h | 6 +++ > src/qemu/test_libvirtd_qemu.aug.in | 1 + > 15 files changed, 161 insertions(+), 10 deletions(-) The code looks more or less okaysh, but I'd rather discuss the future of slirp for now. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi On Thu, Apr 25, 2019 at 3:22 PM Michal Privoznik <mprivozn@redhat.com> wrote: > > On 4/18/19 3:24 PM, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > I am throwing this away for discussions, and early feedback. > > > > With the upcoming release of libslirp[1], we have an opportunity to > > run SLIRP networking in a separate process. This will allow for > > stricter security policies for both qemu & slirp, as slirp is > > notoriously not very safe (discussed on ML, various CVEs, and even the > > code says so explicitly in the comments), yet people rely on it regularly. > > Do they? I mean, SLIRP is broken in libvirt for quite some time and we > haven't noticed nor seen a bug about it. What is the typical use anyway? "user" networking (<interface type='user'>) is not broken, it's guestfwd that is not working for a while apparently. Various projects use slirp forks, that's why we decided to make it again a standalone project / library that can be shared. slirp4netns (used by podman afaik), is perhaps the most worrisome to me. > I can see some potential in combining -netdev user + -chardev where one > could see/inject packets into a guest (if I got that correctly). But > that can't work currently, because libvirt doesn't allow setting all the > interesting bits (like subnet mask). > > > > > For network type "user", libvirt could spawn an helper process (an > > experimental version is [2]). It would setup a socket pair between > > qemu and the helper and use -net socket. qemu could then be built > > without libslirp! (imho, qemu should deprecate built-in -net user > > support, and doesn't need to grow its own sub-process handling) > > > > This libvirt patch is a bit rough, I am not sure where things should > > go. In particular, how to manage the subprocesses properly (security > > aspects, and lifecycle etc), and how to deal with migration (prevent > > migrating from non-helper to helper version etc). > > > > It seems guestfwd has been non-functional for a while. This is also > > something that the current experimental helper doesn't support > > atm. Doing all the various "channel" types that libvirt/qemu support > > would be tedious, and probably unnecessary. But the underlying > > libslirp library support redirections the same way qemu does today, so > > it could be added if necessary. > > > > At this point, the slirp-helper doesn't handle migration. But is it > > really worth it? From a guest POV, it would look like packet lost or > > disconnection. Adding migration handling is possible, to avoid a > > disconnection event. How should it be done? via a libvirt provided > > socket for storage? via its own file transferred by libvirt? via qemu > > somehow (qemu wouldn't really know about the external process but > > would copy around the migration bits?). > > > > Does the helper need to have a "standard" & capabilities, similar to > > what is proposed for vhost-user [3]? This would allow for easier > > competing implementation to emerge (I have plans in mind, not using > > libslirp). > > > > Thanks for the feedback! > > > > [1] https://gitlab.freedesktop.org/slirp/libslirp > > [2] https://github.com/elmarco/libslirp-rs > > [3] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/vhost-user.json > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > m4/virt-driver-qemu.m4 | 5 ++ > > src/qemu/libvirtd_qemu.aug | 1 + > > src/qemu/qemu.conf | 3 ++ > > src/qemu/qemu_capabilities.c | 6 +++ > > src/qemu/qemu_capabilities.h | 1 + > > src/qemu/qemu_command.c | 38 +++++++++++--- > > src/qemu/qemu_command.h | 3 +- > > src/qemu/qemu_conf.c | 7 ++- > > src/qemu/qemu_conf.h | 1 + > > src/qemu/qemu_domain.c | 11 ++++ > > src/qemu/qemu_domain.h | 3 ++ > > src/qemu/qemu_hotplug.c | 5 +- > > src/qemu/qemu_interface.c | 80 ++++++++++++++++++++++++++++++ > > src/qemu/qemu_interface.h | 6 +++ > > src/qemu/test_libvirtd_qemu.aug.in | 1 + > > 15 files changed, 161 insertions(+), 10 deletions(-) > > The code looks more or less okaysh, but I'd rather discuss the future of > slirp for now. > Well, that's what I propose to discuss with this proposal. Make current libslirp usage safer by using it as a subprocess, continue to improve/fix it, and allow competing implementation to emerge. thanks for the feedback -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 18, 2019 at 03:24:13PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I am throwing this away for discussions, and early feedback.
>
> With the upcoming release of libslirp[1], we have an opportunity to
> run SLIRP networking in a separate process. This will allow for
> stricter security policies for both qemu & slirp, as slirp is
> notoriously not very safe (discussed on ML, various CVEs, and even the
> code says so explicitly in the comments), yet people rely on it regularly.
>
> For network type "user", libvirt could spawn an helper process (an
> experimental version is [2]). It would setup a socket pair between
> qemu and the helper and use -net socket. qemu could then be built
> without libslirp! (imho, qemu should deprecate built-in -net user
> support, and doesn't need to grow its own sub-process handling)
>
> This libvirt patch is a bit rough, I am not sure where things should
> go. In particular, how to manage the subprocesses properly (security
> aspects, and lifecycle etc), and how to deal with migration (prevent
> migrating from non-helper to helper version etc).
Security is an interesting question
- DAC permissions
- with session libvirt we have no choice - it will be the
same UID as QEMU and libvirtd
- with system libvirt we can run the helper under the same
UID/GID as QEMU (this might be the shared qemu:qemu pair,
or it might be a custom uniq-per-VM UID/GID pair). With
the shared UID/GID qemu:qemu, it would benefit from a
separate slirp UID/GID. If the VM is using a uniq-per-VM
UID/GID though, using a single "slirp" user would be a
security regression.
I'm inclined to ignore DAC UID/GID as a means of isolating
slirp from QEMU
- MAC permissions
Clearly the slirp helper must have a dedicated SELinux
domain "slirp_t". We should then append the uniq-per-VM
category to this. This gives strong separation between QEMU
and SLIRP, as well as between SLIRPS for each QEMU
- Capabilities
Ideally slirp will have zero capabilities
> At this point, the slirp-helper doesn't handle migration. But is it
> really worth it? From a guest POV, it would look like packet lost or
> disconnection. Adding migration handling is possible, to avoid a
> disconnection event. How should it be done? via a libvirt provided
> socket for storage? via its own file transferred by libvirt? via qemu
> somehow (qemu wouldn't really know about the external process but
> would copy around the migration bits?).
How does migration actually work today with built-in slirp ? I
Assume that all open network connections are lost right now ?
What actual state does QEMU transfer for built-in slirp what
does that achieve in practice ?
Ideally we would "live migrate" all existing QEMU processes using
built-in slirp to use the slirp helper. Even if this doesn't
actually do much useful stuff due to all connections being lost,
it allows us to automatically enable external slirp for everything
out of the box. Requiring an opt-in config for external slirp makes
it less compelling & will massively reduce uptake.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi On Tue, Apr 30, 2019 at 7:31 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > At this point, the slirp-helper doesn't handle migration. But is it > > really worth it? From a guest POV, it would look like packet lost or > > disconnection. Adding migration handling is possible, to avoid a > > disconnection event. How should it be done? via a libvirt provided > > socket for storage? via its own file transferred by libvirt? via qemu > > somehow (qemu wouldn't really know about the external process but > > would copy around the migration bits?). > > How does migration actually work today with built-in slirp ? I It is registered with save_state/load_state callbacks (old-style vmstate), and save/load from the given stream. It uses the "vmstate format" so far, although it could be a blob for what qemu is concerned (there is no json-desc, although there could be, but that would make libslirp even more relying on vmstate format). But the interesting thing is that very few state is saved. Beside some DHCP (bootp) state, only guestfwd sockets are saved. I suppose the reason is that it's not "possible" to migrate a system socket. On destination, UDP sockets will be handled by guest OS fine, new slirp UDP socket will be created, and guest will mostly see some packet lost. For TCP, in general, there isn't anything slirp can really do. Even if slirp did attempt to save/restore TCP sockets, this will result in stream corruption (or just disconnection) in the guest. That's why it probably doesn't make much sense to save much of slirp socket state beside guestfwd, and let the guest handle socket disconnect/reconnect. > Assume that all open network connections are lost right now ? So guestfwd (& udp) sockets shouldn't be "lost" from guest POV. But TCP sockets will be. > > What actual state does QEMU transfer for built-in slirp what > does that achieve in practice ? > > Ideally we would "live migrate" all existing QEMU processes using > built-in slirp to use the slirp helper. Even if this doesn't > actually do much useful stuff due to all connections being lost, > it allows us to automatically enable external slirp for everything > out of the box. Requiring an opt-in config for external slirp makes > it less compelling & will massively reduce uptake. It's not reasonable to expect built-in slirp VM to migrate to external slirp imho. Qemu doesn't know much about the external helper, it talks ethernet frame with it only. When external slirp process learns migration support, either libvirt manages the "state" (read it, transfer it etc), or we could somehow teach qemu to do it, ex "-savevm-external chardev", which would talk a simple protocol to read/save the state. Other ideas? thanks -- Marc-André Lureau -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.