Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

On Wed, Sep 20, 2017 at 12:29:21PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 20, 2017 at 07:18:49PM +0800, Peter Xu wrote:
> > On Wed, Sep 20, 2017 at 12:03:09PM +0100, Daniel P. Berrange wrote:
> > > On Wed, Sep 20, 2017 at 06:49:58PM +0800, Peter Xu wrote:
> > > > On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> > > > > On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > > > > > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > > > > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > > > > > This is not a problem if we are only having one single loop 
> > > > > > > > thread like
> > > > > > > > before.  However, after per-monitor thread is introduced, this 
> > > > > > > > is not
> > > > > > > > true any more, and the race can happen.
> > > > > > > > 
> > > > > > > > The race can be triggered with "make check -j8" sometimes:
> > > > > > > > 
> > > > > > > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > > > > > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > > > > > > 
> > > > > > > > This patch keeps the reference for the watch object when 
> > > > > > > > creating in
> > > > > > > > io_add_watch_poll(), so that the object will never be released 
> > > > > > > > in the
> > > > > > > > context main loop, especially when the context loop is running 
> > > > > > > > in
> > > > > > > > another standalone thread.  Meanwhile, when we want to remove 
> > > > > > > > the watch
> > > > > > > > object, we always first detach the watch object from its owner 
> > > > > > > > context,
> > > > > > > > then we continue with the cleanup.
> > > > > > > > 
> > > > > > > > Without this patch, calling io_remove_watch_poll() in main loop 
> > > > > > > > thread
> > > > > > > > is not thread-safe, since the other per-monitor thread may be 
> > > > > > > > modifying
> > > > > > > > the watch object at the same time.
> > > > > > > 
> > > > > > > This doesn't feel right to me. Why is the main loop thread doing 
> > > > > > > anything
> > > > > > > at all with the Chardev, if there is a per-monitor thread ? The 
> > > > > > > Chardev
> > > > > > > code isn't thread safe so it isn't safe to have two separate 
> > > > > > > threads
> > > > > > > accessing the same Chardev. IOW, if we want a per-monitor thread, 
> > > > > > > then
> > > > > > > we must make sure the main thread never touches that monitor's 
> > > > > > > chardev
> > > > > > > at all.  While your patch here might have avoided the assertion 
> > > > > > > you
> > > > > > > mention above, I fear this is just papering over a fundamental 
> > > > > > > problem
> > > > > > > that still exists, that can only be solved by not letting the 
> > > > > > > mainloop
> > > > > > > touch the chardev at all.
> > > > > > 
> > > > > > The stack I encountered:
> > > > > > 
> > > > > > #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> > > > > > ../sysdeps/unix/sysv/linux/raise.c:54
> > > > > > #1  0x7f658234e36a in __GI_abort () at abort.c:89
> > > > > > #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> > > > > > assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> > > > > > file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > > > > > line=line@entry=91, function=function@entry=0x55c76345fd10 
> > > > > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > > > > > #3  0x7f6582345042 in __GI___assert_fail 
> > > > > > (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 
> > > > > > "/root/git/qemu/chardev/char-io.c", line=91, 
> > > > > > function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> 
> > > > > > "io_watch_poll_finalize") at assert.c:101
> > > > > > #4  0x55c7632c2be5 in io_watch_poll_finalize 
> > > > > > (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
> > > > > > #5  0x7f65847bb859 in g_source_unref_internal () at 
> > > > > > /lib64/libglib-2.0.so.0
> > > > > > #6  0x7f65847bca29 in g_source_destroy_internal () at 
> > > > > > /lib64/libglib-2.0.so.0
> > > > > > #7  0x55c7632c2d30 in io_remove_watch_poll 
> > > > > > (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
> > > > > > #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) 
> > > > > > at /root/git/qemu/chardev/char-io.c:145
> > > > > > #9  0x55c7632c2368 in qemu_chr_fe_set_handlers 
> > > > > > (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, 
> > > > > > be_change=0x0, opaque=0x0, context=0x0, set_open=true)
> > > > > > at /root/git/qemu/chardev/char-fe.c:267
> > > > > > #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, 
> > > > > > del=false) at /root/git/qemu/chardev/char-fe.c:231
> > > > > > #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) 
> > > > > > at /root/git/qemu/monitor.c:600
> > > > > > #12 0x55c762e340ec in monitor_cleanup () at 
> > > > > > /root/git/qemu/monitor.c:4346
> > > > > > #13 

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Daniel P. Berrange]

