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

[Markus Armbruster]

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
>>
>>