A bug of Monitor Chardev ?

A bug of Monitor Chardev ?

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

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!

Re: A bug of Monitor Chardev ?

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

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?


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2920 bytes --]

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

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.

I think we should explicitly document that monitor_init_qmp
is *required* to be invoked from the main thread and then
remove the bottom half usage.  If we ever find a need to
create a new monitor from a non-main thread, that thread
could use an idle callback attached to the default GMainContext
to invoke monitor_init_qmp.

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: A bug of Monitor Chardev ?

[Markus Armbruster]

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 we should explicitly document that monitor_init_qmp
> is *required* to be invoked from the main thread and then
> remove the bottom half usage.

Assert "running in main thread", so screwups crash reliably instead of
creating a race.

>                                If we ever find a need to
> create a new monitor from a non-main thread, that thread
> could use an idle callback attached to the default GMainContext
> to invoke monitor_init_qmp.
>
> Regards,
> Daniel

Re: A bug of Monitor Chardev ?

[Peter Xu]

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.

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.

Thanks,

> 
> > I think we should explicitly document that monitor_init_qmp
> > is *required* to be invoked from the main thread and then
> > remove the bottom half usage.
> 
> Assert "running in main thread", so screwups crash reliably instead of
> creating a race.
> 
> >                                If we ever find a need to
> > create a new monitor from a non-main thread, that thread
> > could use an idle callback attached to the default GMainContext
> > to invoke monitor_init_qmp.
> >
> > Regards,
> > Daniel
> 

-- 
Peter Xu

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

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

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > 
> > 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.


I feel like our root cause problem that the original code was trying to
workaround, is that the chardev is "active" from the very moment it is
created, regardless of whether the frontend is ready to use it.

IIUC, in this case the socket chardev is already listen()ing and
accept()ing incoming clients off the network, before the monitor
has finished configuring its hooks into the chardev. This means
that the initial listen()/accept() I/O watches are using the
default GMainContext, and the monitor *has* to remove them and
put in new watches on the thread private GMainContext.

To eliminate any risk of races, we need to make it possible for the
monitor to configure the GMainContext on the chardevs *before* any
I/O watches are configured.

This in turn suggests that we need to split the chardev initialization
into two phases. First we have the basic chardev creation, with object
creation, option parsing/sanity checking, socket creation, and then
second we have the actual activation where the I/O watches are added.

IOW,  qemu_chr_new() is the former and gets run from generic code in
the main() method, or in QMP chardev_add.  A new 'qemu_chr_activate'
method would be called by whatever frontend is using the chardev,
after registering a custom GMainContext.

This would involve updating every single existing user of chardevs
to add a call to qemu_chr_activate, but that's worth it to eliminate
the race by design, rather than workaround it.

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: A bug of Monitor Chardev ?

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

Hi

On Fri, May 21, 2021 at 8:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > >
> > > 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.
>
>
> I feel like our root cause problem that the original code was trying to
> workaround, is that the chardev is "active" from the very moment it is
> created, regardless of whether the frontend is ready to use it.
>
> IIUC, in this case the socket chardev is already listen()ing and
> accept()ing incoming clients off the network, before the monitor
> has finished configuring its hooks into the chardev. This means
> that the initial listen()/accept() I/O watches are using the
> default GMainContext, and the monitor *has* to remove them and
> put in new watches on the thread private GMainContext.
>
> To eliminate any risk of races, we need to make it possible for the
> monitor to configure the GMainContext on the chardevs *before* any
> I/O watches are configured.
>
> This in turn suggests that we need to split the chardev initialization
> into two phases. First we have the basic chardev creation, with object
> creation, option parsing/sanity checking, socket creation, and then
> second we have the actual activation where the I/O watches are added.
>
> IOW,  qemu_chr_new() is the former and gets run from generic code in
> the main() method, or in QMP chardev_add.  A new 'qemu_chr_activate'
> method would be called by whatever frontend is using the chardev,
> after registering a custom GMainContext.
>
> This would involve updating every single existing user of chardevs
> to add a call to qemu_chr_activate, but that's worth it to eliminate
> the race by design, rather than workaround it.
>


What about my earlier suggestion to add a new
"qemu_chr_be_disable_handlers()" (until update_read_handlers is called
again to enable them and the set a different context)?


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3831 bytes --]

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

