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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/31/2018 10:01 AM, Marc-André Lureau wrote:
>> 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.
>>
>
>>
>> diff --git a/monitor.c b/monitor.c
>> index d8d8211ae4..4d9c1873bc 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)
>>   {
>>       MonitorQAPIEventConf *evconf;
>>       MonitorQAPIEventState *evstate;
>> @@ -688,6 +688,48 @@ 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)
>> +{
>> +    /*
>> +     * monitor_qapi_event_queue_no_recurse() is not reentrant: it
>> +     * would deadlock on monitor_lock.  Work around by queueing
>> +     * events in thread-local storage.
>> +     * TODO: remove this, make it re-enter safe.
>> +     */
>> +    static __thread bool reentered;
>> +    typedef struct MonitorQapiEvent {
>> +        QAPIEvent event;
>> +        QDict *qdict;
>> +        QSIMPLEQ_ENTRY(MonitorQapiEvent) entry;
>> +    } MonitorQapiEvent;
>> +    MonitorQapiEvent *ev;
>> +    static __thread QSIMPLEQ_HEAD(, MonitorQapiEvent) event_queue;
>> +
>> +    if (!reentered) {
>> +        QSIMPLEQ_INIT(&event_queue);
>> +    }
>
> Is it safe to call QSIMPLEQ_INIT() on an already-initialized but empty
> queue?

Yes.

    #define QSIMPLEQ_HEAD(name, type)                                       \
    struct name {                                                           \
        struct type *sqh_first;    /* first element */                      \
        struct type **sqh_last;    /* addr of last next element */          \
    }

    #define QSIMPLEQ_INIT(head) do {                                        \
        (head)->sqh_first = NULL;                                           \
        (head)->sqh_last = &(head)->sqh_first;                              \
    } while (/*CONSTCOND*/0)


Removing the last member results in the same state as QSIMPLEQ_INIT():

    #define QSIMPLEQ_REMOVE_HEAD(head, field) do {                          \
        if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
            (head)->sqh_last = &(head)->sqh_first;                          \
    } while (/*CONSTCOND*/0)

Calling QSIMPLEQ_INIT() again then is a no-op.

>
>> +
>> +    ev = g_new(MonitorQapiEvent, 1);
>> +    ev->qdict = qobject_ref(qdict);
>> +    ev->event = event;
>> +    QSIMPLEQ_INSERT_TAIL(&event_queue, ev, entry);
>> +    if (reentered) {
>> +        return;
>> +    }
>> +
>> +    reentered = true;
>> +
>> +    while ((ev = QSIMPLEQ_FIRST(&event_queue)) != NULL) {
>> +        QSIMPLEQ_REMOVE_HEAD(&event_queue, entry);
>> +        monitor_qapi_event_queue_no_recurse(ev->event, ev->qdict);
>> +        qobject_unref(ev->qdict);
>> +        g_free(ev);
>> +    }
>> +
>> +    reentered = false;
>> +}
>> +
>>   /*
>>    * This function runs evconf->rate ns after sending a throttled
>>    * event.
>>