On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > This is not a problem if we are only having one single loop thread like
> > > before.  However, after per-monitor thread is introduced, this is not
> > > true any more, and the race can happen.
> > > 
> > > The race can be triggered with "make check -j8" sometimes:
> > > 
> > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > 
> > > This patch keeps the reference for the watch object when creating in
> > > io_add_watch_poll(), so that the object will never be released in the
> > > context main loop, especially when the context loop is running in
> > > another standalone thread.  Meanwhile, when we want to remove the watch
> > > object, we always first detach the watch object from its owner context,
> > > then we continue with the cleanup.
> > > 
> > > Without this patch, calling io_remove_watch_poll() in main loop thread
> > > is not thread-safe, since the other per-monitor thread may be modifying
> > > the watch object at the same time.
> > 
> > This doesn't feel right to me. Why is the main loop thread doing anything
> > at all with the Chardev, if there is a per-monitor thread ? The Chardev
> > code isn't thread safe so it isn't safe to have two separate threads
> > accessing the same Chardev. IOW, if we want a per-monitor thread, then
> > we must make sure the main thread never touches that monitor's chardev
> > at all.  While your patch here might have avoided the assertion you
> > mention above, I fear this is just papering over a fundamental problem
> > that still exists, that can only be solved by not letting the mainloop
> > touch the chardev at all.
> 
> The stack I encountered:
> 
> #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/unix/sysv/linux/raise.c:54
> #1  0x7f658234e36a in __GI_abort () at abort.c:89
> #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> line=line@entry=91, function=function@entry=0x55c76345fd10 
> <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> #3  0x7f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 
> "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> 
> "io_watch_poll_finalize") at assert.c:101
> #4  0x55c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at 
> /root/git/qemu/chardev/char-io.c:91
> #5  0x7f65847bb859 in g_source_unref_internal () at 
> /lib64/libglib-2.0.so.0
> #6  0x7f65847bca29 in g_source_destroy_internal () at 
> /lib64/libglib-2.0.so.0
> #7  0x55c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at 
> /root/git/qemu/chardev/char-io.c:139
> #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
> /root/git/qemu/chardev/char-io.c:145
> #9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
> fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, 
> context=0x0, set_open=true)
> at /root/git/qemu/chardev/char-fe.c:267
> #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at 
> /root/git/qemu/chardev/char-fe.c:231
> #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at 
> /root/git/qemu/monitor.c:600
> #12 0x55c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
> #13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
> envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> 
> So it's destroying the CharBackend, but it'll then call
> qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main
thread - it needs to be called from the monitor thread, unless it can
guarantee that the monitor thread has already exited, which seems unlikely

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Daniel P. Berrange]

On Wed, Sep 20, 2017 at 07:18:49PM +0800, Peter Xu wrote:
> On Wed, Sep 20, 2017 at 12:03:09PM +0100, Daniel P. Berrange wrote:
> > On Wed, Sep 20, 2017 at 06:49:58PM +0800, Peter Xu wrote:
> > > On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> > > > On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > > > > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > > > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > > > > This is not a problem if we are only having one single loop 
> > > > > > > thread like
> > > > > > > before.  However, after per-monitor thread is introduced, this is 
> > > > > > > not
> > > > > > > true any more, and the race can happen.
> > > > > > > 
> > > > > > > The race can be triggered with "make check -j8" sometimes:
> > > > > > > 
> > > > > > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > > > > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > > > > > 
> > > > > > > This patch keeps the reference for the watch object when creating 
> > > > > > > in
> > > > > > > io_add_watch_poll(), so that the object will never be released in 
> > > > > > > the
> > > > > > > context main loop, especially when the context loop is running in
> > > > > > > another standalone thread.  Meanwhile, when we want to remove the 
> > > > > > > watch
> > > > > > > object, we always first detach the watch object from its owner 
> > > > > > > context,
> > > > > > > then we continue with the cleanup.
> > > > > > > 
> > > > > > > Without this patch, calling io_remove_watch_poll() in main loop 
> > > > > > > thread
> > > > > > > is not thread-safe, since the other per-monitor thread may be 
> > > > > > > modifying
> > > > > > > the watch object at the same time.
> > > > > > 
> > > > > > This doesn't feel right to me. Why is the main loop thread doing 
> > > > > > anything
> > > > > > at all with the Chardev, if there is a per-monitor thread ? The 
> > > > > > Chardev
> > > > > > code isn't thread safe so it isn't safe to have two separate threads
> > > > > > accessing the same Chardev. IOW, if we want a per-monitor thread, 
> > > > > > then
> > > > > > we must make sure the main thread never touches that monitor's 
> > > > > > chardev
> > > > > > at all.  While your patch here might have avoided the assertion you
> > > > > > mention above, I fear this is just papering over a fundamental 
> > > > > > problem
> > > > > > that still exists, that can only be solved by not letting the 
> > > > > > mainloop
> > > > > > touch the chardev at all.
> > > > > 
> > > > > The stack I encountered:
> > > > > 
> > > > > #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> > > > > ../sysdeps/unix/sysv/linux/raise.c:54
> > > > > #1  0x7f658234e36a in __GI_abort () at abort.c:89
> > > > > #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> > > > > assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> > > > > file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > > > > line=line@entry=91, function=function@entry=0x55c76345fd10 
> > > > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > > > > #3  0x7f6582345042 in __GI___assert_fail 
> > > > > (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 
> > > > > "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 
> > > > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
> > > > > #4  0x55c7632c2be5 in io_watch_poll_finalize 
> > > > > (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
> > > > > #5  0x7f65847bb859 in g_source_unref_internal () at 
> > > > > /lib64/libglib-2.0.so.0
> > > > > #6  0x7f65847bca29 in g_source_destroy_internal () at 
> > > > > /lib64/libglib-2.0.so.0
> > > > > #7  0x55c7632c2d30 in io_remove_watch_poll 
> > > > > (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
> > > > > #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
> > > > > /root/git/qemu/chardev/char-io.c:145
> > > > > #9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
> > > > > fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, 
> > > > > opaque=0x0, context=0x0, set_open=true)
> > > > > at /root/git/qemu/chardev/char-fe.c:267
> > > > > #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, 
> > > > > del=false) at /root/git/qemu/chardev/char-fe.c:231
> > > > > #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) 
> > > > > at /root/git/qemu/monitor.c:600
> > > > > #12 0x55c762e340ec in monitor_cleanup () at 
> > > > > /root/git/qemu/monitor.c:4346
> > > > > #13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
> > > > > envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> > > > > 
> > > > > So it's destroying the CharBackend, but it'll then call
> > > > > qemu_chr_fe_set_handlers() which finally tries to remove the watch 
> > > > > poll.
> > > > 
> 

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > This is not a problem if we are only having one single loop thread like
> > before.  However, after per-monitor thread is introduced, this is not
> > true any more, and the race can happen.
> > 
> > The race can be triggered with "make check -j8" sometimes:
> > 
> >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > 
> > This patch keeps the reference for the watch object when creating in
> > io_add_watch_poll(), so that the object will never be released in the
> > context main loop, especially when the context loop is running in
> > another standalone thread.  Meanwhile, when we want to remove the watch
> > object, we always first detach the watch object from its owner context,
> > then we continue with the cleanup.
> > 
> > Without this patch, calling io_remove_watch_poll() in main loop thread
> > is not thread-safe, since the other per-monitor thread may be modifying
> > the watch object at the same time.
> 
> This doesn't feel right to me. Why is the main loop thread doing anything
> at all with the Chardev, if there is a per-monitor thread ? The Chardev
> code isn't thread safe so it isn't safe to have two separate threads
> accessing the same Chardev. IOW, if we want a per-monitor thread, then
> we must make sure the main thread never touches that monitor's chardev
> at all.  While your patch here might have avoided the assertion you
> mention above, I fear this is just papering over a fundamental problem
> that still exists, that can only be solved by not letting the mainloop
> touch the chardev at all.

The stack I encountered:

#0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:54
#1  0x7f658234e36a in __GI_abort () at abort.c:89
#2  0x7f6582344f97 in __assert_fail_base (fmt=, 
assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
line=line@entry=91, function=function@entry=0x55c76345fd10 
<__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x7f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 
"iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> 
"io_watch_poll_finalize") at assert.c:101
#4  0x55c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at 
/root/git/qemu/chardev/char-io.c:91
#5  0x7f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x7f65847bca29 in g_source_destroy_internal () at 
/lib64/libglib-2.0.so.0
#7  0x55c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at 
/root/git/qemu/chardev/char-io.c:139
#8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
/root/git/qemu/chardev/char-io.c:145
#9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, 
context=0x0, set_open=true)
at /root/git/qemu/chardev/char-fe.c:267
#10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at 
/root/git/qemu/chardev/char-fe.c:231
#11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at 
/root/git/qemu/monitor.c:600
#12 0x55c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

So it's destroying the CharBackend, but it'll then call
qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

If without current patch, I can still encounter the same crash when
doing "make check -j8".

> 
> > 
> > Reviewed-by: Marc-André Lureau 
> > Signed-off-by: Peter Xu 
> > ---
> >  chardev/char-io.c | 15 +--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/chardev/char-io.c b/chardev/char-io.c
> > index f810524..3828c20 100644
> > --- a/chardev/char-io.c
> > +++ b/chardev/char-io.c
> > @@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
> >  g_free(name);
> >  
> >  g_source_attach(>parent, context);
> > -g_source_unref(>parent);
> >  return (GSource *)iwp;
> >  }
> >  
> > @@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
> >  IOWatchPoll *iwp;
> >  
> >  iwp = io_watch_poll_from_source(source);
> > +
> > +/*
> > + * Here the order of destruction really matters.  We need to first
> > + * detach the IOWatchPoll object from the context (which may still
> > + * be running in another loop thread), only after that could we
> > + * continue to operate on iwp->src, or there may be race condition
> > + * between current thread and the context loop thread.
> > + *
> > + * Let's 

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

On Wed, Sep 20, 2017 at 12:03:09PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 20, 2017 at 06:49:58PM +0800, Peter Xu wrote:
> > On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> > > On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > > > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > > > This is not a problem if we are only having one single loop thread 
> > > > > > like
> > > > > > before.  However, after per-monitor thread is introduced, this is 
> > > > > > not
> > > > > > true any more, and the race can happen.
> > > > > > 
> > > > > > The race can be triggered with "make check -j8" sometimes:
> > > > > > 
> > > > > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > > > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > > > > 
> > > > > > This patch keeps the reference for the watch object when creating in
> > > > > > io_add_watch_poll(), so that the object will never be released in 
> > > > > > the
> > > > > > context main loop, especially when the context loop is running in
> > > > > > another standalone thread.  Meanwhile, when we want to remove the 
> > > > > > watch
> > > > > > object, we always first detach the watch object from its owner 
> > > > > > context,
> > > > > > then we continue with the cleanup.
> > > > > > 
> > > > > > Without this patch, calling io_remove_watch_poll() in main loop 
> > > > > > thread
> > > > > > is not thread-safe, since the other per-monitor thread may be 
> > > > > > modifying
> > > > > > the watch object at the same time.
> > > > > 
> > > > > This doesn't feel right to me. Why is the main loop thread doing 
> > > > > anything
> > > > > at all with the Chardev, if there is a per-monitor thread ? The 
> > > > > Chardev
> > > > > code isn't thread safe so it isn't safe to have two separate threads
> > > > > accessing the same Chardev. IOW, if we want a per-monitor thread, then
> > > > > we must make sure the main thread never touches that monitor's chardev
> > > > > at all.  While your patch here might have avoided the assertion you
> > > > > mention above, I fear this is just papering over a fundamental problem
> > > > > that still exists, that can only be solved by not letting the mainloop
> > > > > touch the chardev at all.
> > > > 
> > > > The stack I encountered:
> > > > 
> > > > #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> > > > ../sysdeps/unix/sysv/linux/raise.c:54
> > > > #1  0x7f658234e36a in __GI_abort () at abort.c:89
> > > > #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> > > > assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> > > > file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > > > line=line@entry=91, function=function@entry=0x55c76345fd10 
> > > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > > > #3  0x7f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 
> > > > "iwp->src == NULL", file=0x55c76345fcc0 
> > > > "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 
> > > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
> > > > #4  0x55c7632c2be5 in io_watch_poll_finalize 
> > > > (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
> > > > #5  0x7f65847bb859 in g_source_unref_internal () at 
> > > > /lib64/libglib-2.0.so.0
> > > > #6  0x7f65847bca29 in g_source_destroy_internal () at 
> > > > /lib64/libglib-2.0.so.0
> > > > #7  0x55c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) 
> > > > at /root/git/qemu/chardev/char-io.c:139
> > > > #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
> > > > /root/git/qemu/chardev/char-io.c:145
> > > > #9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
> > > > fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, 
> > > > context=0x0, set_open=true)
> > > > at /root/git/qemu/chardev/char-fe.c:267
> > > > #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, 
> > > > del=false) at /root/git/qemu/chardev/char-fe.c:231
> > > > #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at 
> > > > /root/git/qemu/monitor.c:600
> > > > #12 0x55c762e340ec in monitor_cleanup () at 
> > > > /root/git/qemu/monitor.c:4346
> > > > #13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
> > > > envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> > > > 
> > > > So it's destroying the CharBackend, but it'll then call
> > > > qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.
> > > 
> > > Ok that code is broken - it must not call monitor_cleanup from the main
> > > thread - it needs to be called from the monitor thread, unless it can
> > > guarantee that the monitor thread has already exited, which seems unlikely
> > 
> > The problem is that not all monitors are parsed in the IO 

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Daniel P. Berrange]

On Wed, Sep 20, 2017 at 06:49:58PM +0800, Peter Xu wrote:
> On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> > On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > > This is not a problem if we are only having one single loop thread 
> > > > > like
> > > > > before.  However, after per-monitor thread is introduced, this is not
> > > > > true any more, and the race can happen.
> > > > > 
> > > > > The race can be triggered with "make check -j8" sometimes:
> > > > > 
> > > > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > > > 
> > > > > This patch keeps the reference for the watch object when creating in
> > > > > io_add_watch_poll(), so that the object will never be released in the
> > > > > context main loop, especially when the context loop is running in
> > > > > another standalone thread.  Meanwhile, when we want to remove the 
> > > > > watch
> > > > > object, we always first detach the watch object from its owner 
> > > > > context,
> > > > > then we continue with the cleanup.
> > > > > 
> > > > > Without this patch, calling io_remove_watch_poll() in main loop thread
> > > > > is not thread-safe, since the other per-monitor thread may be 
> > > > > modifying
> > > > > the watch object at the same time.
> > > > 
> > > > This doesn't feel right to me. Why is the main loop thread doing 
> > > > anything
> > > > at all with the Chardev, if there is a per-monitor thread ? The Chardev
> > > > code isn't thread safe so it isn't safe to have two separate threads
> > > > accessing the same Chardev. IOW, if we want a per-monitor thread, then
> > > > we must make sure the main thread never touches that monitor's chardev
> > > > at all.  While your patch here might have avoided the assertion you
> > > > mention above, I fear this is just papering over a fundamental problem
> > > > that still exists, that can only be solved by not letting the mainloop
> > > > touch the chardev at all.
> > > 
> > > The stack I encountered:
> > > 
> > > #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> > > ../sysdeps/unix/sysv/linux/raise.c:54
> > > #1  0x7f658234e36a in __GI_abort () at abort.c:89
> > > #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> > > assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> > > file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > > line=line@entry=91, function=function@entry=0x55c76345fd10 
> > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > > #3  0x7f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 
> > > "iwp->src == NULL", file=0x55c76345fcc0 
> > > "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 
> > > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
> > > #4  0x55c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) 
> > > at /root/git/qemu/chardev/char-io.c:91
> > > #5  0x7f65847bb859 in g_source_unref_internal () at 
> > > /lib64/libglib-2.0.so.0
> > > #6  0x7f65847bca29 in g_source_destroy_internal () at 
> > > /lib64/libglib-2.0.so.0
> > > #7  0x55c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at 
> > > /root/git/qemu/chardev/char-io.c:139
> > > #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
> > > /root/git/qemu/chardev/char-io.c:145
> > > #9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
> > > fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, 
> > > context=0x0, set_open=true)
> > > at /root/git/qemu/chardev/char-fe.c:267
> > > #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, 
> > > del=false) at /root/git/qemu/chardev/char-fe.c:231
> > > #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at 
> > > /root/git/qemu/monitor.c:600
> > > #12 0x55c762e340ec in monitor_cleanup () at 
> > > /root/git/qemu/monitor.c:4346
> > > #13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
> > > envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> > > 
> > > So it's destroying the CharBackend, but it'll then call
> > > qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.
> > 
> > Ok that code is broken - it must not call monitor_cleanup from the main
> > thread - it needs to be called from the monitor thread, unless it can
> > guarantee that the monitor thread has already exited, which seems unlikely
> 
> The problem is that not all monitors are parsed in the IO thread, but
> only those with use_io_thr=true set.
> 
> How about I move the calls of monitor_data_destroy() into that monitor
> IO thread when use_io_thr=true?  And for the rest, I think they still
> need to be destroyed in the main thread.

I think having the monitor sometimes run in the 

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > This is not a problem if we are only having one single loop thread like
> > > > before.  However, after per-monitor thread is introduced, this is not
> > > > true any more, and the race can happen.
> > > > 
> > > > The race can be triggered with "make check -j8" sometimes:
> > > > 
> > > >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > > 
> > > > This patch keeps the reference for the watch object when creating in
> > > > io_add_watch_poll(), so that the object will never be released in the
> > > > context main loop, especially when the context loop is running in
> > > > another standalone thread.  Meanwhile, when we want to remove the watch
> > > > object, we always first detach the watch object from its owner context,
> > > > then we continue with the cleanup.
> > > > 
> > > > Without this patch, calling io_remove_watch_poll() in main loop thread
> > > > is not thread-safe, since the other per-monitor thread may be modifying
> > > > the watch object at the same time.
> > > 
> > > This doesn't feel right to me. Why is the main loop thread doing anything
> > > at all with the Chardev, if there is a per-monitor thread ? The Chardev
> > > code isn't thread safe so it isn't safe to have two separate threads
> > > accessing the same Chardev. IOW, if we want a per-monitor thread, then
> > > we must make sure the main thread never touches that monitor's chardev
> > > at all.  While your patch here might have avoided the assertion you
> > > mention above, I fear this is just papering over a fundamental problem
> > > that still exists, that can only be solved by not letting the mainloop
> > > touch the chardev at all.
> > 
> > The stack I encountered:
> > 
> > #0  0x7f658234c765 in __GI_raise (sig=sig@entry=6) at 
> > ../sysdeps/unix/sysv/linux/raise.c:54
> > #1  0x7f658234e36a in __GI_abort () at abort.c:89
> > #2  0x7f6582344f97 in __assert_fail_base (fmt=, 
> > assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", 
> > file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > line=line@entry=91, function=function@entry=0x55c76345fd10 
> > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > #3  0x7f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 
> > "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", 
> > line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> 
> > "io_watch_poll_finalize") at assert.c:101
> > #4  0x55c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at 
> > /root/git/qemu/chardev/char-io.c:91
> > #5  0x7f65847bb859 in g_source_unref_internal () at 
> > /lib64/libglib-2.0.so.0
> > #6  0x7f65847bca29 in g_source_destroy_internal () at 
> > /lib64/libglib-2.0.so.0
> > #7  0x55c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at 
> > /root/git/qemu/chardev/char-io.c:139
> > #8  0x55c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at 
> > /root/git/qemu/chardev/char-io.c:145
> > #9  0x55c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, 
> > fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, 
> > context=0x0, set_open=true)
> > at /root/git/qemu/chardev/char-fe.c:267
> > #10 0x55c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) 
> > at /root/git/qemu/chardev/char-fe.c:231
> > #11 0x55c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at 
> > /root/git/qemu/monitor.c:600
> > #12 0x55c762e340ec in monitor_cleanup () at 
> > /root/git/qemu/monitor.c:4346
> > #13 0x55c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, 
> > envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> > 
> > So it's destroying the CharBackend, but it'll then call
> > qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.
> 
> Ok that code is broken - it must not call monitor_cleanup from the main
> thread - it needs to be called from the monitor thread, unless it can
> guarantee that the monitor thread has already exited, which seems unlikely

The problem is that not all monitors are parsed in the IO thread, but
only those with use_io_thr=true set.

How about I move the calls of monitor_data_destroy() into that monitor
IO thread when use_io_thr=true?  And for the rest, I think they still
need to be destroyed in the main thread.

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

-- 
Peter Xu

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Daniel P. Berrange]

On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> This is not a problem if we are only having one single loop thread like
> before.  However, after per-monitor thread is introduced, this is not
> true any more, and the race can happen.
> 
> The race can be triggered with "make check -j8" sometimes:
> 
>   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
>   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> 
> This patch keeps the reference for the watch object when creating in
> io_add_watch_poll(), so that the object will never be released in the
> context main loop, especially when the context loop is running in
> another standalone thread.  Meanwhile, when we want to remove the watch
> object, we always first detach the watch object from its owner context,
> then we continue with the cleanup.
> 
> Without this patch, calling io_remove_watch_poll() in main loop thread
> is not thread-safe, since the other per-monitor thread may be modifying
> the watch object at the same time.

This doesn't feel right to me. Why is the main loop thread doing anything
at all with the Chardev, if there is a per-monitor thread ? The Chardev
code isn't thread safe so it isn't safe to have two separate threads
accessing the same Chardev. IOW, if we want a per-monitor thread, then
we must make sure the main thread never touches that monitor's chardev
at all.  While your patch here might have avoided the assertion you
mention above, I fear this is just papering over a fundamental problem
that still exists, that can only be solved by not letting the mainloop
touch the chardev at all.

> 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Peter Xu 
> ---
>  chardev/char-io.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index f810524..3828c20 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
>  g_free(name);
>  
>  g_source_attach(>parent, context);
> -g_source_unref(>parent);
>  return (GSource *)iwp;
>  }
>  
> @@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
>  IOWatchPoll *iwp;
>  
>  iwp = io_watch_poll_from_source(source);
> +
> +/*
> + * Here the order of destruction really matters.  We need to first
> + * detach the IOWatchPoll object from the context (which may still
> + * be running in another loop thread), only after that could we
> + * continue to operate on iwp->src, or there may be race condition
> + * between current thread and the context loop thread.
> + *
> + * Let's blame the glib bug mentioned in commit 2b3167 (again) for
> + * this extra complexity.
> + */
> +g_source_destroy(>parent);
>  if (iwp->src) {
>  g_source_destroy(iwp->src);
>  g_source_unref(iwp->src);
>  iwp->src = NULL;
>  }
> -g_source_destroy(>parent);
> +g_source_unref(>parent);
>  }
>  
>  void remove_fd_in_watch(Chardev *chr)
> -- 
> 2.7.4
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

