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;
_______________________________________________
vbox-dev mailing list
vbox-dev@virtualbox.org
https://www.virtualbox.org/mailman/listinfo/vbox-dev
