| Message ID | 1375960178-10882-1-git-send-email-james.hogan@imgtec.com |
|---|---|
| State | New |
| Headers | show |
Quoting James Hogan (2013-08-08 06:09:38) > Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an > infinite recursion occurs when putting the monitor on a pty (-monitor > pty) and connecting a terminal to the slave port. > > This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to > qemu_chr_be_generic_open(). This event is captured by monitor_event() > which prints a welcome message to the character device. The flush of > that welcome message retriggers another open event in pty_chr_state() > because it checks s->connected, but only sets it to 1 after calling > qemu_chr_be_generic_open(). > > I've fixed this by setting s->connected = 1 before the call to > qemu_chr_be_generic_open() instead of after, so that the recursive > pty_chr_state() doesn't call it again. > > An example snippet of repeating backtrace: > ... > #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288 > #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322 > #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n", > ap=0x7f432be0) at qemu/monitor.c:339 > #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n") > at qemu/monitor.c:347 > #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699 > #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108 > #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113 > #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145 > #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121 > #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > at qemu/qemu-char.c:1063 > #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > at qemu/qemu-char.c:118 > ... > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > Cc: Anthony Liguori <aliguori@us.ibm.com> Thanks for the catch! I reproduced the bug and can confirm the this fixes it. Prior to bd5c51e, the code relied on deferring CHR_EVENT_OPENED via BH to ensure setting s->connected = 1 was visible by the time the event was processed, so this patch simply makes that ordering explicit. Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this > patch is acceptable it probably makes sense for it to follow the other > patch into stable too. > > qemu-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 16f3ad7..1be1cf6 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected) > s->timer_tag = 0; > } > if (!s->connected) { > - qemu_chr_be_generic_open(chr); > s->connected = 1; > + qemu_chr_be_generic_open(chr); > s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr); > } > } > -- > 1.8.1.2
Quoting Michael Roth (2013-08-08 09:03:08) > Quoting James Hogan (2013-08-08 06:09:38) > > Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an > > infinite recursion occurs when putting the monitor on a pty (-monitor > > pty) and connecting a terminal to the slave port. > > > > This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to > > qemu_chr_be_generic_open(). This event is captured by monitor_event() > > which prints a welcome message to the character device. The flush of > > that welcome message retriggers another open event in pty_chr_state() > > because it checks s->connected, but only sets it to 1 after calling > > qemu_chr_be_generic_open(). > > > > I've fixed this by setting s->connected = 1 before the call to > > qemu_chr_be_generic_open() instead of after, so that the recursive > > pty_chr_state() doesn't call it again. > > > > An example snippet of repeating backtrace: > > ... > > #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288 > > #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322 > > #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n", > > ap=0x7f432be0) at qemu/monitor.c:339 > > #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n") > > at qemu/monitor.c:347 > > #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699 > > #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108 > > #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113 > > #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145 > > #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121 > > #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > > at qemu/qemu-char.c:1063 > > #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > > at qemu/qemu-char.c:118 > > ... > > > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > > Cc: Anthony Liguori <aliguori@us.ibm.com> > > Thanks for the catch! > > I reproduced the bug and can confirm the this fixes it. > > Prior to bd5c51e, the code relied on deferring CHR_EVENT_OPENED via BH to > ensure setting s->connected = 1 was visible by the time the event was > processed, so this patch simply makes that ordering explicit. > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > > --- > > Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this > > patch is acceptable it probably makes sense for it to follow the other > > patch into stable too. Agreed, CC'ing stable. > > > > qemu-char.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/qemu-char.c b/qemu-char.c > > index 16f3ad7..1be1cf6 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected) > > s->timer_tag = 0; > > } > > if (!s->connected) { > > - qemu_chr_be_generic_open(chr); > > s->connected = 1; > > + qemu_chr_be_generic_open(chr); > > s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr); > > } > > } > > -- > > 1.8.1.2
Quoting James Hogan (2013-08-08 06:09:38) > Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an > infinite recursion occurs when putting the monitor on a pty (-monitor > pty) and connecting a terminal to the slave port. > > This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to > qemu_chr_be_generic_open(). This event is captured by monitor_event() > which prints a welcome message to the character device. The flush of > that welcome message retriggers another open event in pty_chr_state() > because it checks s->connected, but only sets it to 1 after calling > qemu_chr_be_generic_open(). > > I've fixed this by setting s->connected = 1 before the call to > qemu_chr_be_generic_open() instead of after, so that the recursive > pty_chr_state() doesn't call it again. > > An example snippet of repeating backtrace: > ... > #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288 > #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322 > #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n", > ap=0x7f432be0) at qemu/monitor.c:339 > #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n") > at qemu/monitor.c:347 > #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699 > #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108 > #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113 > #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145 > #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121 > #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > at qemu/qemu-char.c:1063 > #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) > at qemu/qemu-char.c:118 > ... > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > Cc: Anthony Liguori <aliguori@us.ibm.com> > --- > Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this > patch is acceptable it probably makes sense for it to follow the other > patch into stable too. Ping. Looking to pull this in for stable. > > qemu-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 16f3ad7..1be1cf6 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected) > s->timer_tag = 0; > } > if (!s->connected) { > - qemu_chr_be_generic_open(chr); > s->connected = 1; > + qemu_chr_be_generic_open(chr); > s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr); > } > } > -- > 1.8.1.2
Applied. Thanks. Regards, Anthony Liguori
diff --git a/qemu-char.c b/qemu-char.c index 16f3ad7..1be1cf6 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected) s->timer_tag = 0; } if (!s->connected) { - qemu_chr_be_generic_open(chr); s->connected = 1; + qemu_chr_be_generic_open(chr); s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr); } }
Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an infinite recursion occurs when putting the monitor on a pty (-monitor pty) and connecting a terminal to the slave port. This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to qemu_chr_be_generic_open(). This event is captured by monitor_event() which prints a welcome message to the character device. The flush of that welcome message retriggers another open event in pty_chr_state() because it checks s->connected, but only sets it to 1 after calling qemu_chr_be_generic_open(). I've fixed this by setting s->connected = 1 before the call to qemu_chr_be_generic_open() instead of after, so that the recursive pty_chr_state() doesn't call it again. An example snippet of repeating backtrace: ... #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288 #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322 #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n", ap=0x7f432be0) at qemu/monitor.c:339 #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n") at qemu/monitor.c:347 #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699 #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108 #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113 #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145 #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121 #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) at qemu/qemu-char.c:1063 #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720) at qemu/qemu-char.c:118 ... Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Michael Roth <mdroth@linux.vnet.ibm.com> Cc: Anthony Liguori <aliguori@us.ibm.com> --- Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this patch is acceptable it probably makes sense for it to follow the other patch into stable too. qemu-char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)