[vbox-dev] vgdrvIoCtl_CancelAllWaitEvents inconsistent behavior ?

[Hans de Goede]

Hi Michael,

Thank you for looking into this.

On 04-07-17 08:14, Michael Thayer wrote:
> Hello Hans,
> 
> 03.07.2017 20:49, Michael Thayer wrote:
> [Discussion of incorrect looking wait code in VBoxGuest.cpp.]>
>> I did not get any response when I asked around about this.  Reading the
>> code (quite a bit of work, as you presumably discovered too; fortunately
>> the user-space part is a bit easier) suggests that this is not intended,
>> as I could not find a caller in trunk or versions 5.1, 5.0 or 4.3 which
>> depended on this behaviour, and one of the few callers that I did find
>> (src/VBox/Additions/common/VBoxService/VBoxServiceCpuHotPlug.cpp) looks
>> like it would handle it incorrectly.  I will correct this in our code
>> too unless someone here speaks up.
> 
> I am testing out the patch below.  I can only see one caller site where
> this path might trigger in a very unlikely race.  I will put a sleep and
> a LogRel in there to compare the results with and without the change.

So some remarks about this, I was thinking that *maybe* the old
behavior is on purpose, imagine the following:

1) cancel, no waiters, pSession->fPendingCancelWaitEvents get set
2) the thread for which the cancel was intended stops, without ever
    calling into vgdrvIoCtl_WaitEvent again and so does not clear
    the fPendingCancelWaitEvents flag
3) (much) later a new thread in the same session starts waiting

old behavior: new thread sees an empty event, goes back to
waiting

new behavior: new thread sees a VERR_INTERRUPTED -> and stops
? (note I've not looked at the userspace code).

#####################################################################

One other thing worth noticing is that the current wait/cancel
code causes events to get lost at cancel, imagine the following
sequence:

1) Some events get dispatched, moved from pDevExt->f32PendingEvents to pWait->fResEvents
2) vgdrvIoCtl_CancelAllWaitEvents runs replacing pWait->fResEvents with UINT32_MAX
3) vgdrvIoCtl_WaitEvent consumes pWait->fResEvents, sees UINT32_MAX, treats it as
    a cancel.

Note this is something which I plan to fix in my rewrite of this code
to directly use linux wait_queues (which will make the code much much smaller).

Actually mimicking this bug will be hard to do :)

Regards,

Hans




> Index: src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp
> ===================================================================
> --- src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp	(revision 116649)
> +++ src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp	(working copy)
> @@ -1584,7 +1584,7 @@
>       uint32_t fMatches = pDevExt->f32PendingEvents & fReqEvents;
>       if (fMatches & VBOXGUEST_ACQUIRE_STYLE_EVENTS)
>           fMatches &= vgdrvGetAllowedEventMaskForSession(pDevExt, pSession);
> -    if (fMatches || pSession->fPendingCancelWaitEvents)
> +    if (fMatches)
>       {
>           ASMAtomicAndU32(&pDevExt->f32PendingEvents, ~fMatches);
>           RTSpinlockRelease(pDevExt->EventSpinlock);
> @@ -1598,6 +1598,13 @@
>           pSession->fPendingCancelWaitEvents = false;
>           return VINF_SUCCESS;
>       }
> +    else if (pSession->fPendingCancelWaitEvents)
> +    {
> +        RTSpinlockRelease(pDevExt->EventSpinlock);
> +        pInfo->u32Result = VBOXGUEST_WAITEVENT_INTERRUPTED;
> +        pSession->fPendingCancelWaitEvents = false;
> +        return VINF_INTERRUPTED;
> +    }
> 
>       RTSpinlockRelease(pDevExt->EventSpinlock);
>       return VERR_TIMEOUT;
> @@ -1633,7 +1640,7 @@
>        */
>       RTSpinlockAcquire(pDevExt->EventSpinlock);
>       rc = vbdgCheckWaitEventCondition(pDevExt, pSession, pInfo, iEvent,
> fReqEvents);
> -    if (rc == VINF_SUCCESS)
> +    if (rc == VINF_SUCCESS || rc == VERR_INTERRUPTED)
>           return rc;
> 
>       if (!pInfo->u32TimeoutIn)
> @@ -1656,7 +1663,7 @@
>       RTSpinlockAcquire(pDevExt->EventSpinlock);
>       RTListAppend(&pDevExt->WaitList, &pWait->ListNode);
>       rc = vbdgCheckWaitEventCondition(pDevExt, pSession, pInfo, iEvent,
> fReqEvents);
> -    if (rc == VINF_SUCCESS)
> +    if (rc == VINF_SUCCESS || rc == VERR_INTERRUPTED)
>       {
>           vgdrvWaitFreeUnlocked(pDevExt, pWait);
>           return rc;
>