On Sat, Jun 27, 2020 at 10:09:34AM -0700, elena.ufimts...@oracle.com wrote: > From: Jagannathan Raman <jag.ra...@oracle.com> > > Send a message to the remote process to connect PCI device with the > corresponding Proxy object in QEMU
I thought the protocol was simplified to a 1:1 device:socket model, but
this patch seems to implement an N:1 model?
In a 1:1 model the CONNECT_DEV message is not necessary because each
socket is already associated with a specific remote device (e.g. qemu -M
remote -object mplink,dev=lsi-scsi-1,sockpath=/tmp/lsi-scsi-1.sock).
Connecting to the socket already indicates which device we are talking
to.
The N:1 model will work but it's more complex. There is a main socket
that is used for CONNECT_DEV (anything else?) and we need to worry about
the lifecycle of the per-device sockets that are passed over the main
socket.
> @@ -50,3 +58,34 @@ gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition
> cond,
>
> return TRUE;
> }
> +
> +static void process_connect_dev_msg(MPQemuMsg *msg, QIOChannel *com,
> + Error **errp)
> +{
> + char *devid = (char *)msg->data2;
> + QIOChannel *dioc = NULL;
> + DeviceState *dev = NULL;
> + MPQemuMsg ret = { 0 };
> + int rc = 0;
> +
> + g_assert(devid && (devid[msg->size - 1] == '\0'));
Asserts are not suitable for external input validation since a failure
aborts the program and lets the client cause a denial-of-service. When
there are multiple clients, one misbehaved client shouldn't be able to
kill the server. Please validate devid using an if statement and set
errp on failure.
Can msg->size be 0? If yes, this code accesses before the beginning of
the buffer.
> +
> + dev = qdev_find_recursive(sysbus_get_default(), devid);
> + if (!dev || !object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + rc = 0xff;
> + goto exit;
> + }
> +
> + dioc = qio_channel_new_fd(msg->fds[0], errp);
Missing error handling if qio_channel_new_fd() fails. We need to
close(msg->fds[0]) ourselves in this case.
> +
> + qio_channel_add_watch(dioc, G_IO_IN | G_IO_HUP, mpqemu_process_msg,
> + (void *)dev, NULL);
> +
> +exit:
> + ret.cmd = RET_MSG;
> + ret.bytestream = 0;
> + ret.data1.u64 = rc;
> + ret.size = sizeof(ret.data1);
> +
> + mpqemu_msg_send(&ret, com);
> +}
> diff --git a/hw/pci/proxy.c b/hw/pci/proxy.c
> index 6d62399c52..16649ed0ec 100644
> --- a/hw/pci/proxy.c
> +++ b/hw/pci/proxy.c
> @@ -15,10 +15,38 @@
> #include "io/channel-util.h"
> #include "hw/qdev-properties.h"
> #include "monitor/monitor.h"
> +#include "io/mpqemu-link.h"
>
> static void proxy_set_socket(PCIProxyDev *pdev, int fd, Error **errp)
> {
> + DeviceState *dev = DEVICE(pdev);
> + MPQemuMsg msg = { 0 };
> + int fds[2];
> + Error *local_err = NULL;
> +
> pdev->com = qio_channel_new_fd(fd, errp);
> +
> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds)) {
> + error_setg(errp, "Failed to create proxy channel with fd %d", fd);
> + return;
pdev->com needs to be cleaned up.
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 5887c8c6c0..54df3b254e 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -234,6 +234,14 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
> return false;
> }
> break;
> + case CONNECT_DEV:
> + if ((msg->num_fds != 1) ||
> + (msg->fds[0] == -1) ||
> + (msg->fds[0] == -1) ||
This line is duplicated.
signature.asc
Description: PGP signature