On Fri, May 21, 2021 at 08:59:17PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, May 21, 2021 at 8:56 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > > >
> > > > 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.
> >
> >
> > I feel like our root cause problem that the original code was trying to
> > workaround, is that the chardev is "active" from the very moment it is
> > created, regardless of whether the frontend is ready to use it.
> >
> > IIUC, in this case the socket chardev is already listen()ing and
> > accept()ing incoming clients off the network, before the monitor
> > has finished configuring its hooks into the chardev. This means
> > that the initial listen()/accept() I/O watches are using the
> > default GMainContext, and the monitor *has* to remove them and
> > put in new watches on the thread private GMainContext.
> >
> > To eliminate any risk of races, we need to make it possible for the
> > monitor to configure the GMainContext on the chardevs *before* any
> > I/O watches are configured.
> >
> > This in turn suggests that we need to split the chardev initialization
> > into two phases. First we have the basic chardev creation, with object
> > creation, option parsing/sanity checking, socket creation, and then
> > second we have the actual activation where the I/O watches are added.
> >
> > IOW,  qemu_chr_new() is the former and gets run from generic code in
> > the main() method, or in QMP chardev_add.  A new 'qemu_chr_activate'
> > method would be called by whatever frontend is using the chardev,
> > after registering a custom GMainContext.
> >
> > This would involve updating every single existing user of chardevs
> > to add a call to qemu_chr_activate, but that's worth it to eliminate
> > the race by design, rather than workaround it.
> >
> 
> 
> What about my earlier suggestion to add a new
> "qemu_chr_be_disable_handlers()" (until update_read_handlers is called
> again to enable them and the set a different context)?

It could probably work, but it still feels like a bit of a hack to me,
because there's still a window between the chardev create and the
qemu_chr_be_disable_handlers call where new clients arrive.

This may not be a problem in the scenario we're in with the monitor
here, because the mainloop isn't running yet IIUC, but for long term
think we're better off fixing the general problem rather than
introducing more special workarounds.

For example, I think your suggestion will still be racey if we ever
add support for hotplugging additional monitor instances, which
has been talked around recently.

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: A bug of Monitor Chardev ?

[Peter Xu]

On Fri, May 21, 2021 at 05:56:14PM +0100, Daniel P. Berrangé wrote:
> On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > > 
> > > 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.
> 
> 
> I feel like our root cause problem that the original code was trying to
> workaround, is that the chardev is "active" from the very moment it is
> created, regardless of whether the frontend is ready to use it.
> 
> IIUC, in this case the socket chardev is already listen()ing and
> accept()ing incoming clients off the network, before the monitor
> has finished configuring its hooks into the chardev. This means
> that the initial listen()/accept() I/O watches are using the
> default GMainContext, and the monitor *has* to remove them and
> put in new watches on the thread private GMainContext.
> 
> To eliminate any risk of races, we need to make it possible for the
> monitor to configure the GMainContext on the chardevs *before* any
> I/O watches are configured.
> 
> This in turn suggests that we need to split the chardev initialization
> into two phases. First we have the basic chardev creation, with object
> creation, option parsing/sanity checking, socket creation, and then
> second we have the actual activation where the I/O watches are added.

When we are still running the monitor_init() code IIUC the main thread is still
occupied so it won't be able to handle any listen()/accept() even if there's
event already, so IIUC race can only happen when main thread started the event
loop then run concurrently with the BH in the other iothread.

So, besides above split of chardev init (which sounds like a bigger
surgery)... would it also work if we let the main thread wait until that BH
executed in monitor iothread?  Then all pending listen()/accept() event will
directly be done in the monitor thread.

> 
> IOW,  qemu_chr_new() is the former and gets run from generic code in
> the main() method, or in QMP chardev_add.  A new 'qemu_chr_activate'
> method would be called by whatever frontend is using the chardev,
> after registering a custom GMainContext.
> 
> This would involve updating every single existing user of chardevs
> to add a call to qemu_chr_activate, but that's worth it to eliminate
> the race by design, rather than workaround it.
> 
> 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: A bug of Monitor Chardev ?

[Peter Xu]

On Fri, May 21, 2021 at 08:59:17PM +0400, Marc-André Lureau wrote:
> What about my earlier suggestion to add a new
> "qemu_chr_be_disable_handlers()" (until update_read_handlers is called
> again to enable them and the set a different context)?

Yes this sounds both clean and easy indeed (while the other "wait BH run"
approach might be unclean from that pov as we need to do it manually with a
semaphore or something in both the BH and the main thread..).

-- 
Peter Xu

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

