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

[Zebediah Figura (she/her)]

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

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.
> +static void remove_pending_irps(DEVICE_OBJECT *device)
> +{
> - IRP *irp;
> -
> - while ((irp = pop_pending_irp(device)))
> - {
> -
>    irp->IoStatus.Status = STATUS_DEVICE_REMOVED;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> - }
>
> +}
> -
>
> static struct list device_list = LIST_INIT(device_list);
>
> static FAST_MUTEX driver_lock;
>
> @@ -207,6 +242,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp)
>               POWER_STATE state = {.DeviceState = PowerDeviceD0};
>               NTSTATUS status;
> -
>        KeInitializeDeviceQueue(&device_obj->DeviceQueue);
> -
>         ok(!stack->Parameters.StartDevice.AllocatedResources, "expected no resources\n");
>         ok(!stack->Parameters.StartDevice.AllocatedResourcesTranslated, "expected no translated resources\n");
>
> @@ -223,6 +260,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++;
> -
>        remove_pending_irps(device_obj);
>         if (device->removed)
>         {
>             IoSetDeviceInterfaceState(&device->child_symlink, FALSE);
>
> @@ -289,8 +334,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++;
> -
>        remove_pending_irps(device_obj);
> -
>        ret = STATUS_SUCCESS;
> -
>        break;
> -
> -
>    case IRP_MN_QUERY_REMOVE_DEVICE:
> -
>        query_remove_device_count++;
> -
>        remove_pending_irps(device_obj);
> -
>        ret = STATUS_SUCCESS;
> -
>        break;
> -
> -
>    case IRP_MN_CANCEL_REMOVE_DEVICE:
> -
>        cancel_remove_device_count++;
>         ret = STATUS_SUCCESS;
>         break;
> }
>
> @@ -579,8 +636,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 +649,20 @@ 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:
> -
>        return push_pending_irp(device_obj, irp);
> -
> -
>    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 +678,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 432bc168259..8fdb6c75f99 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_DEVICE_REMOVED, "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);