[vbox-dev] vgdrvIoCtl_CancelAllWaitEvents inconsistent behavior ?

[Michael Thayer]

Hello Hans,

04.07.2017 08:55, Hans de Goede wrote:
[Discussion of inconsistent wait event cancelling code in VBoxGuest.]
> 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).

I think you are reading too much intelligent design into the interface.
I was the guilty person who created it, long ago, and I don't think I
thought it through sufficiently well at the time.  That said, I don't
think I wrote the current implementation code.  I looked at the
user-space code I mentioned again, and actually realised that it does
handle the old (and the new) code.  Testing seems to confirm this.

> #####################################################################
> 
> 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 :)
[...]

I really don't think you need to mimic this.  Since
CANCEL_ALL_WAITEVENTS is currently only used for cleanly stopping
processes, it is not likely to get noticed either way.  That said, it
might be worth us fixing it in case this ever changes.

Regards
Michael
-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher