[PATCH 3/5] ntoskrnl.exe/tests: Add some pending / remove tests.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Mon Jun 28 11:15:11 CDT 2021
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.
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);
>
More information about the wine-devel
mailing list