[PATCH 4/6] ntoskrnl.exe: Send IRP_MN_SURPRISE_REMOVAL before removing children.

[Rémi Bernon]

On 6/18/21 11:06 PM, Zebediah Figura (she/her) wrote:
> On 6/18/21 1:49 PM, Rémi Bernon wrote:
>> On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
>>> On 6/18/21 7:06 AM, Rémi Bernon wrote:
>>>> So that mini driver gets notified first and has a chance to cancel
>>>> pending IRPs.
>>>
>>> Which minidriver?
>>>
>>> Isn't it the responsibility of the child to terminate all pending IRPs
>>> (and disallow further ones) on removal? See also [0], [1], [2], which
>>> say that queued requests should be completed in (both, for some reason)
>>> IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
>>>
>>> I think this deserves tests.
>>>
>>> [0]
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-device-in-a-function-driver 
>>>
>>>
>>>
>>> [1]
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an-irp-mn-surprise-removal-request 
>>>
>>>
>>>
>>> [2]
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-device-in-a-bus-drive 
>>>
>>>
>>>
>>
>> I really don't have the big picture yet, so I meant the one implemented
>> in driver_hid.c. If it doesn't cancel the IRP it has queued on device
>> removal, the test never completes the device removal on Windows.
>>
>> On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we
>> don't call it before removing the "children" in ntoskrnl.exe. I don't
>> really understand who are the children there, but driver_hid apparently
>> isn't.
>>
> 
> Allow me to explain in detail. Apologies if any of this is old news to 
> you. If nothing else I'll adapt it to a wiki page later ;-)
> 
> 
> TLC PDO
>                      winehid.sys + hidclass.sys
> HID FDO
> -------------------------------------------------
> HID PDO
>                      winebus.sys
> root FDO
> -------------------------------------------------
> root PDO
>                      ntoskrnl.exe
> [PNP manager]
> 
> 
> In the case of the HID test driver it's a little different:
> 
> 
> TLC PDO
>                      driver_hid.dll + hidclass.sys
> root/HID FDO
> -------------------------------------------------
> root/HID PDO
>                      ntoskrnl.exe
> [PNP manager]
> 
> 
> A root PNP device is the one passed to AddDevice for a root PNP driver. 
> Usually it's intended to enumerate children; here we kind of cheat and 
> make the root PnP device the same device as the HID device. It's a bit 
> weird but it works.
> 
> The TLC (top-level collection) PDO is managed entirely within 
> hidclass.sys; the minidriver never interacts with it. In theory there's 
> supposed to be one or more of these; we don't handle the "or more" case 
> yet.
> 
> The application only ever interacts with the TLC PDO. It's actually 
> impossible to open the HID device stack (IIRC it fails with 
> STATUS_UNSUCCESSFUL). In response to read/write/hidd ioctls, hidclass 
> creates new irps and sends them to the HID device stack, i.e. the "HID 
> FDO/HID PDO" pair in the above diagrams.
> 
> [Note that IRPs are always sent to the top of a stack first, so in this 
> case the minidriver has a chance to handle it. In a real HID driver the 
> minidriver would always handle it itself, rather than sending it down to 
> the PDO. You can mentally replace winehid with hidusb, and winebus with 
> (say) usbccgp. Obviously usbccgp doesn't know anything about HID. Our 
> architecture is a little weird, though, for various reasons.]
> 
> There is usually no TLC FDO, but in theory there could be, and in 
> practice I believe mouse and keyboard drivers attach to the TLC PDO.
> 
> On reflection, I think I may have confused myself, so this patch 
> actually makes more sense than it initially did. It'd still be nice to 
> have tests, though, and the commit message should ideally be more specific.
> 

Thanks, this is definitely not old news, although it's still quite blurry.

 From [1] I understand that the hid minidriver lives at the HID FDO 
level, which means that (minidriver, hidclass.sys) act as the bus FDO 
driver here.

Reading [2] or [3] doesn't make it completely explicit if the children 
should receive the removal IRP first, but from what I understand it is 
how it's generally supposed to work.

With that understanding I now believe it's correct to call the IRP on 
the children of the device first.

The problem I'm facing here comes from the way hidclass.sys PDO handles 
its removal. It waits for the reading thread to complete, which may be 
waiting on IRPs queued in the HID FDO.

We could either:

1) call the surprise removal first on the device stack, then call the 
normal removal on the stack, giving a chance for the HID FDO to cancel 
its queued IRPs.

or,

2) combine the waits between halt_event and the pending IRP in the HID 
PDO device thread, so that the thread can terminate even with a pending IRP.


Unrelated, but I'm not sure to understand why the HID PDO has to 
re-create IRPs every time it needs to call the minidriver (except for 
the device thread of course), wouldn't it be possible to just use the 
normal IRP propagation down the stack?


[1] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/hid-architecture

[2] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an-irp-mn-surprise-removal-request

[3] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-device-in-a-bus-driver
-- 
Rémi Bernon <rbernon at codeweavers.com>