On Tue, Sep 19, 2017 at 02:59:37PM -0500, Eric Blake wrote:
> On 09/14/2017 02:50 AM, Peter Xu wrote:
> > This is not a problem if we are only having one single loop thread like
> > before.  However, after per-monitor thread is introduced, this is not
> > true any more, and the race can happen.
> > 
> > The race can be triggered with "make check -j8" sometimes:
> > 
> >   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> >   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > 
> > This patch keeps the reference for the watch object when creating in
> > io_add_watch_poll(), so that the object will never be released in the
> > context main loop, especially when the context loop is running in
> > another standalone thread.  Meanwhile, when we want to remove the watch
> > object, we always first detach the watch object from its owner context,
> > then we continue with the cleanup.
> > 
> > Without this patch, calling io_remove_watch_poll() in main loop thread
> > is not thread-safe, since the other per-monitor thread may be modifying
> > the watch object at the same time.
> > 
> > Reviewed-by: Marc-André Lureau 
> > Signed-off-by: Peter Xu 
> > ---
> 
> > + * Let's blame the glib bug mentioned in commit 2b3167 (again) for
> 
> That 6-char commit id may become ambiguous soon (it's still rare to see
> ambiguity with an 8-char id, although I've seen it more in recent times
> than in the past; and git itself has moved from a 7-char default
> abbreviation length in the 2.0 days to what is now a 10-char default
> abbreviation in 2.13.5).

