Re: A bug of Monitor Chardev ?

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> On Fri, May 21, 2021 at 09:25:52AM +0200, Markus Armbruster wrote:
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > > On Wed, May 19, 2021 at 08:17:51PM +0400, Marc-André Lureau wrote:
> > >> Hi
> > >> 
> > >> On Mon, May 17, 2021 at 11:11 AM Longpeng (Mike, Cloud Infrastructure
> > >> Service Product Dept.) <longpeng2@huawei.com> wrote:
> > >> 
> > >> > We find a race during QEMU starting, which would case the QEMU process
> > >> > coredump.
> > >> >
> > >> > <main loop>                             |    <MON iothread>
> > >> >                                         |
> > >> > [1] create MON chardev                  |
> > >> > qemu_create_early_backends              |
> > >> >   chardev_init_func                     |
> > >> >                                         |
> > >> > [2] create MON iothread                 |
> > >> > qemu_create_late_backends               |
> > >> >   mon_init_func                         |
> > >> >         aio_bh_schedule----------------------->
> > >> > monitor_qmp_setup_handlers_bh
> > >> > [3] enter main loog                     |    tcp_chr_update_read_handler
> > >> > (* A client come in, e.g. Libvirt *)    |      update_ioc_handlers
> > >> >
> > >> tcp_chr_new_client                      |
> > >> >   update_ioc_handlers                   |
> > >> >                                         |
> > >> >     [4] create new hup_source           |
> > >> >         s->hup_source = *PTR1*          |
> > >> >           g_source_attach(s->hup_source)|
> > >> >                                         |        [5]
> > >> > remove_hup_source(*PTR1*)
> > >> >                                         |            (create new
> > >> > hup_source)
> > >> >                                         |             s->hup_source =
> > >> > *PTR2*
> > >> >         [6] g_source_attach_unlocked    |
> > >> >               *PTR1* is freed by [5]    |
> > >> >
> > >> > Do you have any suggestion to fix this bug ? Thanks!
> > >> >
> > >> >
> > >> I see.. I think the simplest would be for the chardev to not be dispatched
> > >> in the original thread after monitor_init_qmp(). It looks like this should
> > >> translate at least to calling qio_net_listener_set_client_func_full() with
> > >> NULL handlers. I can't see where we could fit that in the chardev API.
> > >> Perhaps add a new qemu_chr_be_disable_handlers() (until
> > >> update_read_handlers is called again to enable them)?
> > >> 
> > >> Daniel? Paolo?
> > >
> > > IIUC, the problem is:
> > >
> > >   - when we first create the chardev, its IO watches are setup with
> > >     the default (NULL) GMainContext which is processed by the main
> > >     thread
> > >
> > >   - when we create the monitor, we re-initialize the chardev to
> > >     attach its IO watches to a custom GMainCOntext associated with
> > >     the monitor thread.
> > >
> > >   - The re-initialization is happening in a bottom half that runs
> > >     in the monitor thread, thus the main thread can already start
> > >     processing an IO event in parallel
> > >
> > > Looking at the code in qmp.c monitor_init_qmp method it has a
> > > comment:
> > >
> > >         /*
> > >          * We can't call qemu_chr_fe_set_handlers() directly here
> > >          * since chardev might be running in the monitor I/O
> > >          * thread.  Schedule a bottom half.
> > >          */
> > >
> > > AFAICT, that comment is wrong. monitor_init_qmp is called from
> > > monitor_init, which is called from monitor_init_opts, which is
> > > called from qemu_create_late_backends, which runs in the main
> > > thread.
> > 
> > Goes back to commit a5ed352596a8b7eb2f9acce34371b944ac3056c4
> > Author: Peter Xu <peterx@redhat.com>
> > Date:   Fri Mar 9 16:59:52 2018 +0800
> > 
> >     monitor: allow using IO thread for parsing
> >     
> >     For each Monitor, add one field "use_io_thr" to show whether it will be
> >     using the dedicated monitor IO thread to handle input/output.  When set,
> >     monitor IO parsing work will be offloaded to the dedicated monitor IO
> >     thread, rather than the original main loop thread.
> >     
> >     This only works for QMP.  HMP will always be run on the main loop
> >     thread.
> >     
> >     Currently we're still keeping use_io_thr off always.  Will turn it on
> >     later at some point.
> >     
> >     One thing to mention is that we cannot set use_io_thr for every QMP
> >     monitor.  The problem is that MUXed typed chardevs may not work well
> >     with it now. When MUX is used, frontend of chardev can be the monitor
> >     plus something else.  The only thing we know would be safe to be run
> >     outside main thread so far is the monitor frontend. All the rest of the
> >     frontends should still be run in main thread only.
> >     
> >     Signed-off-by: Peter Xu <peterx@redhat.com>
> >     Message-Id: <20180309090006.10018-10-peterx@redhat.com>
> >     Reviewed-by: Eric Blake <eblake@redhat.com>
> >     [eblake: squash in Peter's followup patch to avoid test failures]
> >     Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > Peter, do you remember why you went for a bottom half?
> > 
> > Hmm, back then it was in monitor_init(), which was called from several
> > places.  Did we manage to lose the need for a bottom half along the way?
> > 
> > Note that the initial comment was a bit different:
> > 
> >         if (mon->use_io_thr) {
> >             /*
> >              * Make sure the old iowatch is gone.  It's possible when
> >              * e.g. the chardev is in client mode, with wait=on.
> >              */
> >             remove_fd_in_watch(chr);
> >             /*
> >              * We can't call qemu_chr_fe_set_handlers() directly here
> >              * since during the procedure the chardev will be active
> >              * and running in monitor iothread, while we'll still do
> >              * something before returning from it, which is a possible
> >              * race too.  To avoid that, we just create a BH to setup
> >              * the handlers.
> >              */
> >             aio_bh_schedule_oneshot(monitor_get_aio_context(),
> >                                     monitor_qmp_setup_handlers_bh, mon);
> >             /* We'll add this to mon_list in the BH when setup done */
> >             return;
> >         } else {
> >             qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> >                                      monitor_qmp_read, monitor_qmp_event,
> >                                      NULL, mon, NULL, true);
> >         }
> > 
> > I changed it in commit 774a6b67a40.
> 
> I think the original problem was that if qemu_chr_fe_set_handlers() is called
> in main thread, it can start to race somehow within execution of the function
> qemu_chr_fe_set_handlers() right after we switch context at:
> 
>     qemu_chr_be_update_read_handlers(s, context);
> 
> Then the rest code in qemu_chr_fe_set_handlers() will continue to run in main
> thread for sure, but the should be running with the new iothread context, which
> introduce a race condition.
> 
> Running qemu_chr_be_update_read_handlers() in BH resolves that because then all
> things run in the monitor iothread only and natually serialized.

The first message in this thread, however, claims that it is *not*
in fact serialized, when using the BH. 

> So the new comment looks indeed not fully right, as the chr device should be
> indeed within main thread context before qemu_chr_fe_set_handlers(), it's just
> that the race may start right away if without BH when context switch happens
> for the chr.

It sounds like both the comment and the code are potentially wrong.

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 :|