When Cancel Safe Queues Are Not So Safe...

Discussion in 'Windows Vista Drivers' started by 0dbell, Jun 28, 2007.

  1. 0dbell

    0dbell Guest

    Case in question:

    1. An IRP handling thread calls IoCsqRemoveNextIrp and starts
    servicing the IRP, which may be non-atomic (or sometimes worse -
    waiting for some condititions to be fulfilled.

    2. IRP_MJ_CLEANUP is issued/received. The cleanup handler happily
    empties all pending IRPs in CSQ (IoCsqRemoveNextIrp). It even
    correctly identifies that there is an "IRP in process" that was
    removed from the CSQ, but was not completed yet. So... it attempts to
    cancel it as well:

    pendingIRP->IoStatus.Information = 0;
    pendingIRP->IoStatus.Status = STATUS_CANCELLED;
    IoCompleteRequest(pendingIRP, IO_NO_INCREMENT);

    But... what if the cleanup handler was called exactly at the point
    where the IRP handling thread was *in the middle of completing* the
    IRP? That is:

    pendingIRP->IoStatus.Information = numbytes;
    ==> CleanupHandler() was called here.
    pendingIRP->IoStatus.Status = STATUS_SUCCESS;
    IoCompleteRequest(pendingIRP, IO_NO_INCREMENT);

    Will chaos ensue?

    This looks to me like a case where a SpinLock is needed... (around
    those 3 lines of code).

    But... one of the "advertising feaures" of CSQs was that no external
    spinlocks are needed anymore if CSQs are used correctly.

    Am I missing something?

    Thanks,
    Don
     
    0dbell, Jun 28, 2007
    #1
    1. Advertisements

  2. This is a very well-known issue. You should use atomic operations
    (ExInterlockedXxx) to prevent this race.

    For your convinience, IoSetCancelRoutine is such an atomic operation.

    Walter Oney's book describes this in detail, and provides the logic to
    implement your own CSQ.

    If you're a novice and feel not so comfortable with all of this - just do
    not try to cancel the "in progress" IRP already taken from CSQ. The IRPs in the
    queue are automatically cancellable, CSQ implementation does this for you.
     
    Maxim S. Shatskih, Jun 29, 2007
    #2
    1. Advertisements

  3. 0dbell

    0dbell Guest

    Maxim, thank you for your answer. I am glad to realize that I am not
    dreaming a non-issue.

    You also correctly identified that I am a novice. However, I am trying
    to "do things right", so if handling the "in progress" IRP is the
    right thing to do, I prefer facing this challent sooner than later.

    I actually tried surrounding those 3 statements with a spinlock but
    soon realized that this is not a good idea since a call is made to
    IoCompleteRequest() while the spinlock is held which wreaks havoc...

    So, I temporarily replaced it by:

    KeEnterCriticalRegion();
    {
    pendingIRP->IoStatus.Status = STATUS_SUCCESS;
    pendingIRP->IoStatus.Information = numbytes;
    IoCompleteRequest(pendingIRP, IO_NO_INCREMENT);
    InterlockedExchangePointer(&dx->pCurIrpRemovedFromCsq, NULL);
    }
    KeLeaveCriticalRegion();

    But despite this being working at the moment, I think that using
    critical region in such manner (PASSIVE_LEVEL) is incorrect. Hence
    this is temprary until I find "the right way".

    As for using IoSetCancelRoutine I am now confused (just as before
    posting my original question): I thought that CSQ were meant to
    eliminate the need for explicit IoSetCancelRoutine? Isn't this why I
    implemented CsqCompleteCanceledIrp()?

    I tried to follow your advice of "just do not try to cancel the "in
    progress" IRP already taken from CSQ" but that actually created a
    different problem of IRP_MJ_CLOSE not following IRP_MJ_CLEANUP in
    certain circumstances.

    Any light you can shed on the issues I raisd here?

    Thanks,
    Don
     
    0dbell, Jun 29, 2007
    #3
  4. 0dbell

    ijor Guest

    CSQ is *cancel* safe, it is not "cleanup safe". If you can have pending IRPs
    for a very long time, then you need to mantain a "cleaned-up" state for the
    file object. Consider what will happen if for some reason an I/O IRP arrives
    to your dispatch routine after the IRP_MJ_CLEANUP completed.
     
    ijor, Jun 29, 2007
    #4
  5. 0dbell

    Tim Roberts Guest

    Remember that you do not have to cancel an IRP if it is going to be
    completed soon anyway.
     
    Tim Roberts, Jul 2, 2007
    #5
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.