[PATCH 3/5] ntoskrnl.exe/tests: Add some pending / remove tests.

[Zebediah Figura (she/her)]

On 6/28/21 12:35 PM, Rémi Bernon wrote:
> On 6/28/21 6:15 PM, Zebediah Figura (she/her) wrote:
>> On 6/28/21 5:22 AM, Rémi Bernon wrote:
>>> This shows that removing a device should send IRP_MN_SURPRISE_REMOVAL
>>> only, and that it's still possible to use DeviceIoControl on already
>>> opened handles.
>>>
>>> IRP_MN_REMOVE_DEVICE should only be sent when all then opened handles
>>> are closed.
>>>
>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>> ---
>>>    dlls/ntoskrnl.exe/tests/driver.h     |  2 +
>>>    dlls/ntoskrnl.exe/tests/driver_pnp.c | 81 ++++++++++++++++++++++++++--
>>>    dlls/ntoskrnl.exe/tests/ntoskrnl.c   | 30 ++++++++++-
>>>    3 files changed, 107 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/dlls/ntoskrnl.exe/tests/driver.h
>>> b/dlls/ntoskrnl.exe/tests/driver.h
>>> index 2c62baa0a61..455695ad36b 100644
>>> --- a/dlls/ntoskrnl.exe/tests/driver.h
>>> +++ b/dlls/ntoskrnl.exe/tests/driver.h
>>> @@ -35,6 +35,8 @@
>>>    #define IOCTL_WINETEST_RETURN_STATUS
>>> CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_BUFFERED, FILE_ANY_ACCESS)
>>>    #define IOCTL_WINETEST_MISMATCHED_STATUS
>>> CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80b, METHOD_NEITHER, FILE_ANY_ACCESS)
>>>    #define IOCTL_WINETEST_COMPLETION
>>> CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS)
>>> +#define IOCTL_WINETEST_MARK_PENDING     CTL_CODE(FILE_DEVICE_UNKNOWN,
>>> 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS)
>>> +#define IOCTL_WINETEST_CHECK_REMOVED    CTL_CODE(FILE_DEVICE_UNKNOWN,
>>> 0x80e, METHOD_NEITHER, FILE_ANY_ACCESS)
>>>    #define IOCTL_WINETEST_BUS_MAIN
>>> CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED,
>>> FILE_ANY_ACCESS)
>>>    #define IOCTL_WINETEST_BUS_REGISTER_IFACE
>>> CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED,
>>> FILE_ANY_ACCESS)
>>> diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c
>>> b/dlls/ntoskrnl.exe/tests/driver_pnp.c
>>> index 774c98bae89..58dd32039cf 100644
>>> --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c
>>> +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c
>>> @@ -41,6 +41,11 @@ static UNICODE_STRING control_symlink, bus_symlink;
>>>    static DRIVER_OBJECT *driver_obj;
>>>    static DEVICE_OBJECT *bus_fdo, *bus_pdo;
>>> +static DWORD remove_device_count;
>>> +static DWORD surprise_removal_count;
>>> +static DWORD query_remove_device_count;
>>> +static DWORD cancel_remove_device_count;
>>> +
>>>    struct device
>>>    {
>>>        struct list entry;
>>> @@ -51,6 +56,36 @@ struct device
>>>        DEVICE_POWER_STATE power_state;
>>>    };
>>> +static IRP *pop_pending_irp(DEVICE_OBJECT *device)
>>> +{
>>> +    KDEVICE_QUEUE_ENTRY *entry;
>>> +    IRP *irp;
>>> +
>>> +    if (!(entry = KeRemoveDeviceQueue(&device->DeviceQueue))) irp =
>>> NULL;
>>> +    else irp = CONTAINING_RECORD(entry, IRP,
>>> Tail.Overlay.DeviceQueueEntry);
>>> +
>>> +    return irp;
>>> +}
>>> +
>>> +static NTSTATUS push_pending_irp(DEVICE_OBJECT *device, IRP *irp)
>>> +{
>>> +    do { IoMarkIrpPending(irp); }
>>> +    while (!KeInsertDeviceQueue(&device->DeviceQueue,
>>> &irp->Tail.Overlay.DeviceQueueEntry));
>>> +
>>> +    return STATUS_PENDING;
>>> +}
>>> +
>>
>> See, the problem is that as far as I can tell (though it's tricky,
>> because Microsoft doesn't actually provide any code samples) this
>> doesn't match the intended usage pattern of KeInsertDeviceQueue(), which
>> makes it look like a mistake. And it still doesn't look better than this:
>>
>> static IRP *pop_pending_irp(DEVICE_OBJECT *device)
>> {
>>       LIST_ENTRY *entry;
>>       IRQL irql;
>>       IRP *irp;
>>
>>       KeAcquireSpinLock(&device->lock, &irql);
>>       entry = RemoveHeadList(&device->pending_irps);
>>       KeReleaseSpinLock(&device->lock, irql);
>>
>>       if (entry == &device->pending_irps)
>>           return NULL;
>>       return CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
>> }
>>
>> static void push_pending_irp(DEVICE_OBJECT *device, IRP *irp)
>> {
>>       IRQL irql;
>>
>>       IoMarkIrpPending(irp);
>>       KeAcquireSpinLock(&device->lock, &irql);
>>       InsertTailList(&device->pending_irps, &irp->Tail.Overlay.ListEntry);
>>       KeReleaseSpinLock(&device->lock, irql);
>> }
>>
>> A couple more lines of code, and it uses a couple extra fields, but it's
>> that much clearer what it's supposed to be doing.
>>
> 
> I don't know, it just feels a waste to have this code duplicated every
> where, so the smaller it is the better. Not having to add these members
> in every driver (and in particular the spinlock) is also better IMHO,
> especially if we consider the functions could then be shared in a common
> private header.

Don't get me wrong, I'm in favor of sharing this code. But I'd also like 
the shared code to be clear and readable, and I just don't think that 
KDEVICE_QUEUE APIs allow for that.

Plus, if we really want to, we could always alias the KDEVICE_QUEUE with 
our own to save memory. After all, we're not using it...

> I think it's also easier to make mistakes with the acquire/release, and
> the temptation to reuse the lock for something else than this
> KeInsertDeviceQueue little specificity.

Sure, it should probably be renamed "queue_lock" or something.

>> On a sort of related note, though, I have considered having some sort of
>> framework for PnP drivers. Device removal is one reason why. In this
>> case WDF might actually not be a terrible fit; I've been sort of working
>> on reimplementing that locally, and while its API isn't good, it also
>> doesn't seem very bad. But I also haven't gotten very far into it, so
>> it's worth examining how it handles certain things before proceeding.
>>
> 
> Yes, that would be nice.
>