On Fri, May 21, 2021 at 01:09:17PM -0400, Peter Xu wrote:
> On Fri, May 21, 2021 at 05:56:14PM +0100, Daniel P. Berrangé wrote:
> > On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > > > 
> > > > 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.
> > 
> > 
> > I feel like our root cause problem that the original code was trying to
> > workaround, is that the chardev is "active" from the very moment it is
> > created, regardless of whether the frontend is ready to use it.
> > 
> > IIUC, in this case the socket chardev is already listen()ing and
> > accept()ing incoming clients off the network, before the monitor
> > has finished configuring its hooks into the chardev. This means
> > that the initial listen()/accept() I/O watches are using the
> > default GMainContext, and the monitor *has* to remove them and
> > put in new watches on the thread private GMainContext.
> > 
> > To eliminate any risk of races, we need to make it possible for the
> > monitor to configure the GMainContext on the chardevs *before* any
> > I/O watches are configured.
> > 
> > This in turn suggests that we need to split the chardev initialization
> > into two phases. First we have the basic chardev creation, with object
> > creation, option parsing/sanity checking, socket creation, and then
> > second we have the actual activation where the I/O watches are added.
> 
> When we are still running the monitor_init() code IIUC the main thread is still
> occupied so it won't be able to handle any listen()/accept() even if there's
> event already, so IIUC race can only happen when main thread started the event
> loop then run concurrently with the BH in the other iothread.
> 
> So, besides above split of chardev init (which sounds like a bigger
> surgery)... would it also work if we let the main thread wait until that BH
> executed in monitor iothread?  Then all pending listen()/accept() event will
> directly be done in the monitor thread.

My concern is what happens when we add support for monitor hotplug
in the future. A client wanting to add a second monitor to a running
QEMU will run "chardev-add" followed by a hypothetical "monitor-add"
command. Both of these will be processed in the monitor thread, so
the main thread will have already started handling I/O events for
the chardev before "monitor-add" gets processed. Thus I think the
only long term safe solution is to have a design that guarantees
no I/O events are registered by *any* chardev impl, until the
frontend connects to the chardev.


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: A bug of Monitor Chardev ?

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

Hi Marc,

在 2021/5/22 0:59, Marc-André Lureau 写道:
> Hi
> 
> On Fri, May 21, 2021 at 8:56 PM Daniel P. Berrangé <berrange@redhat.com
> <mailto:berrange@redhat.com>> wrote:
> 
>     On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
>     > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
>     > >
>     > > 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.
> 
> 
>     I feel like our root cause problem that the original code was trying to
>     workaround, is that the chardev is "active" from the very moment it is
>     created, regardless of whether the frontend is ready to use it.
> 
>     IIUC, in this case the socket chardev is already listen()ing and
>     accept()ing incoming clients off the network, before the monitor
>     has finished configuring its hooks into the chardev. This means
>     that the initial listen()/accept() I/O watches are using the
>     default GMainContext, and the monitor *has* to remove them and
>     put in new watches on the thread private GMainContext.
> 
>     To eliminate any risk of races, we need to make it possible for the
>     monitor to configure the GMainContext on the chardevs *before* any
>     I/O watches are configured.
> 
>     This in turn suggests that we need to split the chardev initialization
>     into two phases. First we have the basic chardev creation, with object
>     creation, option parsing/sanity checking, socket creation, and then
>     second we have the actual activation where the I/O watches are added.
> 
>     IOW,  qemu_chr_new() is the former and gets run from generic code in
>     the main() method, or in QMP chardev_add.  A new 'qemu_chr_activate'
>     method would be called by whatever frontend is using the chardev,
>     after registering a custom GMainContext.
> 
>     This would involve updating every single existing user of chardevs
>     to add a call to qemu_chr_activate, but that's worth it to eliminate
>     the race by design, rather than workaround it.
> 
> 
> 
> What about my earlier suggestion to add a new "qemu_chr_be_disable_handlers()"
> (until update_read_handlers is called again to enable them and the set a
> different context)?
> 

In this case, the BH calls the update_read_handlers, so the new added
"qemu_chr_be_disable_handlers" will be called in the monitor iothread BH ? If
so, I'm not sure whether it is safe enough, because the Chardev may still be
accessed in parallel by main loop and iothread for a while.

How about call "qemu_chr_be_disable_handlers" before set the
monitor_qmp_setup_handlers_bh ?

I think Daniel's soluation is perfect, but it's beyond my ability, I'm not
expert in Chardev/QMP, it's difficult to guarantee no other bugs will be
introduced, so we prefer to take the simplest and safest way to fix the bug in
our production.

