Re: [Qemu-devel] [PATCH v5 36/48] ivshmem: add check on protocol version in QEMU

[Claudio Fontana]

On 02.10.2015 21:09, address@hidden wrote:
> From: David Marchand <address@hidden>
> 
> Send a protocol version as the first message from server, clients must
> close communication if they don't support this protocol version.  Older
> QEMUs should be fine with this change in the protocol since they
> overrides their own vm_id on reception of an id associated to no
> eventfd.
> 
> Signed-off-by: David Marchand <address@hidden>
> Signed-off-by: Marc-André Lureau <address@hidden>
> [use fifo_update_and_get()]
> ---
>  contrib/ivshmem-client/ivshmem-client.c | 13 ++++++++++---
>  contrib/ivshmem-client/ivshmem-client.h |  1 +
>  contrib/ivshmem-server/ivshmem-server.c |  9 +++++++++
>  contrib/ivshmem-server/ivshmem-server.h |  1 +
>  docs/specs/ivshmem_device_spec.txt      |  9 ++++++---
>  hw/misc/ivshmem.c                       | 31 +++++++++++++++++++++++++++++--
>  include/hw/misc/ivshmem.h               | 25 +++++++++++++++++++++++++
>  7 files changed, 81 insertions(+), 8 deletions(-)
>  create mode 100644 include/hw/misc/ivshmem.h
> 
> diff --git a/contrib/ivshmem-client/ivshmem-client.c 
> b/contrib/ivshmem-client/ivshmem-client.c
> index 34a65b1..8a7cbfd 100644
> --- a/contrib/ivshmem-client/ivshmem-client.c
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -206,10 +206,17 @@ ivshmem_client_connect(IvshmemClient *client)
>          goto err_close;
>      }
>  
> -    /* first, we expect our index + a fd == -1 */
> +    /* first, we expect a protocol version */
> +    if (ivshmem_client_read_one_msg(client, &tmp, &fd) < 0 ||
> +        (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n");
> +        goto err_close;
> +    }
> +
> +    /* then, we expect our index + a fd == -1 */
>      if (ivshmem_client_read_one_msg(client, &client->local.id, &fd) < 0 ||
>          client->local.id < 0 || fd != -1) {
> -        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n");
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n");
>          goto err_close;
>      }
>      IVSHMEM_CLIENT_DEBUG(client, "our_id=%ld\n", client->local.id);
> @@ -221,7 +228,7 @@ ivshmem_client_connect(IvshmemClient *client)
>          if (fd >= 0) {
>              close(fd);
>          }
> -        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n");
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (3)\n");
>          goto err_close;
>      }
>      client->shm_fd = fd;
> diff --git a/contrib/ivshmem-client/ivshmem-client.h 
> b/contrib/ivshmem-client/ivshmem-client.h
> index 284c4a3..9215f34 100644
> --- a/contrib/ivshmem-client/ivshmem-client.h
> +++ b/contrib/ivshmem-client/ivshmem-client.h
> @@ -23,6 +23,7 @@
>  #include <sys/select.h>
>  
>  #include "qemu/queue.h"
> +#include "hw/misc/ivshmem.h"
>  
>  /**
>   * Maximum number of notification vectors supported by the client
> diff --git a/contrib/ivshmem-server/ivshmem-server.c 
> b/contrib/ivshmem-server/ivshmem-server.c
> index 4a25d28..060f414 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -101,6 +101,15 @@ ivshmem_server_send_initial_info(IvshmemServer *server, 
> IvshmemServerPeer *peer)
>  {
>      int ret;
>  
> +    /* send our protocol version first */
> +    ret = ivshmem_server_send_one_msg(peer->sock_fd, 
> IVSHMEM_PROTOCOL_VERSION,
> +                                      -1);
> +    if (ret < 0) {
> +        IVSHMEM_SERVER_DEBUG(server, "cannot send version: %s\n",
> +                             strerror(errno));
> +        return -1;
> +    }
> +
>      /* send the peer id to the client */
>      ret = ivshmem_server_send_one_msg(peer->sock_fd, peer->id, -1);
>      if (ret < 0) {
> diff --git a/contrib/ivshmem-server/ivshmem-server.h 
> b/contrib/ivshmem-server/ivshmem-server.h
> index e9b0e7a..65b3c2d 100644
> --- a/contrib/ivshmem-server/ivshmem-server.h
> +++ b/contrib/ivshmem-server/ivshmem-server.h
> @@ -32,6 +32,7 @@
>  #include <stdbool.h>
>  
>  #include "qemu/queue.h"
> +#include "hw/misc/ivshmem.h"
>  
>  /**
>   * Maximum number of notification vectors supported by the server
> diff --git a/docs/specs/ivshmem_device_spec.txt 
> b/docs/specs/ivshmem_device_spec.txt
> index 12f338e..3435116 100644
> --- a/docs/specs/ivshmem_device_spec.txt
> +++ b/docs/specs/ivshmem_device_spec.txt
> @@ -64,6 +64,8 @@ It creates a shared memory object then waits for clients to 
> connect on a unix
>  socket.
>  
>  For each client (QEMU process) that connects to the server:
> +- the server sends a protocol version, if client does not support it, the 
> client
> +  closes the communication,
>  - the server assigns an ID for this client and sends this ID to him as the 
> first
>    message,
>  - the server sends a fd to the shared memory object to this client,
> @@ -86,9 +88,10 @@ been provided in qemu.git/contrib/ivshmem-client for debug.
>  
>  *QEMU as an ivshmem client*
>  
> -At initialisation, when creating the ivshmem device, QEMU gets its ID from 
> the
> -server then makes it available through BAR0 IVPosition register for the VM to
> -use (see 'PCI device registers' subsection).
> +At initialisation, when creating the ivshmem device, QEMU first receives a
> +protocol version and closes communication with server if it does not match.
> +Then, QEMU gets its ID from the server then makes it available through BAR0
> +IVPosition register for the VM to use (see 'PCI device registers' 
> subsection).
>  QEMU then uses the fd to the shared memory to map it to BAR2.
>  eventfds for all other clients received from the server are stored to 
> implement
>  BAR0 Doorbell register (see 'PCI device registers' subsection).
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 1b58010..3b6acd6 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -27,6 +27,8 @@
>  #include "qemu/fifo8.h"
>  #include "sysemu/char.h"
>  
> +#include "hw/misc/ivshmem.h"
> +
>  #include <sys/mman.h>
>  #include <sys/types.h>
>  #include <limits.h>
> @@ -597,6 +599,31 @@ static void ivshmem_read(void *opaque, const uint8_t 
> *buf, int size)
>      }
>  }
>  
> +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int 
> size)
> +{
> +    IVShmemState *s = opaque;
> +    int tmp;
> +    long version;
> +
> +    if (!fifo_update_and_get(s, buf, size,
> +                             &version, sizeof(version))) {
> +        return;
> +    }
> +
> +    tmp = qemu_chr_fe_get_msgfd(s->server_chr);
> +    if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) {
> +        fprintf(stderr, "incompatible version, you are connecting to a 
> ivshmem-"
> +                "server using a different protocol please check your 
> setup\n");
> +        qemu_chr_delete(s->server_chr);
> +        s->server_chr = NULL;
> +        return;
> +    }
> +
> +    IVSHMEM_DPRINTF("version check ok, switch to real chardev handler\n");
> +    qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
> +                          ivshmem_event, s);
> +}
> +
>  /* Select the MSI-X vectors used by device.
>   * ivshmem maps events to vectors statically, so
>   * we just enable all vectors on init and after reset. */
> @@ -770,8 +797,8 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>  
>          s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *));
>  
> -        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, 
> ivshmem_read,
> -                     ivshmem_event, s);
> +        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive,
> +                              ivshmem_check_version, ivshmem_event, s);
>      } else {
>          /* just map the file immediately, we're not using a server */
>          int fd;
> diff --git a/include/hw/misc/ivshmem.h b/include/hw/misc/ivshmem.h
> new file mode 100644
> index 0000000..433ef53
> --- /dev/null
> +++ b/include/hw/misc/ivshmem.h
> @@ -0,0 +1,25 @@
> +
> +/*
> + * Inter-VM Shared Memory PCI device.
> + *
> + * Author:
> + *      Cam Macdonell <address@hidden>
> + *
> + * Based On: cirrus_vga.c
> + *          Copyright (c) 2004 Fabrice Bellard
> + *          Copyright (c) 2004 Makoto Suzuki (suzu)
> + *
> + *      and rtl8139.c
> + *          Copyright (c) 2006 Igor Kovalenko
> + *
> + * This code is licensed under the GNU GPL v2.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +#ifndef IVSHMEM_H
> +#define IVSHMEM_H
> +
> +#define IVSHMEM_PROTOCOL_VERSION 0
> +
> +#endif /* IVSHMEM_H */
> 

Reviewed-by: Claudio Fontana <address@hidden>