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