[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 22/38] ivshmem: Plug leaks on unplug, fix peer d
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 22/38] ivshmem: Plug leaks on unplug, fix peer disconnect |
Date: |
Wed, 2 Mar 2016 18:47:37 +0100 |
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
- Re: [Qemu-devel] [PATCH 22/38] ivshmem: Plug leaks on unplug, fix peer disconnect,
Marc-André Lureau <=