[PATCH v2 1/5] ntoskrnl.exe/tests: Retry timer wait to address spurious failures.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v2: Use a custom queue, remove IRPs with STATUS_DELETE_PENDING status. dlls/ntoskrnl.exe/tests/driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index f1c72d8fdaf..918b4632c40 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -756,6 +756,8 @@ static void test_sync(void) ok(ret == 0, "got %#x\n", ret); ret = wait_single(&timer, 0); + /* aliasing makes it sometimes succeeds, try again in that case */ + if (ret == 0) ret = wait_single(&timer, 0); ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); ret = wait_single(&timer, -40 * 10000); -- 2.32.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 6 +- dlls/ntoskrnl.exe/sync.c | 42 +++++++++++ dlls/ntoskrnl.exe/tests/driver.c | 113 ++++++++++++++++++++++++++++ include/ddk/wdm.h | 4 + 4 files changed, 162 insertions(+), 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index 5fdaa922f45..c1244a42146 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec @@ -567,7 +567,7 @@ @ stub KeI386SetGdtSelector @ stub KeIcacheFlushCount @ stdcall KeInitializeApc(ptr ptr long ptr ptr ptr long ptr) -@ stub KeInitializeDeviceQueue +@ stdcall KeInitializeDeviceQueue(ptr) @ stdcall KeInitializeDpc(ptr ptr ptr) @ stdcall KeInitializeEvent(ptr long long) @ stub KeInitializeInterrupt @@ -579,7 +579,7 @@ @ stdcall KeInitializeTimer(ptr) @ stdcall KeInitializeTimerEx(ptr long) @ stub KeInsertByKeyDeviceQueue -@ stub KeInsertDeviceQueue +@ stdcall KeInsertDeviceQueue(ptr ptr) @ stub KeInsertHeadQueue @ stdcall KeInsertQueue(ptr ptr) @ stub KeInsertQueueApc @@ -617,7 +617,7 @@ @ stdcall KeReleaseSpinLockFromDpcLevel(ptr) @ stub KeRemoveByKeyDeviceQueue @ stub KeRemoveByKeyDeviceQueueIfBusy -@ stub KeRemoveDeviceQueue +@ stdcall KeRemoveDeviceQueue(ptr) @ stub KeRemoveEntryDeviceQueue @ stub KeRemoveQueue @ stub KeRemoveQueueDpc diff --git a/dlls/ntoskrnl.exe/sync.c b/dlls/ntoskrnl.exe/sync.c index 445c8d890ab..5d028792e5f 100644 --- a/dlls/ntoskrnl.exe/sync.c +++ b/dlls/ntoskrnl.exe/sync.c @@ -1362,3 +1362,45 @@ BOOLEAN WINAPI KeSetTimer(KTIMER *timer, LARGE_INTEGER duetime, KDPC *dpc) return KeSetTimerEx(timer, duetime, 0, dpc); } + +void WINAPI KeInitializeDeviceQueue( KDEVICE_QUEUE *queue ) +{ + TRACE( "queue %p.\n", queue ); + + KeInitializeSpinLock( &queue->Lock ); + InitializeListHead( &queue->DeviceListHead ); + queue->Busy = FALSE; + queue->Type = IO_TYPE_DEVICE_QUEUE; + queue->Size = sizeof(*queue); +} + +BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry ) +{ + KIRQL irql; + + TRACE( "queue %p, entry %p.\n", queue, entry ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if ((entry->Inserted = queue->Busy)) + InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry ); + queue->Busy = TRUE; + KeReleaseSpinLock( &queue->Lock, irql ); + + return entry->Inserted; +} + +KDEVICE_QUEUE_ENTRY *WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue ) +{ + LIST_ENTRY *entry = NULL; + KIRQL irql; + + TRACE( "queue %p.\n", queue ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE; + else entry = RemoveHeadList( &queue->DeviceListHead ); + KeReleaseSpinLock( &queue->Lock, irql ); + + if (!entry) return NULL; + return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry ); +} diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index 918b4632c40..d5f8b07bd28 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -141,6 +141,118 @@ static void test_irp_struct(IRP *irp, DEVICE_OBJECT *device) IoFreeIrp(irp); } +static int cancel_queue_cnt; + +static void WINAPI cancel_queued_irp(DEVICE_OBJECT *device, IRP *irp) +{ + IoReleaseCancelSpinLock(irp->CancelIrql); + ok(irp->Cancel == TRUE, "Cancel = %x\n", irp->Cancel); + ok(!irp->CancelRoutine, "CancelRoutine = %p\n", irp->CancelRoutine); + irp->IoStatus.Status = STATUS_CANCELLED; + irp->IoStatus.Information = 0; + cancel_queue_cnt++; +} + +static void test_queue(void) +{ + KDEVICE_QUEUE_ENTRY *entry; + KDEVICE_QUEUE queue; + BOOLEAN ret; + KIRQL irql; + IRP *irp; + + irp = IoAllocateIrp(1, FALSE); + + memset(&queue, 0xcd, sizeof(queue)); + KeInitializeDeviceQueue(&queue); + ok(!queue.Busy, "unexpected Busy state\n"); + ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size); + ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n", queue.Type); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(!entry, "expected KeRemoveDeviceQueue to return NULL\n"); + ok(!queue.Busy, "unexpected Busy state\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(!IsListEmpty(&queue.DeviceListHead), "unexpected empty queue list\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry == NULL, "expected KeRemoveDeviceQueue to return NULL\n"); + ok(!queue.Busy, "unexpected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoCancelIrp(irp); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoFreeIrp(irp); + + irp = IoAllocateIrp(1, FALSE); + + IoAcquireCancelSpinLock(&irql); + IoSetCancelRoutine(irp, cancel_queued_irp); + IoReleaseCancelSpinLock(irql); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + IoCancelIrp(irp); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(cancel_queue_cnt, "expected cancel routine to be called\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(cancel_queue_cnt, "expected cancel routine to be called\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoFreeIrp(irp); +} + static void test_mdl_map(void) { char buffer[20] = "test buffer"; @@ -2128,6 +2240,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st test_init_funcs(); test_load_driver(); test_sync(); + test_queue(); test_version(); test_stack_callout(); test_lookaside_list(); diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h index 447833c4bc9..9bc3305471c 100644 --- a/include/ddk/wdm.h +++ b/include/ddk/wdm.h @@ -397,6 +397,7 @@ typedef struct _WAIT_CONTEXT_BLOCK { #define IO_TYPE_ERROR_LOG 0x0b #define IO_TYPE_ERROR_MESSAGE 0x0c #define IO_TYPE_DEVICE_OBJECT_EXTENSION 0x0d +#define IO_TYPE_DEVICE_QUEUE 0x14 typedef struct _DEVICE_OBJECT { CSHORT Type; @@ -1750,6 +1751,7 @@ void WINAPI KeEnterCriticalRegion(void); void WINAPI KeGenericCallDpc(PKDEFERRED_ROUTINE,PVOID); ULONG WINAPI KeGetCurrentProcessorNumber(void); PKTHREAD WINAPI KeGetCurrentThread(void); +void WINAPI KeInitializeDeviceQueue(KDEVICE_QUEUE*); void WINAPI KeInitializeDpc(KDPC*,PKDEFERRED_ROUTINE,void*); void WINAPI KeInitializeEvent(PRKEVENT,EVENT_TYPE,BOOLEAN); void WINAPI KeInitializeMutex(PRKMUTEX,ULONG); @@ -1757,6 +1759,7 @@ void WINAPI KeInitializeSemaphore(PRKSEMAPHORE,LONG,LONG); void WINAPI KeInitializeSpinLock(KSPIN_LOCK*); void WINAPI KeInitializeTimerEx(PKTIMER,TIMER_TYPE); void WINAPI KeInitializeTimer(KTIMER*); +BOOLEAN WINAPI KeInsertDeviceQueue(KDEVICE_QUEUE*,KDEVICE_QUEUE_ENTRY*); void WINAPI KeLeaveCriticalRegion(void); ULONG WINAPI KeQueryActiveProcessorCountEx(USHORT); KAFFINITY WINAPI KeQueryActiveProcessors(void); @@ -1769,6 +1772,7 @@ LONG WINAPI KeReleaseMutex(PRKMUTEX,BOOLEAN); LONG WINAPI KeReleaseSemaphore(PRKSEMAPHORE,KPRIORITY,LONG,BOOLEAN); void WINAPI KeReleaseSpinLock(KSPIN_LOCK*,KIRQL); void WINAPI KeReleaseSpinLockFromDpcLevel(KSPIN_LOCK*); +KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue(KDEVICE_QUEUE*); LONG WINAPI KeResetEvent(PRKEVENT); void WINAPI KeRevertToUserAffinityThread(void); void WINAPI KeRevertToUserAffinityThreadEx(KAFFINITY affinity); -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93265 Your paranoid android. === w1064_tsign (64 bit report) === ntoskrnl.exe: driver.c:384: Test failed: Got unexpected test_load_image_notify_count 0.
Sorry I didn't notice this before, but... On 6/29/21 2:21 AM, Rémi Bernon wrote:
+BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry ) +{ + KIRQL irql; + + TRACE( "queue %p, entry %p.\n", queue, entry ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if ((entry->Inserted = queue->Busy)) + InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry ); + queue->Busy = TRUE; + KeReleaseSpinLock( &queue->Lock, irql ); + + return entry->Inserted; +}
I don't think it's thread-safe to access entry->Inserted outside of the lock.
+ +KDEVICE_QUEUE_ENTRY *WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue ) +{ + LIST_ENTRY *entry = NULL; + KIRQL irql; + + TRACE( "queue %p.\n", queue ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE; + else entry = RemoveHeadList( &queue->DeviceListHead ); + KeReleaseSpinLock( &queue->Lock, irql ); + + if (!entry) return NULL; + return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry ); +}
And while we're at it, should this unset entry->Inserted?
On 6/29/21 6:58 PM, Zebediah Figura (she/her) wrote:
Sorry I didn't notice this before, but...
On 6/29/21 2:21 AM, Rémi Bernon wrote:
+BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry ) +{ + KIRQL irql; + + TRACE( "queue %p, entry %p.\n", queue, entry ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if ((entry->Inserted = queue->Busy)) + InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry ); + queue->Busy = TRUE; + KeReleaseSpinLock( &queue->Lock, irql ); + + return entry->Inserted; +}
I don't think it's thread-safe to access entry->Inserted outside of the lock.
+ +KDEVICE_QUEUE_ENTRY *WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue ) +{ + LIST_ENTRY *entry = NULL; + KIRQL irql; + + TRACE( "queue %p.\n", queue ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE; + else entry = RemoveHeadList( &queue->DeviceListHead ); + KeReleaseSpinLock( &queue->Lock, irql ); + + if (!entry) return NULL; + return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry ); +}
And while we're at it, should this unset entry->Inserted?
Yeah maybe, I'll add a few tests to make sure. FWIW the next patches don't even need this anymore. -- Rémi Bernon <rbernon(a)codeweavers.com>
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(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.h | 2 + dlls/ntoskrnl.exe/tests/driver_pnp.c | 99 ++++++++++++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 30 ++++++++- 3 files changed, 125 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..98ca2ff7961 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,6 +41,56 @@ 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 irp_queue +{ + KSPIN_LOCK lock; + LIST_ENTRY list; +}; + +static IRP *irp_queue_pop(struct irp_queue *queue) +{ + KIRQL irql; + IRP *irp; + + KeAcquireSpinLock(&queue->lock, &irql); + if (IsListEmpty(&queue->list)) irp = NULL; + else irp = CONTAINING_RECORD(RemoveHeadList(&queue->list), IRP, Tail.Overlay.ListEntry); + KeReleaseSpinLock(&queue->lock, irql); + + return irp; +} + +static void irp_queue_push(struct irp_queue *queue, IRP *irp) +{ + KIRQL irql; + + KeAcquireSpinLock(&queue->lock, &irql); + InsertTailList(&queue->list, &irp->Tail.Overlay.ListEntry); + KeReleaseSpinLock(&queue->lock, irql); +} + +static void irp_queue_clear(struct irp_queue *queue) +{ + IRP *irp; + + while ((irp = irp_queue_pop(queue))) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } +} + +static void irp_queue_init(struct irp_queue *queue) +{ + KeInitializeSpinLock(&queue->lock); + InitializeListHead(&queue->list); +} + struct device { struct list entry; @@ -49,6 +99,7 @@ struct device BOOL removed; UNICODE_STRING child_symlink; DEVICE_POWER_STATE power_state; + struct irp_queue irp_queue; }; static struct list device_list = LIST_INIT(device_list); @@ -207,6 +258,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) POWER_STATE state = {.DeviceState = PowerDeviceD0}; NTSTATUS status; + irp_queue_init(&device->irp_queue); + ok(!stack->Parameters.StartDevice.AllocatedResources, "expected no resources\n"); ok(!stack->Parameters.StartDevice.AllocatedResourcesTranslated, "expected no translated resources\n"); @@ -223,6 +276,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) } case IRP_MN_REMOVE_DEVICE: + /* should've been checked and reset by IOCTL_WINETEST_CHECK_REMOVED */ + ok(remove_device_count == 0, "expected no IRP_MN_REMOVE_DEVICE\n"); + todo_wine ok(surprise_removal_count == 0, "expected no IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + + remove_device_count++; + irp_queue_clear(&device->irp_queue); if (device->removed) { IoSetDeviceInterfaceState(&device->child_symlink, FALSE); @@ -289,8 +350,20 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) break; } - case IRP_MN_QUERY_REMOVE_DEVICE: case IRP_MN_SURPRISE_REMOVAL: + surprise_removal_count++; + irp_queue_clear(&device->irp_queue); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_QUERY_REMOVE_DEVICE: + query_remove_device_count++; + irp_queue_clear(&device->irp_queue); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_CANCEL_REMOVE_DEVICE: + cancel_remove_device_count++; ret = STATUS_SUCCESS; break; } @@ -579,8 +652,10 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) } } -static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) +static NTSTATUS pdo_ioctl(DEVICE_OBJECT *device_obj, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) { + struct device *device = device_obj->DeviceExtension; + switch (code) { case IOCTL_WINETEST_CHILD_GET_ID: @@ -590,6 +665,22 @@ static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *st irp->IoStatus.Information = sizeof(device->id); return STATUS_SUCCESS; + case IOCTL_WINETEST_MARK_PENDING: + IoMarkIrpPending(irp); + irp_queue_push(&device->irp_queue, irp); + return STATUS_PENDING; + + case IOCTL_WINETEST_CHECK_REMOVED: + ok(remove_device_count == 0, "expected IRP_MN_REMOVE_DEVICE\n"); + ok(surprise_removal_count == 1, "expected IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + remove_device_count = 0; + surprise_removal_count = 0; + query_remove_device_count = 0; + cancel_remove_device_count = 0; + return STATUS_SUCCESS; + default: ok(0, "Unexpected ioctl %#x.\n", code); return STATUS_NOT_IMPLEMENTED; @@ -605,10 +696,10 @@ static NTSTATUS WINAPI driver_ioctl(DEVICE_OBJECT *device, IRP *irp) if (device == bus_fdo) status = fdo_ioctl(irp, stack, code); else - status = pdo_ioctl(device->DeviceExtension, irp, stack, code); + status = pdo_ioctl(device, irp, stack, code); irp->IoStatus.Status = status; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; } diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index a3c2a303a59..71dbfe1a5e9 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1150,10 +1150,11 @@ static void test_pnp_devices(void) }; HDEVNOTIFY notify_handle; DWORD size, type, dword; + HANDLE bus, child, tmp; OBJECT_ATTRIBUTES attr; UNICODE_STRING string; + OVERLAPPED ovl = {0}; IO_STATUS_BLOCK io; - HANDLE bus, child; HDEVINFO set; HWND window; BOOL ret; @@ -1349,6 +1350,14 @@ static void test_pnp_devices(void) CloseHandle(child); + ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, 0); + ok(!ret, "failed to open child: %#x\n", ret); + + ret = DeviceIoControl(child, IOCTL_WINETEST_MARK_PENDING, NULL, 0, NULL, 0, &size, &ovl); + ok(!ret, "DeviceIoControl succeded\n"); + ok(GetLastError() == ERROR_IO_PENDING, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + id = 1; ret = DeviceIoControl(bus, IOCTL_WINETEST_BUS_REMOVE_CHILD, &id, sizeof(id), NULL, 0, &size, NULL); ok(ret, "got error %u\n", GetLastError()); @@ -1357,7 +1366,24 @@ static void test_pnp_devices(void) ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal); - ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + ret = DeviceIoControl(child, IOCTL_WINETEST_CHECK_REMOVED, NULL, 0, NULL, 0, &size, NULL); + todo_wine ok(ret, "got error %u\n", GetLastError()); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + todo_wine ok(ret == STATUS_NO_SUCH_DEVICE, "got %#x\n", ret); + + ret = GetOverlappedResult(child, &ovl, &size, TRUE); + ok(!ret, "unexpected success.\n"); + ok(GetLastError() == ERROR_ACCESS_DENIED, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + + CloseHandle(child); + + pump_messages(); + ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); + ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); ok(ret == STATUS_OBJECT_NAME_NOT_FOUND, "got %#x\n", ret); CloseHandle(bus); -- 2.32.0
This causes failures on my 64-bit Windows 7 VM; apparently KeInitializeSpinLock isn't actually exported from ntoskrnl until Windows 8.
On 6/29/21 7:10 PM, Zebediah Figura (she/her) wrote:
This causes failures on my 64-bit Windows 7 VM; apparently KeInitializeSpinLock isn't actually exported from ntoskrnl until Windows 8.
I'm surprised that Marvin didn't report any failure. Should I then initialize the spinlock with 0? -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/29/21 3:14 PM, Rémi Bernon wrote:
On 6/29/21 7:10 PM, Zebediah Figura (she/her) wrote:
This causes failures on my 64-bit Windows 7 VM; apparently KeInitializeSpinLock isn't actually exported from ntoskrnl until Windows 8.
I'm surprised that Marvin didn't report any failure. Should I then initialize the spinlock with 0?
I believe Marvin didn't report any failure because the only 64-bit machine that can run tests is the one with test-signing enabled, which runs Windows 10. But I still usually use my Windows 7 machine (which also has test-signing enabled) for all the tests and I'd appreciate being able to continue doing that ;-) Windows conditionally (and the condition is big and terrible) defines KeInitializeSpinLock as an inline function. I think it'd be reasonable for us to do that unconditionally.
On 6/29/21 10:25 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 3:14 PM, Rémi Bernon wrote:
On 6/29/21 7:10 PM, Zebediah Figura (she/her) wrote:
This causes failures on my 64-bit Windows 7 VM; apparently KeInitializeSpinLock isn't actually exported from ntoskrnl until Windows 8.
I'm surprised that Marvin didn't report any failure. Should I then initialize the spinlock with 0?
I believe Marvin didn't report any failure because the only 64-bit machine that can run tests is the one with test-signing enabled, which runs Windows 10. But I still usually use my Windows 7 machine (which also has test-signing enabled) for all the tests and I'd appreciate being able to continue doing that ;-)
Windows conditionally (and the condition is big and terrible) defines KeInitializeSpinLock as an inline function. I think it'd be reasonable for us to do that unconditionally.
I'm not sure how to do that properly. It still needs to be exported, so a static FORCEINLINE version in the header will conflict with the one in ntoskrnl.exe. I can add something like that to the header, and #undef in ntoskrnl.exe: void WINAPI KeInitializeSpinLock(KSPIN_LOCK*); #define KeInitializeSpinLock( lock ) ((void)(*lock = 0)) Is that acceptable? Should it be guarded? I'm not sure if __WINESRC__ is there when building samples (especially the drivers). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/29/21 3:43 PM, Rémi Bernon wrote:
On 6/29/21 10:25 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 3:14 PM, Rémi Bernon wrote:
On 6/29/21 7:10 PM, Zebediah Figura (she/her) wrote:
This causes failures on my 64-bit Windows 7 VM; apparently KeInitializeSpinLock isn't actually exported from ntoskrnl until Windows 8.
I'm surprised that Marvin didn't report any failure. Should I then initialize the spinlock with 0?
I believe Marvin didn't report any failure because the only 64-bit machine that can run tests is the one with test-signing enabled, which runs Windows 10. But I still usually use my Windows 7 machine (which also has test-signing enabled) for all the tests and I'd appreciate being able to continue doing that ;-)
Windows conditionally (and the condition is big and terrible) defines KeInitializeSpinLock as an inline function. I think it'd be reasonable for us to do that unconditionally.
I'm not sure how to do that properly. It still needs to be exported, so a static FORCEINLINE version in the header will conflict with the one in ntoskrnl.exe.
I can add something like that to the header, and #undef in ntoskrnl.exe:
void WINAPI KeInitializeSpinLock(KSPIN_LOCK*); #define KeInitializeSpinLock( lock ) ((void)(*lock = 0))
Is that acceptable? Should it be guarded? I'm not sure if __WINESRC__ is there when building samples (especially the drivers).
The usual way to deal with this is to add a prefix to the exported function, e.g. NTOSKRNL_InterlockedCompareExchange.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index a512015d478..897be25ed51 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -235,6 +235,22 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) return dst; } +static void remove_pending_irps(DEVICE_OBJECT *device) +{ + struct device_extension *ext = device->DeviceExtension; + LIST_ENTRY *entry; + + EnterCriticalSection(&ext->cs); + while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) + { + IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); + queued_irp->IoStatus.u.Status = STATUS_DELETE_PENDING; + queued_irp->IoStatus.Information = 0; + IoCompleteRequest(queued_irp, IO_NO_INCREMENT); + } + LeaveCriticalSection(&ext->cs); +} + DEVICE_OBJECT *bus_create_hid_device(const WCHAR *busidW, WORD vid, WORD pid, WORD input, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, const platform_vtbl *vtbl, DWORD platform_data_size) @@ -687,17 +703,8 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; - LIST_ENTRY *entry; - EnterCriticalSection(&ext->cs); - while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) - { - IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); - queued_irp->IoStatus.u.Status = STATUS_DELETE_PENDING; - queued_irp->IoStatus.Information = 0; - IoCompleteRequest(queued_irp, IO_NO_INCREMENT); - } - LeaveCriticalSection(&ext->cs); + remove_pending_irps(device); ext->vtbl->free_device(device); -- 2.32.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; + case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device); -- 2.32.0
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); }
void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break;
+ case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; +
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device;
+ EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs);
ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
On 6/29/21 7:17 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; +
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
Although we do, I don't think there's guarantees that IRP_MN_SURPRISE_REMOVAL is always sent first. According to [1] the sequence could be IRP_MN_START_DEVICE -> IRP_MN_REMOVE_DEVICE -but probably no IRP could be queued in between- or IRP_MN_QUERY_REMOVE_DEVICE -> IRP_MN_REMOVE_DEVICE -which we don't support. [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/understandi... -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/29/21 12:25 PM, Rémi Bernon wrote:
On 6/29/21 7:17 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; +
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
Although we do, I don't think there's guarantees that IRP_MN_SURPRISE_REMOVAL is always sent first.
According to [1] the sequence could be IRP_MN_START_DEVICE -> IRP_MN_REMOVE_DEVICE -but probably no IRP could be queued in between- or IRP_MN_QUERY_REMOVE_DEVICE -> IRP_MN_REMOVE_DEVICE -which we don't support.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/understandi...
Right, but either way we shouldn't get any IRPs *after* IRP_MN_REMOVE_DEVICE. So nothing should be checking ext->removed, or trying to access the CS.
On 6/29/21 7:31 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 12:25 PM, Rémi Bernon wrote:
On 6/29/21 7:17 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; +
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
Although we do, I don't think there's guarantees that IRP_MN_SURPRISE_REMOVAL is always sent first.
According to [1] the sequence could be IRP_MN_START_DEVICE -> IRP_MN_REMOVE_DEVICE -but probably no IRP could be queued in between- or IRP_MN_QUERY_REMOVE_DEVICE -> IRP_MN_REMOVE_DEVICE -which we don't support.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/understandi...
Right, but either way we shouldn't get any IRPs *after* IRP_MN_REMOVE_DEVICE. So nothing should be checking ext->removed, or trying to access the CS.
I don't see exactly what prevents this, but alright. FWIW the CS is currently acquired in IRP_MN_REMOVE_DEVICE, even before these patches. The only way I can see that this is indeed unnecessary is if IRP_MN_REMOVE_DEVICE is special and only sent when all the handles to the device have been closed and all IRPs have been processed (either completed or marked pending). (And if there's no such guarantees I can easily see how this could go wrong if IRP_MN_REMOVE_DEVICE completes before another IRP finished its processing) As far as I can tell from the IRP_MN_REMOVE_DEVICE call sites, we just send it when the device is removed from the list, and it doesn't look obvious to me that there's such guarantees. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/29/21 5:04 PM, Rémi Bernon wrote:
On 6/29/21 7:31 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 12:25 PM, Rémi Bernon wrote:
On 6/29/21 7:17 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; +
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
Although we do, I don't think there's guarantees that IRP_MN_SURPRISE_REMOVAL is always sent first.
According to [1] the sequence could be IRP_MN_START_DEVICE -> IRP_MN_REMOVE_DEVICE -but probably no IRP could be queued in between- or IRP_MN_QUERY_REMOVE_DEVICE -> IRP_MN_REMOVE_DEVICE -which we don't support.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/understandi...
Right, but either way we shouldn't get any IRPs *after* IRP_MN_REMOVE_DEVICE. So nothing should be checking ext->removed, or trying to access the CS.
I don't see exactly what prevents this, but alright. FWIW the CS is currently acquired in IRP_MN_REMOVE_DEVICE, even before these patches.
The only way I can see that this is indeed unnecessary is if IRP_MN_REMOVE_DEVICE is special and only sent when all the handles to the device have been closed and all IRPs have been processed (either completed or marked pending).
(And if there's no such guarantees I can easily see how this could go wrong if IRP_MN_REMOVE_DEVICE completes before another IRP finished its processing)
As far as I can tell from the IRP_MN_REMOVE_DEVICE call sites, we just send it when the device is removed from the list, and it doesn't look obvious to me that there's such guarantees.
My understanding is that this *is* guaranteed, if only by convention, although on rereading all the documentation I can find I'm less sure. According to [2] the children always get remove IRPs first, and we have tests for this and everything. This implies (though of course I can't find it stated outright) that by the time we get IRP_MN_REMOVE_DEVICE nothing should be sending us a new IRP (no user space handles, no child devices...) But in theory any kernel component could find our device and send it IRPs... I suspect that ultimately the kernel doesn't stop you from sending IRPs on a removed device if you're still holding a reference to the device object. In theory I could even see components actually doing that, although I can't imagine why they would. On the other hand, you're supposed to clean up *all* your allocations during IRP_MN_REMOVE_DEVICE. But that excludes the device extension, and in theory you could put things there... In the case of winebus specifically, it only makes sense for winehid to be sending us IRPs (on behalf of hidclass), and hidclass shouldn't send any IRPs once it's shut down. See also my comment about hidclass's expectations in [3]. In fact, if we have any pending IRPs in IRP_MN_REMOVE_DEVICE, something's already gone wrong. We may want to take this approach generally, i.e. assume that IRPs should be complete by IRP_MN_REMOVE_DEVICE and that no new ones should be queued, and start complaining very vocally if any native (or builtin) drivers violate this. And if that situation does occur we can start examining how native drivers deal with it. From my years of working with DirectShow this pattern becomes familiar: design a complex interface involving several layers of callbacks, and then completely underspecify what functions are valid to call when and from which context, and how things should be synchronized. You'd think that the kernel would be more careful about this, but no. [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an... [3] https://www.winehq.org/pipermail/wine-devel/2021-June/189142.html
participants (3)
-
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)