char: fix segfault on chardev detach

PATCH In review Amit Shah 2 participants 5 messages Aug 28, 2013 – Sep 25, 2013

Share & tools

Amit Shah

author

2013-08-28 12:37 UTC #0 lore raw

v3 cover char: fix segfault on chardev detach

This series fixes a segfault when a frontend is detached from a chardev while the chardev had a pending callback registered. Further details in patch 3.

Please review.

v3:
* fix whitespace issues in patch 1
v2:
* Move tag property to CharDriverState to simplify everything (Gerd)

Amit Shah (3):
char: move backends' io watch tag to CharDriverState
char: use common function to disable callbacks on chardev close
char: remove watch callback on chardev detach from frontend

 include/sysemu/char.h |  1 +
 qemu-char.c           | 82 +++++++++++++++++++--------------------------------
 2 files changed, 32 insertions(+), 51 deletions(-)

Signature · 1 line

1.8.3.1

Amit Shah

author

2013-08-28 12:37 UTC #1 lore raw

PATCH v3 1/3 char: move backends' io watch tag to CharDriverState

All the backends implement an io watcher tag for callbacks. Move it to CharDriverState from each backend's struct to make accessing the tag from backend-neutral functions easier.

This will be used later to cancel a callback on chardev detach from a frontend.

CC <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 include/sysemu/char.h |  1 +
 qemu-char.c           | 77 ++++++++++++++++++++++++++-------------------------
 2 files changed, 40 insertions(+), 38 deletions(-)

include/sysemu/char.h +1 -0

index 8053130..ad101d9 100644
@@ -78,6 +78,7 @@ struct CharDriverState {
     int explicit_be_open;
     int avail_connections;
     int is_mux;
+    guint fd_in_tag;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
 };

qemu-char.c +39 ~~-38~~

179 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Amit Shah

author

2013-08-28 12:37 UTC #2 lore raw

PATCH v3 2/3 char: use common function to disable callbacks on chardev close

In reply to Amit Shah

This deduplicates code used a lot of times.

CC <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 qemu-char.c | 62 +++++++++++++++++++------------------------------------------
 1 file changed, 19 insertions(+), 43 deletions(-)

qemu-char.c +19 ~~-43~~

index 306d2e1..3dcf322 100644
@@ -725,6 +725,14 @@ static void io_remove_watch_poll(guint tag)
     g_source_destroy(&iwp->parent);
 }
 
+static void remove_fd_in_watch(CharDriverState *chr)
+{
+    if (chr->fd_in_tag) {
+        io_remove_watch_poll(chr->fd_in_tag);
+        chr->fd_in_tag = 0;
+    }
+}
+
 #ifndef _WIN32
 static GIOChannel *io_channel_from_fd(int fd)
 {

@@ -829,10 +837,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     status = g_io_channel_read_chars(chan, (gchar *)buf,
                                      len, &bytes_read, NULL);
     if (status == G_IO_STATUS_EOF) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }

@@ -862,11 +867,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->fd_in) {
         chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll,
                                            fd_chr_read, chr);

@@ -877,11 +878,7 @@ static void fd_chr_close(struct CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->fd_in) {
         g_io_channel_unref(s->fd_in);
     }

@@ -1126,10 +1123,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
          * We check more frequently in case the guests sends data to

@@ -1155,10 +1149,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     PtyCharDriver *s = chr->opaque;
     int fd;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
+    remove_fd_in_watch(chr);
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);

@@ -2220,10 +2211,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     s->bufcnt = bytes_read;
     s->bufptr = s->bufcnt;
     if (status != G_IO_STATUS_NORMAL) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         return FALSE;
     }

@@ -2241,11 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->chan) {
         chr->fd_in_tag = io_add_watch_poll(s->chan, udp_chr_read_poll,
                                            udp_chr_read, chr);

@@ -2255,10 +2239,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
+
+    remove_fd_in_watch(chr);
     if (s->chan) {
         g_io_channel_unref(s->chan);
         closesocket(s->fd);

@@ -2493,10 +2475,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
         if (s->listen_chan) {
             s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
         }
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         g_io_channel_unref(s->chan);
         s->chan = NULL;
         closesocket(s->fd);

@@ -2610,10 +2589,7 @@ static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         if (s->chan) {
             g_io_channel_unref(s->chan);
         }

Signature · 1 line

1.8.3.1

Amit Shah

author

2013-08-28 12:37 UTC #3 lore raw

PATCH v3 3/3 char: remove watch callback on chardev detach from frontend

In reply to Amit Shah

If a frontend device releases the chardev (via unplug), the chr handlers are set to NULL via qdev's exit callbacks invoking qemu_chr_add_handlers(). If the chardev had a pending operation, a callback will be invoked, which will try to access data in the just-released frontend, causing a segfault.

Ensure the callbacks are disabled when frontends release chardevs.

This was seen when a virtio-serial port was unplugged when heavy guest->host IO was in progress (causing a callback to be registered). In the window in which the throttling was active, unplugging ports caused a qemu segfault.

https://bugzilla.redhat.com/show_bug.cgi?id=985205

CC <[email protected]>

Reported-by Sibiao Luo <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 qemu-char.c | 3 +++
 1 file changed, 3 insertions(+)

qemu-char.c +3 -0

index 3dcf322..347fa5e 100644
@@ -193,6 +193,8 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
     va_end(ap);
 }
 
+static void remove_fd_in_watch(CharDriverState *chr);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,

@@ -203,6 +205,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
 
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
+        remove_fd_in_watch(s);
     } else {
         fe_open = 1;
     }

Signature · 1 line

1.8.3.1

Michael Roth

Reviewed-by on the v3 series

2013-09-25 00:50 UTC #4 lore raw

In reply to Amit Shah

Quoting Amit Shah (2013-08-28 07:37:19)

Quoted 5 lines

This series fixes a segfault when a frontend is detached from a chardev while the chardev had a pending callback registered. Further details in patch 3.
Please review.

Reviewed-by Michael Roth <[email protected]>

Ping for 1.6.1

Quoted 17 lines

v3:
* fix whitespace issues in patch 1
v2:
* Move tag property to CharDriverState to simplify everything (Gerd)
Amit Shah (3):
char: move backends' io watch tag to CharDriverState
char: use common function to disable callbacks on chardev close
char: remove watch callback on chardev detach from frontend
 include/sysemu/char.h |  1 +
 qemu-char.c           | 82 +++++++++++++++++++--------------------------------
 2 files changed, 32 insertions(+), 51 deletions(-)
Signature · 1 line
1.8.3.1