Thanks for noticing this.  I'll use 10 chars in next post.

-- 
Peter Xu

Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Eric Blake]

On 09/14/2017 02:50 AM, Peter Xu wrote:
> This is not a problem if we are only having one single loop thread like
> before.  However, after per-monitor thread is introduced, this is not
> true any more, and the race can happen.
> 
> The race can be triggered with "make check -j8" sometimes:
> 
>   qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
>   io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> 
> This patch keeps the reference for the watch object when creating in
> io_add_watch_poll(), so that the object will never be released in the
> context main loop, especially when the context loop is running in
> another standalone thread.  Meanwhile, when we want to remove the watch
> object, we always first detach the watch object from its owner context,
> then we continue with the cleanup.
> 
> Without this patch, calling io_remove_watch_poll() in main loop thread
> is not thread-safe, since the other per-monitor thread may be modifying
> the watch object at the same time.
> 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Peter Xu 
> ---

> + * Let's blame the glib bug mentioned in commit 2b3167 (again) for

That 6-char commit id may become ambiguous soon (it's still rare to see
ambiguity with an 8-char id, although I've seen it more in recent times
than in the past; and git itself has moved from a 7-char default
abbreviation length in the 2.0 days to what is now a 10-char default
abbreviation in 2.13.5).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

This is not a problem if we are only having one single loop thread like
before.  However, after per-monitor thread is introduced, this is not
true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

  qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
  io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in