> 
> -- 
> Marc-André Lureau

Re: A bug of Monitor Chardev ?

[Markus Armbruster]

"Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
<longpeng2@huawei.com> writes:

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

Do we?  We talked, but I'm not sure we reached a conclusion.

Re: A bug of Monitor Chardev ?

[Daniel P. Berrangé]

On Tue, Jun 08, 2021 at 04:07:30PM +0200, Markus Armbruster wrote:
> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
> <longpeng2@huawei.com> writes:
> 
> > 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!
> 
> Do we?  We talked, but I'm not sure we reached a conclusion.

Seems like we ended up with two options.

  1. A workaround for the current  specific problem by rearranging
     the initilization code in the monitor a little.

  2. A design fix of splitting the chardev creation into two
     parts, one creation, and one activation.

The latter is significantly more work, but is a better long term bet IMHO.
But what we really is someone motivated to actually implement one of the
two options.

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: A bug of Monitor Chardev ?

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

在 2021/6/8 23:37, Daniel P. Berrangé 写道:
> On Tue, Jun 08, 2021 at 04:07:30PM +0200, Markus Armbruster wrote:
>> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
>> <longpeng2@huawei.com> writes:
>>
>>> 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!
>>
>> Do we?  We talked, but I'm not sure we reached a conclusion.
> 
> Seems like we ended up with two options.
> 
>   1. A workaround for the current  specific problem by rearranging
>      the initilization code in the monitor a little.
> 
>   2. A design fix of splitting the chardev creation into two
>      parts, one creation, and one activation.
> 
> The latter is significantly more work, but is a better long term bet IMHO.
> But what we really is someone motivated to actually implement one of the
> two options.
> 

How about the following implementation of option-1 ? We've tested it for several
weeks, it works fine.

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a484641..ecb3db9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -722,6 +722,19 @@ static void tcp_chr_update_read_handler(Chardev *chr)
     update_ioc_handlers(s);
 }

+static void tcp_chr_disable_handler(Chardev *chr)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->listener && s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
+        qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
+                                              NULL, chr->gcontext);
+    }
+
+    remove_fd_in_watch(chr);
+    remove_hup_source(s);
+}
+
 static bool tcp_chr_is_connected(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -1703,6 +1716,7 @@ static void char_socket_class_init(ObjectClass *oc, void
*data)
     cc->chr_add_watch = tcp_chr_add_watch;
     cc->chr_set_reconnect_time = tcp_chr_set_reconnect_time;
     cc->chr_update_read_handler = tcp_chr_update_read_handler;
+    cc->chr_disable_handler = tcp_chr_disable_handler;
     cc->chr_is_connected = tcp_chr_is_connected;
     cc->chr_get_connect_id = tcp_chr_get_connect_id;

diff --git a/chardev/char.c b/chardev/char.c
index ff0a3cf..990fe4f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -238,6 +238,15 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
     }
 }

+void qemu_chr_be_disable_handlers(Chardev *s)
+{
+    ChardevClass *cc = CHARDEV_GET_CLASS(s);
+
+    if (cc->chr_disable_handler) {
+        cc->chr_disable_handler(s);
+    }
+}
+
 int qemu_chr_add_client(Chardev *s, int fd)
 {
     return CHARDEV_GET_CLASS(s)->chr_add_client ?
diff --git a/include/chardev/char.h b/include/chardev/char.h
index d1ec628..7a8c740 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -212,6 +212,8 @@ void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
 void qemu_chr_be_update_read_handlers(Chardev *s,
                                       GMainContext *context);

+void qemu_chr_be_disable_handlers(Chardev *s);
+
 /**
  * qemu_chr_be_event:
  * @event: the event to send
@@ -282,6 +284,7 @@ typedef struct ChardevClass {
     int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
     GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
     void (*chr_update_read_handler)(Chardev *s);
+    void (*chr_disable_handler)(Chardev *s);
     int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
     int (*get_msgfds)(Chardev *s, int* fds, int num);
     int (*set_msgfds)(Chardev *s, int *fds, int num);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 9a69ae4..2c2248c 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -413,11 +413,13 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
          * 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 chardev might be running in the monitor I/O
-         * thread.  Schedule a bottom half.
+         * Before schedule a bottom half, we should clean up the handler in the
+         * default context to prevent the race between main thread and iothread
          */
+        qemu_chr_be_disable_handlers(chr);
+
         aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
                                 monitor_qmp_setup_handlers_bh, mon);
         /* The bottom half will add @mon to @mon_list */
