[Qemu-devel] [PATCH 0/9] char: fix segfault on chardev detach

[Qemu-devel] [PATCH 0/9] char: fix segfault on chardev detach

[Amit Shah]

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

Please review,

Amit Shah (9):
  char: remove watch callback on chardev detach from frontend
  char: introduce tcp_chr_detach()
  char: introduce fd_chr_detach()
  char: introduce pty_chr_detach()
  char: introduce udp_chr_detach()
  char: use the new fd_chr_detach to dedup code
  char: use the new pty_chr_detach to dedup code
  char: use the new udp_chr_detach to dedup code
  char: use the new tcp_chr_detach to dedup code

 include/sysemu/char.h |   1 +
 qemu-char.c           | 102 +++++++++++++++++++++++++++++---------------------
 2 files changed, 60 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/9] char: remove watch callback on chardev detach from frontend

[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 NULL pointer dereference.

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: <qemu-stable@nongnu.org>
Reported-by: Sibiao Luo <sluo@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 include/sysemu/char.h | 1 +
 qemu-char.c           | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 8053130..3400b04 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -69,6 +69,7 @@ struct CharDriverState {
     void (*chr_accept_input)(struct CharDriverState *chr);
     void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
     void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
+    void (*chr_detach)(struct CharDriverState *chr);
     void *opaque;
     char *label;
     char *filename;
diff --git a/qemu-char.c b/qemu-char.c
index 6259496..f27fdb6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -203,6 +203,9 @@ void qemu_chr_add_handlers(CharDriverState *s,
 
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
+        if (s->handler_opaque && s->chr_detach) {
+            s->chr_detach(s);
+        }
     } else {
         fe_open = 1;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()

[Amit Shah]

Remove any registered callbacks if a frontend is detached.

CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index f27fdb6..e235334 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2321,6 +2321,16 @@ typedef struct {
     int msgfd;
 } TCPCharDriver;
 
+static void tcp_chr_detach(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    if (s->tag) {
+        io_remove_watch_poll(s->tag);
+        s->tag = 0;
+    }
+}
+
 static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque);
 
 static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
@@ -2689,6 +2699,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->chr_detach = tcp_chr_detach;
     chr->get_msgfd = tcp_get_msgfd;
     chr->chr_add_client = tcp_chr_add_client;
     chr->chr_add_watch = tcp_chr_add_watch;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/9] char: introduce fd_chr_detach()

[Amit Shah]

Remove any registered callbacks if a frontend is detached.

CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index e235334..91ae1da 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -806,6 +806,17 @@ typedef struct FDCharDriver {
     QTAILQ_ENTRY(FDCharDriver) node;
 } FDCharDriver;
 
+
+static void fd_chr_detach(struct CharDriverState *chr)
+{
+    FDCharDriver *s = chr->opaque;
+
+    if (s->fd_in_tag) {
+        io_remove_watch_poll(s->fd_in_tag);
+        s->fd_in_tag = 0;
+    }
+}
+
 static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     FDCharDriver *s = chr->opaque;
@@ -913,6 +924,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     chr->chr_write = fd_chr_write;
     chr->chr_update_read_handler = fd_chr_update_read_handler;
     chr->chr_close = fd_chr_close;
+    chr->chr_detach = fd_chr_detach;
 
     return chr;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/9] char: introduce pty_chr_detach()

[Amit Shah]

Remove any registered callbacks if a frontend is detached.

CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 91ae1da..befecf2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1036,6 +1036,16 @@ typedef struct {
 static void pty_chr_update_read_handler(CharDriverState *chr);
 static void pty_chr_state(CharDriverState *chr, int connected);
 
+static void pty_chr_detach(struct CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->fd_tag) {
+        io_remove_watch_poll(s->fd_tag);
+        s->fd_tag = 0;
+    }
+}
+
 static gboolean pty_chr_timer(gpointer opaque)
 {
     struct CharDriverState *chr = opaque;
@@ -1215,6 +1225,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     chr->chr_update_read_handler = pty_chr_update_read_handler;
     chr->chr_close = pty_chr_close;
     chr->chr_add_watch = pty_chr_add_watch;
+    chr->chr_detach = pty_chr_detach;
     chr->explicit_be_open = true;
 
     s->fd = io_channel_from_fd(master_fd);
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/9] char: introduce udp_chr_detach()

[Amit Shah]

Remove any registered callbacks if a frontend is detached.

CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index befecf2..4b26ff9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2198,6 +2198,16 @@ typedef struct {
     int max_size;
 } NetCharDriver;
 
+static void udp_chr_detach(CharDriverState *chr)
+{
+    NetCharDriver *s = chr->opaque;
+
+    if (s->tag) {
+        io_remove_watch_poll(s->tag);
+        s->tag = 0;
+    }
+}
+
 static int udp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     NetCharDriver *s = chr->opaque;
@@ -2309,6 +2319,7 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
     chr->chr_write = udp_chr_write;
     chr->chr_update_read_handler = udp_chr_update_read_handler;
     chr->chr_close = udp_chr_close;