io_add_watch_poll(), so that the object will never be released in the
context main loop, especially when the context loop is running in
another standalone thread.  Meanwhile, when we want to remove the watch
object, we always first detach the watch object from its owner context,
then we continue with the cleanup.

Without this patch, calling io_remove_watch_poll() in main loop thread
is not thread-safe, since the other per-monitor thread may be modifying
the watch object at the same time.

Reviewed-by: Marc-André Lureau 
Signed-off-by: Peter Xu 
---
 chardev/char-io.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index f810524..3828c20 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
 g_free(name);
 
 g_source_attach(>parent, context);
-g_source_unref(>parent);
 return (GSource *)iwp;
 }
 
@@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
 IOWatchPoll *iwp;
 
 iwp = io_watch_poll_from_source(source);
+
+/*
+ * Here the order of destruction really matters.  We need to first
+ * detach the IOWatchPoll object from the context (which may still
+ * be running in another loop thread), only after that could we
+ * continue to operate on iwp->src, or there may be race condition
+ * between current thread and the context loop thread.
+ *
+ * Let's blame the glib bug mentioned in commit 2b3167 (again) for
+ * this extra complexity.
+ */
+g_source_destroy(>parent);
 if (iwp->src) {
 g_source_destroy(iwp->src);
 g_source_unref(iwp->src);
 iwp->src = NULL;
 }
-g_source_destroy(>parent);
+g_source_unref(>parent);
 }
 
 void remove_fd_in_watch(Chardev *chr)
-- 
2.7.4