On Thu, Jul 19, 2018 at 11:05:34AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Thu, Jul 19, 2018 at 09:20:34AM +0200, Markus Armbruster wrote:
> >> Peter Xu <pet...@redhat.com> writes:
> >>
> >> > On Wed, Jul 18, 2018 at 05:38:11PM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <pet...@redhat.com> writes:
> >> >>
> >> >> > After the Out-Of-Band work, the monitor iothread may be accessing the
> >> >> > cur_mon as well (via monitor_qmp_dispatch_one()).
>
> Since renamed to monitor_qmp_dispatch().
True.
>
> Further down, we concluded that cur_mon isn't actually used from the I/O
> thread, didn't we?
I think so; if not we should either fix it or apply this patch. :)
>
> >> >> > Let's convert the
> >> >> > cur_mon variable to be a per-thread variable to make sure there won't
> >> >> > be
> >> >> > a race between threads when accessing the variable.
> >> >>
> >> >> Hmm... why hasn't the OOB work created such a race already?
> >> >>
> >> >> A monitor reads, parses, dispatches and executes commands, formats and
> >> >> sends replies.
> >> >>
> >> >> Before OOB, all of that ran in the main thread. Any access of cur_mon
> >> >> should therefore be from the main thread. No races.
> >> >>
> >> >> OOB moves read, parse, format and send to an I/O thread. Dispatch and
> >> >> execute remain in the main thread. *Except* for commands executed OOB,
> >> >> dispatch and execute move to the I/O thread, too.
> >> >>
> >> >> Why is this not racy? I guess it relies on careful non-use of cur_mon
> >> >> in any part that may now execute in the I/O thread. Scary...
> >> >
> >> > I think it's because cur_mon is not really used in out-of-band command
> >> > executions - now we only have a few out-of-band enabled commands, and
> >> > IIUC none of them is using cur_mon (for example, in
> >> > qmp_migrate_recover() we don't even call error_report, and the code
> >> > path is quite straight forward to make sure of that). So IIUC cur_mon
> >> > variable is still only touched by main thread for now hence we should
> >> > be safe. However that condition might change in the future when we
> >> > add more out-of-band capable commands.
> >> >
> >> > (not to mention that I don't even know whether there are real users of
> >> > out-of-band if we haven't yet started to support that for libvirt...)
> >>
> >> It's not just the actual OOB commands (there are just two), it's also
> >> the monitor code to read, parse, format and send.
> >
> > My understanding is that read, parse, format, send will not touch
> > cur_mon (it was touched before but some patches in the out-of-band
> > series should have removed the last users when parsing). So IIUC only
> > the dispatcher would touch that now. I didn't consider the callers
> > like net_init_socket() and I'm only considering the monitor code (and
> > those callers should be only in the main thread too after all).
>
> There *is* cur_mon use outside dispatch & execute, e.g.
>
> void error_vprintf(const char *fmt, va_list ap)
> {
> if (cur_mon && !monitor_cur_is_qmp()) {
> monitor_vprintf(cur_mon, fmt, ap);
> } else {
> vfprintf(stderr, fmt, ap);
> }
> }
>
> Obviously unsafe to use outside the main thread. Consider:
>
> bool monitor_cur_is_qmp(void)
> {
> return cur_mon && monitor_is_qmp(cur_mon);
> }
>
> static inline bool monitor_is_qmp(const Monitor *mon)
> {
> return (mon->flags & MONITOR_USE_CONTROL);
> }
>
> If monitor_cur_is_qmp() reads cur_mon twice (which it is entitled to
> do), this crashes when the main thread sets cur_mon back to null in
> between.
Yes, but I thought we should not even use these error_vprintf() or
sister functions outside the QMP handlers, or at least that's what I
thought. For example, in parsers, we should always use error_setg()
or something similar but never error_report().
>
> Did the OOB work make things any worse? Let's see.
>
> @cur_mon is null unless the main thread is running monitor code, either
> HMP within monitor_read():
>
> cur_mon = opaque;
>
> if (cur_mon->rs) {
> for (i = 0; i < size; i++)
> readline_handle_byte(cur_mon->rs, buf[i]);
> } else {
> if (size == 0 || buf[size - 1] != 0)
> monitor_printf(cur_mon, "corrupted command\n");
> else
> handle_hmp_command(cur_mon, (char *)buf);
> }
>
> cur_mon = old_mon;
>
> or QMP within monitor_qmp_dispatch():
>
> old_mon = cur_mon;
> cur_mon = mon;
>
> rsp = qmp_dispatch(mon->qmp.commands, req, qmp_oob_enabled(mon));
>
> cur_mon = old_mon;
>
> In both cases, old_mon is always null.
>
> Fine print: before commit 227a07552f3 "monitor: move the cur_mon hack
> deeper for QMP", we ran more code for QMP with cur_mon set, namely the
> JSON parser, but that doesn't matter here.
>
> More fine print: there's also qmp_human_monitor_command(), which stacks
> an HMP monitor on top of the QMP monitor. Also doesn't matter here.
>
> The OOB work doesn't add any new races as long as
>
> * it doesn't add assignments to @cur_mon, and
>
> * none of the code it moves out of the main thread accesses @cur_mon.
>
> The first condition obviously holds. The second one isn't obvious, but
> I figure it holds, too.
>
> Okay, I think I've convince myself the OOB work didn't add
> cur_mon-related races.
Hopefully, yes. Thanks for the double check.
>
> >> >> Should this go into 3.0 to reduce the risk of bugs?
> >> >
> >> > Yes I think it would be good to have that even for 3.0, since it still
> >> > can be seen as a bug fix of existing code.
> >>
> >> Agreed.
> >>
> >> > Regards,
> >> >
> >> >> > Note that thread variables are not initialized to a valid value when
> >> >> > new
> >> >> > thread is created.
> >>
> >> Confusing. It sounds like @cur_mon's initial value would be
> >> indeterminate, like an automatic variable's. Not true. Variables with
> >> thread storage duration are initialized when the thread is created.
> >> Since @cur_mon's declaration lacks an initializer, it'll be initialized
> >> to a null pointer. Your sentence is correct when you consider that null
> >> pointer not a valid value.
> >
> > Yes that's what I meant. So how about this?
> >
> > Note that the per-thread @cur_mon variable is not initialized to
> > point to a valid Monitor struct when a new thread is created (the
> > default value will be NULL).
> >
> > Please feel free to tune it up.
>
> I think what the patch really changes is the value of @cur_mon outside
> the main thread: it remains null there now. Before, it depended on what
> the main thread was doing, and therefore could not be used safely.
>
> In other words, the patch makes uses of @cur_mon like the one in
> error_vprintf() shown above safe.
>
> I think that's what we should explain in the commit message. I can try
> rewriting it,
I'll appreciate that if so.
> but right now I got to run.
Must be lunch time! :)
Regards,
>
> >>
> >> >> > However for our case we don't need to set it up,
> >> >> > since the cur_mon variable is only used in such a pattern:
> >> >> >
> >> >> > old_mon = cur_mon;
> >> >> > cur_mon = xxx;
> >> >> > (do something, read cur_mon if necessary in the stack)
> >
> > [1]
> >
> >> >> > cur_mon = old_mon;
> >> >> >
> >> >> > It plays a role as stack variable, so no need to be initialized at
> >> >> > all.
> >> >> > We only need to make sure the variable won't be changed unexpectedly
> >> >> > by
> >> >> > other threads.
> >>
> >> Do we need this paragraph? The commit doesn't mess with @cur_mon's
> >> initial value at all...
> >
> > I was trying to explain why we don't need to initialize that variable
> > for each thread. A common idea (at least that's what I have had in
> > mind) is that when we create a new thread we should possibly inherit
> > that @cur_mon variable in a copy-on-write fashion for that new thread.
> > But that's not really necessary for the use case like above (as long
> > as we don't create thread during [1], and that's what we do).
> >
> > If you think the patch explains itself better without these lines,
> > please feel free to drop it.
> >
> >>
> >> >> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> >> >> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> >> >> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> >> >> > [peterx: touch up commit message a bit]
> >> >> > Signed-off-by: Peter Xu <pet...@redhat.com>
> >
> > Thanks,
--
Peter Xu
