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@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.