Re: [Qemu-devel] [PATCH RFC] monitor: temporary fix for dead-lock on event recursion

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Jul 31, 2018 at 9:05 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> With a Spice port chardev, it is possible to reenter
>>> monitor_qapi_event_queue() (when the client disconnects for
>>> example). This will dead-lock on monitor_lock.
>>>
>>> Instead, use some TLS variables to check for recursion and queue the
>>> events.
>> [...]
>>>
>>> Note that error report is now moved to the first caller, which may
>>> receive an error for a recursed event. This is probably fine (95% of
>>> callers use &error_abort, the rest have NULL error and ignore it)
>>
>> I'm not 100% sure I understand this paragraph, but it might be moot; see
>> [*] below.
>>
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  monitor.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 50 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index d8d8211ae4..d580c5a79c 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -633,7 +633,7 @@ static void monitor_qapi_event_handler(void *opaque);
>>>   * applying any rate limiting if required.
>>>   */
>>>  static void
>>> -monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>>> +monitor_qapi_event_queue_no_recurse(QAPIEvent event, QDict *qdict, Error **errp)
>>>  {
>>>      MonitorQAPIEventConf *evconf;
>>>      MonitorQAPIEventState *evstate;
>>> @@ -688,6 +688,55 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>>>      qemu_mutex_unlock(&monitor_lock);
>>>  }
>>>
>>> +static void
>>> +monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    /*
>>> +     * If the function recurse, monitor_lock will dead-lock.
>>> +     * Instead, queue pending events in TLS.
>>
>> recurses
>>
>> The claim is correct before the patch.  But the comment needs to explain
>> current code.  Perhaps:
>>
>>         * monitor_qapi_event_queue_no_recurse() is not reentrant: it
>>         * would deadlock on monitor_lock.  Work around by queueing
>>         * events in thread-local storage.
>
> ok
>
>>
>>> +     * TODO: remove this, make it re-enter safe.
>>> +     */
>>> +    static __thread bool recurse;
>>> +    typedef struct MonitorQapiEvent {
>>> +        QAPIEvent event;
>>> +        QDict *qdict;
>>> +        QSIMPLEQ_ENTRY(MonitorQapiEvent) entry;
>>> +    } MonitorQapiEvent;
>>> +    MonitorQapiEvent *ev;
>>> +    static __thread QSIMPLEQ_HEAD(, MonitorQapiEvent) event_queue;
>>
>> Let's put the static variables first.
>
> ok
>
>>> +
>>> +    if (!recurse) {
>>> +        QSIMPLEQ_INIT(&event_queue);
>>> +    }
>>> +
>>> +    ev = g_new(MonitorQapiEvent, 1);
>>> +    ev->qdict = qobject_ref(qdict);
>>> +    ev->event = event;
>>> +    QSIMPLEQ_INSERT_TAIL(&event_queue, ev, entry);
>>> +    if (recurse) {
>>> +        return;
>>> +    }
>>> +
>>> +    recurse = true;
>>> +
>>> +    while ((ev = QSIMPLEQ_FIRST(&event_queue)) != NULL) {
>>> +        QSIMPLEQ_REMOVE_HEAD(&event_queue, entry);
>>
>> Could we use QSIMPLEQ_FOREACH_SAFE()?
>
> I don't think so, the next variable could be NULL, while a recursive
> call could be adding an element.
>
>>
>>> +        if (!local_err) {
>>> +            monitor_qapi_event_queue_no_recurse(ev->event, ev->qdict,
>>> +                                                &local_err);
>>> +        }
>>
>> [*] This looks scary: we silently throw away events after event queuing
>> fails.
>>
>> Fortunately, monitor_qapi_event_queue_no_recurse() cannot fail.  It
>> takes an Error ** parameter only so you can put it into @qmp_emit.
>>
>
> right
>
>> Aside: I wish we'd get rid of the indirection through @qmp_emit.
>>
>
> for later: You would hard code a function name instead?

Yes.

@qmp_emit is always monitor_qapi_event_queue in qemu-system-FOO, and
event_test_emit in test-qmp-event.  That's a linker job, not an indirect
call job.

>> Let's pass &error_abort and drop @local_err.
>
> actually, we can just ignore errp, right?

Not sure what you mean, but you've since sent v2, so let me figure it
out there :)

> for later: What about dropping error from the emit function?

Fine with me.  Easy enough to bring back if we find a need.

>
>>
>>> +        qobject_unref(ev->qdict);
>>> +        g_free(ev);
>>> +    }
>>> +
>>> +    recurse = false;
>>
>> Aha: @recurse means we've reentered the function.
>>
>> "Recurse" is imperative mood.  Misleading, as it's not an order to
>> recurse.
>>
>> Rename to @recursed?  @reentered?
>>
>
> ok, let's rename it "reentered"
>
>>> +
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>> +}
>>> +
>>>  /*
>>>   * This function runs evconf->rate ns after sending a throttled
>>>   * event.
>>
>> monitor_lock clearly needs a rethink.  Your TODO comment suggests you
>> agree.  But that's something for 3.1.
>>
>> I hate messing with locks at -rc3, but I also hate shipping known
>> deadlocks.  Your patch isn't pretty, but probably as simple as we can
>> make it for 3.0.  A few more review eyeballs would be nice.
>>
>
> thanks, I'll send v2