Re: [Qemu-devel] [PATCH 22/38] ivshmem: Plug leaks on unplug, fix peer disconnect

[Marc-André Lureau]

Hi

On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
> close_peer_eventfds() cleans up three things: ioeventfd triggers if
> they exist, eventfds, and the array to store them.
>
> Commit 98609cd (v1.2.0) fixed it not to clean up ioeventfd triggers
> when they don't exist (property ioeventfd=off, which is the default).
> Unfortunately, the fix also made it skip cleanup of the eventfds and
> the array then.  This is a memory and file descriptor leak on unplug.
>
> Additionally, the reset of nb_eventfds is skipped.  Doesn't matter on
> unplug.  On peer disconnect, however, this permanently wedges the
> interrupt vectors used for that peer's ID.  The eventfds stay behind,
> but aren't connected to a peer anymore.  When the ID gets recycled for
> a new peer, the new peer's eventfds get assigned to vectors after the
> old ones.  Commonly, the device's number of vectors matches the
> server's, so the new ones get dropped with a "Too many eventfd
> received" message.  Interrupts either don't work (common case) or go
> to the wrong vector.
>
> Fix by narrowing the conditional to just the ioeventfd trigger
> cleanup.
>
> While there, move the "invalid" peer check to the only caller where it
> can actually happen.
>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/misc/ivshmem.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index fc46666..c366087 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -428,21 +428,17 @@ static void close_peer_eventfds(IVShmemState *s, int 
> posn)
>  {
>      int i, n;
>
> -    if (!ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        return;
> -    }
> -    if (posn < 0 || posn >= s->nb_peers) {
> -        error_report("invalid peer %d", posn);
> -        return;
> -    }
> -
> +    assert(posn >= 0 && posn < s->nb_peers);
>      n = s->peers[posn].nb_eventfds;
>
> -    memory_region_transaction_begin();
> -    for (i = 0; i < n; i++) {
> -        ivshmem_del_eventfd(s, posn, i);
> +    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> +        memory_region_transaction_begin();
> +        for (i = 0; i < n; i++) {
> +            ivshmem_del_eventfd(s, posn, i);
> +        }
> +        memory_region_transaction_commit();
>      }
> -    memory_region_transaction_commit();
> +
>      for (i = 0; i < n; i++) {
>          event_notifier_cleanup(&s->peers[posn].eventfds[i]);
>      }

Looks good, that makes me wonder, what would happen if posn == vm_id?
I think this should be made an invalid condition or it should revert
setup_interrupt().

> @@ -598,6 +594,10 @@ static void process_msg_shmem(IVShmemState *s, int fd)
>  static void process_msg_disconnect(IVShmemState *s, uint16_t posn)
>  {
>      IVSHMEM_DPRINTF("posn %d has gone away\n", posn);
> +    if (posn >= s->nb_peers) {
> +        error_report("invalid peer %d", posn);
> +        return;
> +    }
>      close_peer_eventfds(s, posn);
>  }
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau