Re: [PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Zebediah Figura (she/her)]

On 7/1/21 2:51 AM, Rémi Bernon wrote:
> Handle IRP_MN_SURPRISE_REMOVAL and notify device thread to stop it, but
> only wait for it in IRP_MN_REMOVE_DEVICE, as it's probably waiting for
> an IRP sent to the lower driver, which needs to be notified of removal
> too first.
> Signed-off-by: Rémi Bernon
>
> dlls/hidclass.sys/device.c | 39 ++++++++++++++++++++++++++++++++++++++
>   dlls/hidclass.sys/hid.h    |  3 +++
>   dlls/hidclass.sys/pnp.c    | 24 ++++++++++++++++++++++-
>   3 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> index 82366ad1888..d3f5fa90fc7 100644
> --- a/dlls/hidclass.sys/device.c
> +++ b/dlls/hidclass.sys/device.c
> @@ -431,11 +431,24 @@ NTSTATUS WINAPI pdo_ioctl(DEVICE_OBJECT *device, IRP *irp)
>       NTSTATUS rc = STATUS_SUCCESS;
>       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
>       BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> -
>
> BOOL removed;
> -
>
> KIRQL irql;
>
> irp->IoStatus.Information = 0;
>
> TRACE("device %p ioctl(%x)\n", device, irpsp->Parameters.DeviceIoControl.IoControlCode);
> -
>
> KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> -
>
> removed = ext->u.pdo.removed;
> -
>
> KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> -
> -
>
> if (removed)
> -
>
> {
> -
>    irp->IoStatus.Status = STATUS_DELETE_PENDING;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>    return STATUS_DELETE_PENDING;
> -
>
> }
> -
> switch (irpsp->Parameters.DeviceIoControl.IoControlCode)
> {
>     case IOCTL_HID_GET_POLL_FREQUENCY_MSEC:
>
> @@ -588,6 +601,19 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp)
>       NTSTATUS rc = STATUS_SUCCESS;
>       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
>       int ptr = -1;
> -
>
> BOOL removed;
> -
>
> KIRQL irql;
> -
> -
>
> KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> -
>
> removed = ext->u.pdo.removed;
> -
>
> KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> -
> -
>
> if (removed)
> -
>
> {
> -
>    irp->IoStatus.Status = STATUS_DELETE_PENDING;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>    return STATUS_DELETE_PENDING;
> -
>
> }
>
> packet = malloc(buffer_size);
>  ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext );
>
> @@ -662,7 +688,20 @@ NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp)
>       const WINE_HIDP_PREPARSED_DATA *data = ext->u.pdo.preparsed_data;
>       HID_XFER_PACKET packet;
>       ULONG max_len;
> -
>
> BOOL removed;
>  NTSTATUS rc;
> -
>
> KIRQL irql;
> -
> -
>
> KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> -
>
> removed = ext->u.pdo.removed;
> -
>
> KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> -
> -
>
> if (removed)
> -
>
> {
> -
>    irp->IoStatus.Status = STATUS_DELETE_PENDING;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>    return STATUS_DELETE_PENDING;
> -
>
> }
>
> irp->IoStatus.Information = 0;
>
> diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h
> index 5a502840691..863bb9a8bd4 100644
> --- a/dlls/hidclass.sys/hid.h
> +++ b/dlls/hidclass.sys/hid.h
> @@ -67,6 +67,9 @@ typedef struct _BASE_DEVICE_EXTENSION
>               KSPIN_LOCK irp_queue_lock;
>               LIST_ENTRY irp_queue;
> -
>        KSPIN_LOCK lock;
> -
>        BOOL       removed;

Style nitpick, but none of the other members of this structure are 
aligned...
> -
>         BOOL is_mouse;
>         UNICODE_STRING mouse_link_name;
>         BOOL is_keyboard;
>
> diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> index 5f59257cdf7..5d6fb3d068c 100644
> --- a/dlls/hidclass.sys/pnp.c
> +++ b/dlls/hidclass.sys/pnp.c
> @@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>       }
>   }
>
> +static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
> +{
> - IRP *irp;
> -
> - while ((irp = pop_irp_from_queue(ext)))
> - {
> -
>    irp->IoStatus.Status = STATUS_DELETE_PENDING;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> - }
>
> +}
> -
>
> static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> {
>     IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
>     BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
>     NTSTATUS status = irp->IoStatus.Status;
> -
>
> IRP *queued_irp;
> -
>
> KIRQL irql;
>
> TRACE("irp %p, minor function %#x.\n", irp, irpsp->MinorFunction);
>
> @@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>                   IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
>               if (ext->u.pdo.is_keyboard)
>                   IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
> -
> -
>        ext->u.pdo.removed = FALSE;
>         status = STATUS_SUCCESS;
>         break;
>
>     case IRP_MN_REMOVE_DEVICE:
>     {
> -
>        IRP *queued_irp;
> -
>        remove_pending_irps(ext);

In that case there's no reason to delete queued IRPs later in the handler.
>           send_wm_input_device_change(ext, GIDC_REMOVAL);
>
> @@ -494,6 +509,13 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>           }
>       case IRP_MN_SURPRISE_REMOVAL:
> -
>        KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> -
>        ext->u.pdo.removed = TRUE;
> -
>        KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> -
> -
>        remove_pending_irps(ext);
> -
> -
>        SetEvent(ext->u.pdo.halt_event);
>         status = STATUS_SUCCESS;
>         break;