From: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
To: qemu-devel@nongnu.org, kraxel@redhat.com,
marcandre.lureau@redhat.com, uril@redhat.com
Subject: [PATCH] usbredir: Prevent recursion in usbredir_write
Date: Wed, 18 Dec 2019 11:30:12 +0000 [thread overview]
Message-ID: <20191218113012.13331-1-dgilbert@redhat.com> (raw)
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
I've got a case where usbredir_write manages to call back into itself
via spice; this patch causes the recursion to fail (0 bytes) the write;
this seems to avoid the deadlock I was previously seeing.
I can't say I fully understand the interaction of usbredir and spice;
but there are a few similar guards in spice and usbredir
to catch other cases especially onces also related to spice_server_char_device_wakeup
This case seems to be triggered by repeated migration+repeated
reconnection of the viewer; but my debugging suggests the migration
finished before this hits.
The backtrace of the hang looks like:
reds_handle_ticket
reds_handle_other_links
reds_channel_do_link
red_channel_connect
spicevmc_connect
usbredir_create_parser
usbredirparser_do_write
usbredir_write
qemu_chr_fe_write
qemu_chr_write
qemu_chr_write_buffer
spice_chr_write
spice_server_char_device_wakeup
red_char_device_wakeup
red_char_device_write_to_device
vmc_write
usbredirparser_do_write
usbredir_write
qemu_chr_fe_write
qemu_chr_write
qemu_chr_write_buffer
qemu_mutex_lock_impl
and we fail as we lang through qemu_chr_write_buffer's lock
twice.
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1752320
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/usb/redirect.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index e0f5ca6f81..97f2c3a7da 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -113,6 +113,7 @@ struct USBRedirDevice {
/* Properties */
CharBackend cs;
bool enable_streams;
+ bool in_write;
uint8_t debug;
int32_t bootindex;
char *filter_str;
@@ -290,6 +291,13 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
return 0;
}
+ /* Recursion check */
+ if (dev->in_write) {
+ DPRINTF("usbredir_write recursion\n");
+ return 0;
+ }
+ dev->in_write = true;
+
r = qemu_chr_fe_write(&dev->cs, data, count);
if (r < count) {
if (!dev->watch) {
@@ -300,6 +308,7 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
r = 0;
}
}
+ dev->in_write = false;
return r;
}
--
2.23.0
next reply other threads:[~2019-12-18 11:31 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-18 11:30 Dr. David Alan Gilbert (git) [this message]
2020-01-07 7:29 ` [PATCH] usbredir: Prevent recursion in usbredir_write Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191218113012.13331-1-dgilbert@redhat.com \
--to=dgilbert@redhat.com \
--cc=kraxel@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=uril@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.