Re: [PATCH v7 12/21] multi-process: Connect Proxy Object with device in the remote process

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]

On Sat, Jun 27, 2020 at 10:09:34AM -0700, elena.ufimtseva@oracle.com wrote:
> From: Jagannathan Raman <jag.raman@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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]