Re: [PATCH] ntoskrnl.exe: Implement Ke(Initialize|Insert|Remove)DeviceQueue.

[Zebediah Figura (she/her)]

On 6/22/21 7:02 AM, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon
>
> MSDN says that the cancel spinlock has to be held whenever these device
> queues are used, when setting cancel routines, but it's not clear to me
> why, as IRP cancellation has apparently no effect on these functions
> behavior (I initially thought they should check and not queue cancelled
> IRPs, but it looks like they don't).

Where do you see this? I couldn't find any mention of it from a brief 
search.
> dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  6 +-
>   dlls/ntoskrnl.exe/sync.c            | 41 ++++++++++++
>   dlls/ntoskrnl.exe/tests/driver.c    | 96 +++++++++++++++++++++++++++++
>   include/ddk/wdm.h                   |  4 ++
>   4 files changed, 144 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..9063608be4a 100644
> --- a/dlls/ntoskrnl.exe/sync.c
> +++ b/dlls/ntoskrnl.exe/sync.c
> @@ -1362,3 +1362,44 @@ 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 f1c72d8fdaf..3446dc8e529 100644
> --- a/dlls/ntoskrnl.exe/tests/driver.c
> +++ b/dlls/ntoskrnl.exe/tests/driver.c
> @@ -141,6 +141,101 @@ 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);

Could we test the actual contents of the queue list after all these calls?
> -
> - 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");
> -
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
> - ok(queue.Busy, "expected Busy state\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");
> -
> - entry = KeRemoveDeviceQueue(&queue);
> - ok(entry == NULL, "expected KeRemoveDeviceQueue to return NULL\n");
> - ok(!queue.Busy, "unexpected Busy state\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");
> - ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry);
> - ok(ret, "expected KeInsertDeviceQueue to insert IRP\n");
> - ok(queue.Busy, "expected Busy state\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");
> -
> - 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");
> -
> - 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");
> -
> - IoFreeIrp(irp);
>
> +}
> - static void test_mdl_map(void)
> {
>     char buffer[20] = "test buffer";
>
> @@ -2126,6 +2221,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);