+    chr->chr_detach = udp_chr_detach;
     /* be isn't opened until we get a connection */
     chr->explicit_be_open = true;
     return chr;
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/9] char: use the new fd_chr_detach to dedup code

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 4b26ff9..88ed131 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -844,10 +844,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 (s->fd_in_tag) {
-            io_remove_watch_poll(s->fd_in_tag);
-            s->fd_in_tag = 0;
-        }
+        fd_chr_detach(chr);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }
@@ -877,11 +874,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (s->fd_in_tag) {
-        io_remove_watch_poll(s->fd_in_tag);
-        s->fd_in_tag = 0;
-    }
-
+    fd_chr_detach(chr);
     if (s->fd_in) {
         s->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll, fd_chr_read, chr);
     }
@@ -891,11 +884,7 @@ static void fd_chr_close(struct CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (s->fd_in_tag) {
-        io_remove_watch_poll(s->fd_in_tag);
-        s->fd_in_tag = 0;
-    }
-
+    fd_chr_detach(chr);
     if (s->fd_in) {
         g_io_channel_unref(s->fd_in);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/9] char: use the new pty_chr_detach to dedup code

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 88ed131..d667e8c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1141,10 +1141,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        if (s->fd_tag) {
-            io_remove_watch_poll(s->fd_tag);
-            s->fd_tag = 0;
-        }
+        pty_chr_detach(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
@@ -1169,10 +1166,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     PtyCharDriver *s = chr->opaque;
     int fd;
 
-    if (s->fd_tag) {
-        io_remove_watch_poll(s->fd_tag);
-        s->fd_tag = 0;
-    }
+    pty_chr_detach(chr);
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/9] char: use the new udp_chr_detach to dedup code

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d667e8c..2caab95 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2240,10 +2240,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 (s->tag) {
-            io_remove_watch_poll(s->tag);
-            s->tag = 0;
-        }
+        udp_chr_detach(chr);
         return FALSE;
     }
 
@@ -2261,11 +2258,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
 
-    if (s->tag) {
-        io_remove_watch_poll(s->tag);
-        s->tag = 0;
-    }
-
+    udp_chr_detach(chr);
     if (s->chan) {
         s->tag = io_add_watch_poll(s->chan, udp_chr_read_poll, udp_chr_read, chr);
     }
@@ -2274,10 +2267,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
-    if (s->tag) {
-        io_remove_watch_poll(s->tag);
-        s->tag = 0;
-    }
+
+    udp_chr_detach(chr);
     if (s->chan) {
         g_io_channel_unref(s->chan);
         closesocket(s->fd);
-- 
1.8.3.1

[Qemu-devel] [PATCH 9/9] char: use the new tcp_chr_detach to dedup code

[Amit Shah]

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2caab95..db256f8 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2514,10 +2514,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 (s->tag) {
-            io_remove_watch_poll(s->tag);
-            s->tag = 0;
-        }
+        tcp_chr_detach(chr);
         g_io_channel_unref(s->chan);
         s->chan = NULL;
         closesocket(s->fd);
@@ -2630,10 +2627,7 @@ static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        if (s->tag) {
-            io_remove_watch_poll(s->tag);
-            s->tag = 0;
-        }
+        tcp_chr_detach(chr);
         if (s->chan) {
             g_io_channel_unref(s->chan);
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()

[Gerd Hoffmann]

  Hi,

> +static void tcp_chr_detach(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    if (s->tag) {
> +        io_remove_watch_poll(s->tag);
> +        s->tag = 0;
> +    }
> +}

Lots of simliar functions in the other patches.

Doesn't it make sense to move the tag field from TCPCharDriver to
CharDriverState instead, so we don't need a new callback in the first
place?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()

[Amit Shah]

On (Wed) 28 Aug 2013 [09:09:47], Gerd Hoffmann wrote:
>   Hi,
> 
> > +static void tcp_chr_detach(CharDriverState *chr)
> > +{
> > +    TCPCharDriver *s = chr->opaque;
> > +
> > +    if (s->tag) {
> > +        io_remove_watch_poll(s->tag);
> > +        s->tag = 0;
> > +    }
> > +}
> 
> Lots of simliar functions in the other patches.
> 
> Doesn't it make sense to move the tag field from TCPCharDriver to
> CharDriverState instead, so we don't need a new callback in the first
> place?

Yep, I thought about it, but it might get tricky to handle it:
tcp needs two, one for listening sockets and one for connected ones.

We don't need to worry about the listening socket for this patchset,
should we then just keep that in the tcp struct, and use the tag as
the generic one in CharDriverState for all of the backends?

		Amit

Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()

[Gerd Hoffmann]

> We don't need to worry about the listening socket for this patchset,
> should we then just keep that in the tcp struct, and use the tag as
> the generic one in CharDriverState for all of the backends?

Yes, I think that will simplify the series.  And maybe name the one one
in CharDriverState 'fd_in_tag' to make more clear what it is.

cheers,
  Gerd