Recently I received several crashdumps from diffrent sources with the same problem. After analysis there is apparent bug in my code. 5 years old and coming from original BulkUsb sample. This is IMO design problem and it is still present in WDK 6000. Also, it seems other WDK samples suffer from the same problem. For example, Toaster although there it could be protected by different means.
I’d like to discuss the design decision which IMO inevitably leads to this problem. There are race conditions which can’t be reasonably avoided. The window is narrow and the problem didn’t appear until the last Core Duo CPUs became widely available.
Well, the code looks like this (from WDK 6000 BulkUsb sample):
-
IssueWaitWake() function uses PoRequestPowerIrp() to send WW IRP.
-
BulkUsb_DispatchPower() handler receives WW IRP and processes it:
case IRP_MN_WAIT_WAKE:
(PIRP) InterlockedExchangePointer(&deviceExtension->WaitWakeIrp, Irp);
if(InterlockedExchange(&deviceExtension->FlagWWDispatched, 1)){
// CancelWaitWake ran before we could store the IRP. We must
// cancel the IRP here.
if(InterlockedExchangePointer(&deviceExtension->WaitWakeIrp, NULL)){
// CancelWaitWake cannot touch this irp now and we will complete it
…
}
}
>> possible race conditions <<<
IoMarkIrpPending(Irp);
IoCopyCurrentIrpStackLocationToNext(Irp);
IoSetCompletionRoutine(Irp, WaitWakeCompletionRoutine, deviceExtension, TRUE, TRUE, TRUE);
PoStartNextPowerIrp(Irp);
ntStatus = PoCallDriver(deviceExtension->TopOfStackDeviceObject, Irp);
- In parallel, CancelWaitWake() function can be run on different CPU/core:
// If our irp has not been dispatched yet, WaitWakeIrp may be NULL. However,
// the IRP may already have been sent and thus we still need to cancel it. By
// setting FlagWWDispatched here our dispatch routine will know to cancel the IRP
//
InterlockedExchange(&DeviceExtension->FlagWWDispatched, 1);
Irp = (PIRP) InterlockedExchangePointer(&DeviceExtension->WaitWakeIrp, NULL);
if(Irp) {
IoCancelIrp(Irp);
if(InterlockedExchange(&DeviceExtension->FlagWWCancel, 1)) {
PoStartNextPowerIrp(Irp);
Irp->IoStatus.Status = STATUS_CANCELLED;
Irp->IoStatus.Information = 0;
IoCompleteRequest(Irp, IO_NO_INCREMENT);
}
}
Now imagine code #3 runs at the point marked as >>> possible race conditions <<< at code fragment #2. Let’s presume FlagWWCancel is 1 (set from completion routine and the logic is IMO also wrong) and IRP is completed. #2 continues and several functions are applied to completed IRP. It leads to immediate BSODs (NULL pointers and so on) until PoCallDriver() is called. Then it leads to double IRP completion.
Maybe it can be “fixed” some way using synchronization and next flags but the design problem I see it following. The code which cancels WW IRP calls IoCancelIrp() which is OK and then, under some circumstances, calls IoCompleteRequest(). Which is IMO plain wrong. The code doesn’t own the IRP which was previously sent down the stack. I guess nobody but the driver which receives the power (!) IRP in the dispatch routine and currently processes it or has it pending, has rigth to call IoCompleteRequest(). Once the IRP is passed down the stack, it shouldn’t be touched until completion routine is called. And in this case, for power IRP created by PoRequestPowerIrp(), I don’t see a reason why completion routine should return STATUS_MORE_PROCESSING_REQUIRED and depend on other code to call IoCompleteRequest().
Maybe I’m missing something. Opinions welcome.
Best regards,
Michal Vodicka
UPEK, Inc.
[xxxxx@upek.com, http://www.upek.com]