-- 
1.8.3.1


> Regards,
> Daniel
> 

Re: A bug of Monitor Chardev ?

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 6667 bytes --]

Hi

On Wed, Jun 9, 2021 at 4:21 AM Longpeng (Mike, Cloud Infrastructure Service
Product Dept.) <longpeng2@huawei.com> wrote:

>
>
> 在 2021/6/8 23:37, Daniel P. Berrangé 写道:
> > On Tue, Jun 08, 2021 at 04:07:30PM +0200, Markus Armbruster wrote:
> >> "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
> >> <longpeng2@huawei.com> writes:
> >>
> >>> 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!
> >>
> >> Do we?  We talked, but I'm not sure we reached a conclusion.
> >
> > Seems like we ended up with two options.
> >
> >   1. A workaround for the current  specific problem by rearranging
> >      the initilization code in the monitor a little.
> >
> >   2. A design fix of splitting the chardev creation into two
> >      parts, one creation, and one activation.
> >
> > The latter is significantly more work, but is a better long term bet
> IMHO.
> > But what we really is someone motivated to actually implement one of the
> > two options.
> >
>
> How about the following implementation of option-1 ? We've tested it for
> several
> weeks, it works fine.
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>

If we go with this option, disable_handler will need to be implemented for
all chardevs that have some (or use a QEMU_CHAR_FEATURE_DISABLE_HANDLER
flag, which will limit the chardevs that can be associated with a monitor
and knowingly break previously working setups).


index a484641..ecb3db9 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -722,6 +722,19 @@ static void tcp_chr_update_read_handler(Chardev *chr)
>      update_ioc_handlers(s);
>  }
>
> +static void tcp_chr_disable_handler(Chardev *chr)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +
> +    if (s->listener && s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
> +        qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
> +                                              NULL, chr->gcontext);
> +    }
> +
> +    remove_fd_in_watch(chr);
> +    remove_hup_source(s);
> +}
> +
>  static bool tcp_chr_is_connected(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -1703,6 +1716,7 @@ static void char_socket_class_init(ObjectClass *oc,
> void
> *data)
>      cc->chr_add_watch = tcp_chr_add_watch;
>      cc->chr_set_reconnect_time = tcp_chr_set_reconnect_time;
>      cc->chr_update_read_handler = tcp_chr_update_read_handler;
> +    cc->chr_disable_handler = tcp_chr_disable_handler;
>      cc->chr_is_connected = tcp_chr_is_connected;
>      cc->chr_get_connect_id = tcp_chr_get_connect_id;
>
> diff --git a/chardev/char.c b/chardev/char.c
> index ff0a3cf..990fe4f 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -238,6 +238,15 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
>      }
>  }
>
> +void qemu_chr_be_disable_handlers(Chardev *s)
> +{
> +    ChardevClass *cc = CHARDEV_GET_CLASS(s);
> +
> +    if (cc->chr_disable_handler) {
> +        cc->chr_disable_handler(s);
> +    }
> +}
> +
>  int qemu_chr_add_client(Chardev *s, int fd)
>  {
>      return CHARDEV_GET_CLASS(s)->chr_add_client ?
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index d1ec628..7a8c740 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -212,6 +212,8 @@ void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf,
> int len);
>  void qemu_chr_be_update_read_handlers(Chardev *s,
>                                        GMainContext *context);
>
> +void qemu_chr_be_disable_handlers(Chardev *s);
> +
>  /**
>   * qemu_chr_be_event:
>   * @event: the event to send
> @@ -282,6 +284,7 @@ typedef struct ChardevClass {
>      int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
>      GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
>      void (*chr_update_read_handler)(Chardev *s);
> +    void (*chr_disable_handler)(Chardev *s);
>      int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
>      int (*get_msgfds)(Chardev *s, int* fds, int num);
>      int (*set_msgfds)(Chardev *s, int *fds, int num);
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 9a69ae4..2c2248c 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -413,11 +413,13 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
>           * 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 chardev might be running in the monitor I/O
> -         * thread.  Schedule a bottom half.
> +         * Before schedule a bottom half, we should clean up the handler
> in the
> +         * default context to prevent the race between main thread and
> iothread
>           */
> +        qemu_chr_be_disable_handlers(chr);
> +
>          aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
>                                  monitor_qmp_setup_handlers_bh, mon);
>          /* The bottom half will add @mon to @mon_list */
> --
> 1.8.3.1
>
>
> > Regards,
> > Daniel
> >
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8546 bytes --]