[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 22/38] ivshmem: Plug leaks on unplug, fix peer disconnect |
Date: |
Wed, 02 Mar 2016 20:19:37 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> 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().
When called from pci_ivshmem_exit(): perfectly fine.
When called from process_msg_disconnect(): invalid as long as
ivshmem-spec.txt doesn't assign a sane meaning to it. Let's make it an
error there, okay?
>> @@ -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
>>
>>