[Spice-devel] [PATCH spice] server: do not fail to read all agent data when called recursively

[Alon Levy]

ACK.

----- "Hans de Goede" <hdegoede at redhat.com> wrote:

> We can loose (not handle) a virtio port write from the guest:
> 
> - server/reds.c: vdagent_char_device_wakeup get called
>   by hw/spice-vmc.c because data has arrived from the guest,
> - hw/spice-vmc.c: vmc_read get calls
> - If the vmc_read call depletes the current buffer it calls
>   virtio_serial_throttle_port(&svc->port, false)
> - This causes vmc_have_data to get called, which if in the
>   mean time another buffer has arrived causes
>   vdagent_char_device_wakeup to gets called again
>   (so recursively)
> - vdagent_char_device_wakeup is protected against recursive
>   calling and ignores the second call (a nasty hack)
> - if no other data arrives, the arrived data will not get read
> 
> Together with another nasty hack in the linux guest virtio_console
> driver, where it waits for a write to be acked by the host before
> continuing with the next one, this can lead to the guest
> getting stuck / hang (until the write is read by the spice-server
> which never happens becaus of the above issues).
> ---
>  server/reds.c |   23 ++++++++++++++---------
>  1 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index a88ca95..f4dfa46 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1231,21 +1231,14 @@ static void dispatch_vdi_port_data(int port,
> VDIReadBuf *buf)
>  
>  static int read_from_vdi_port(void)
>  {
> -    // FIXME: UGLY HACK. Result of spice-vmc vmc_read triggering
> flush of throttled data, and recalling this.
> -    static int inside_call = 0;
>      int quit_loop = 0;
>      VDIPortState *state = &reds->agent_state;
>      SpiceCharDeviceInterface *sif;
>      VDIReadBuf *dispatch_buf;
>      int total = 0;
>      int n;
> -    if (inside_call) {
> -        return 0;
> -    }
> -    inside_call = 1;
>  
>      if (!reds->agent_state.connected || reds->mig_target) {
> -        inside_call = 0;
>          return 0;
>      }
>  
> @@ -1314,13 +1307,25 @@ static int read_from_vdi_port(void)
>              dispatch_vdi_port_data(state->vdi_chunk_header.port,
> dispatch_buf);
>          }
>      }
> -    inside_call = 0;
>      return total;
>  }
>  
>  void vdagent_char_device_wakeup(SpiceCharDeviceInstance *sin)
>  {
> -    while (read_from_vdi_port());
> +    static int vdagent_char_device_wakeups = 0;
> +
> +    vdagent_char_device_wakeups++;
> +
> +    if (vdagent_char_device_wakeups != 1) {
> +        /* When spice-vmc vmc_read triggers flush of throttled data,
> we may get
> +           recalled and we may not call read_from_vdi_port()
> recursively */
> +        return;
> +    }
> +
> +    while (vdagent_char_device_wakeups) {
> +        while (read_from_vdi_port()) {}
> +        vdagent_char_device_wakeups--;
> +    }
>  }
>  
>  static void reds_handle_agent_mouse_event()
> -- 
> 1.7.3.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel