[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 26/42] ivshmem: Plug leaks on unplug, fix pee
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v2 26/42] ivshmem: Plug leaks on unplug, fix peer disconnect |
Date: |
Wed, 9 Mar 2016 13:45:35 +0100 |
On Mon, Mar 7, 2016 at 8:25 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, and tighten it to reject own ID.
>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
Reviewed-by: Marc-André Lureau <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 e568263..2f2f43f 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]);
> }
> @@ -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 || posn == s->vm_id) {
> + error_report("invalid peer %d", posn);
> + return;
> + }
> close_peer_eventfds(s, posn);
> }
>
> --
> 2.4.3
>
>
--
Marc-André Lureau
- [Qemu-devel] [PATCH v2 41/42] ivshmem: Require master to have ID zero, (continued)
- [Qemu-devel] [PATCH v2 41/42] ivshmem: Require master to have ID zero, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 07/42] event_notifier: Make event_notifier_init_fd() #ifdef CONFIG_EVENTFD, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 22/42] ivshmem: Leave INTx alone when using MSI-X, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 15/42] ivshmem: Clean up after commit 9940c32, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 18/42] ivshmem: Fix harmless misuse of Error, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 27/42] ivshmem: Receive shared memory synchronously in realize(), Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 32/42] ivshmem: Tighten check of property "size", Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 26/42] ivshmem: Plug leaks on unplug, fix peer disconnect, Markus Armbruster, 2016/03/07
- Re: [Qemu-devel] [PATCH v2 26/42] ivshmem: Plug leaks on unplug, fix peer disconnect,
Marc-André Lureau <=
- [Qemu-devel] [PATCH v2 37/42] ivshmem: Replace int role_val by OnOffAuto master, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 39/42] ivshmem: Clean up after the previous commit, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 31/42] ivshmem: Simplify how we cope with short reads from server, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 29/42] ivshmem: Rely on server sending the ID right after the version, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 38/42] ivshmem: Split ivshmem-plain, ivshmem-doorbell off ivshmem, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 30/42] ivshmem: Drop the hackish test for UNIX domain chardev, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 17/42] ivshmem: Don't destroy the chardev on version mismatch, Markus Armbruster, 2016/03/07
- Re: [Qemu-devel] [PATCH v2 00/42] ivshmem: Fixes, cleanups, device model split, Paolo Bonzini, 2016/03/08