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

[Rémi Bernon]

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.

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.

> 
> 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.
-- 
Rémi Bernon <rbernon at codeweavers.com>