Hi On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <arm...@redhat.com> 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 <pbonz...@redhat.com> > Signed-off-by: Markus Armbruster <arm...@redhat.com> > --- > 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