From ca832988edab61a75b266ba7d73910fa1468c1a8 Mon Sep 17 00:00:00 2001 From: Bill Zissimopoulos Date: Tue, 18 May 2021 12:30:17 -0700 Subject: [PATCH] dll,sys: fix issue #369 The original WinFsp protocol for shutting down a file system was to issue an FSP_FSCTL_STOP control code to the fsctl device. This would set the IOQ to the "stopped" state and would also cancel all active IRP's. Cancelation of IRP's would sometimes free buffers that may have still been in use by the user mode file system threads; hence access violation. To fix this problem a new control code FSP_FSCTL_STOP0 is introduced. The new file system shutdown protocol is backwards compatible with the original one and works as follows: - First the file system process issues an FSP_FSCTL_STOP0 control code which sets the IOQ to the "stopped" state but does NOT cancel IRP's. - Then the file system process waits for its dispatcher threads to complete (see FspFileSystemStopDispatcher). - Finally the file system process issues an FSP_FSCTL_STOP control code which stops the (already stopped) IOQ and cancels all IRP's. --- inc/winfsp/fsctl.h | 3 +++ src/dll/fs.c | 8 +++++--- src/dll/fsctl.c | 10 ++++++++++ src/sys/driver.h | 2 +- src/sys/fsctl.c | 22 ++++++++++++++++++++++ src/sys/ioq.c | 21 ++++++++++++--------- src/sys/volume.c | 32 ++++++++++++++++++++++++++++---- 7 files changed, 81 insertions(+), 17 deletions(-) diff --git a/inc/winfsp/fsctl.h b/inc/winfsp/fsctl.h index 9d2a28fe..d1386c99 100644 --- a/inc/winfsp/fsctl.h +++ b/inc/winfsp/fsctl.h @@ -85,6 +85,8 @@ extern const __declspec(selectany) GUID FspFsvrtDeviceClassGuid = CTL_CODE(FILE_DEVICE_FILE_SYSTEM, 0x800 + 't', METHOD_OUT_DIRECT, FILE_ANY_ACCESS) #define FSP_FSCTL_STOP \ CTL_CODE(FILE_DEVICE_FILE_SYSTEM, 0x800 + 'S', METHOD_BUFFERED, FILE_ANY_ACCESS) +#define FSP_FSCTL_STOP0 \ + CTL_CODE(FILE_DEVICE_FILE_SYSTEM, 0x800 + 's', METHOD_BUFFERED, FILE_ANY_ACCESS) #define FSP_FSCTL_NOTIFY \ CTL_CODE(FILE_DEVICE_FILE_SYSTEM, 0x800 + 'n', METHOD_NEITHER, FILE_ANY_ACCESS) @@ -647,6 +649,7 @@ FSP_API NTSTATUS FspFsctlTransact(HANDLE VolumeHandle, PVOID RequestBuf, SIZE_T *PRequestBufSize, BOOLEAN Batch); FSP_API NTSTATUS FspFsctlStop(HANDLE VolumeHandle); +FSP_API NTSTATUS FspFsctlStop0(HANDLE VolumeHandle); FSP_API NTSTATUS FspFsctlNotify(HANDLE VolumeHandle, FSP_FSCTL_NOTIFY_INFO *NotifyInfo, SIZE_T Size); FSP_API NTSTATUS FspFsctlGetVolumeList(PWSTR DevicePath, diff --git a/src/dll/fs.c b/src/dll/fs.c index d0d525bc..9527e8f1 100644 --- a/src/dll/fs.c +++ b/src/dll/fs.c @@ -349,7 +349,7 @@ exit: FspFileSystemSetDispatcherResult(FileSystem, Result); - FspFsctlStop(FileSystem->VolumeHandle); + FspFsctlStop0(FileSystem->VolumeHandle); if (0 != DispatcherThread) { @@ -398,11 +398,13 @@ FSP_API VOID FspFileSystemStopDispatcher(FSP_FILE_SYSTEM *FileSystem) if (0 == FileSystem->DispatcherThread) return; - FspFsctlStop(FileSystem->VolumeHandle); + FspFsctlStop0(FileSystem->VolumeHandle); WaitForSingleObject(FileSystem->DispatcherThread, INFINITE); CloseHandle(FileSystem->DispatcherThread); FileSystem->DispatcherThread = 0; + + FspFsctlStop(FileSystem->VolumeHandle); } FSP_API VOID FspFileSystemSendResponse(FSP_FILE_SYSTEM *FileSystem, @@ -423,7 +425,7 @@ FSP_API VOID FspFileSystemSendResponse(FSP_FILE_SYSTEM *FileSystem, { FspFileSystemSetDispatcherResult(FileSystem, Result); - FspFsctlStop(FileSystem->VolumeHandle); + FspFsctlStop0(FileSystem->VolumeHandle); } } diff --git a/src/dll/fsctl.c b/src/dll/fsctl.c index 9fad2ae1..60664cfa 100644 --- a/src/dll/fsctl.c +++ b/src/dll/fsctl.c @@ -161,6 +161,16 @@ FSP_API NTSTATUS FspFsctlStop(HANDLE VolumeHandle) return STATUS_SUCCESS; } +FSP_API NTSTATUS FspFsctlStop0(HANDLE VolumeHandle) +{ + DWORD Bytes; + + if (!DeviceIoControl(VolumeHandle, FSP_FSCTL_STOP0, 0, 0, 0, 0, &Bytes, 0)) + return FspNtStatusFromWin32(GetLastError()); + + return STATUS_SUCCESS; +} + FSP_API NTSTATUS FspFsctlNotify(HANDLE VolumeHandle, FSP_FSCTL_NOTIFY_INFO *NotifyInfo, SIZE_T Size) { diff --git a/src/sys/driver.h b/src/sys/driver.h index 79536a0a..38780e22 100644 --- a/src/sys/driver.h +++ b/src/sys/driver.h @@ -946,7 +946,7 @@ NTSTATUS FspIoqCreate( ULONG IrpCapacity, PLARGE_INTEGER IrpTimeout, VOID (*CompleteCanceledIrp)(PIRP Irp), FSP_IOQ **PIoq); VOID FspIoqDelete(FSP_IOQ *Ioq); -VOID FspIoqStop(FSP_IOQ *Ioq); +VOID FspIoqStop(FSP_IOQ *Ioq, BOOLEAN CancelIrps); BOOLEAN FspIoqStopped(FSP_IOQ *Ioq); VOID FspIoqRemoveExpired(FSP_IOQ *Ioq, UINT64 InterruptTime); BOOLEAN FspIoqPostIrpEx(FSP_IOQ *Ioq, PIRP Irp, BOOLEAN BestEffort, NTSTATUS *PResult); diff --git a/src/sys/fsctl.c b/src/sys/fsctl.c index b6b18cc9..80d3f51e 100644 --- a/src/sys/fsctl.c +++ b/src/sys/fsctl.c @@ -95,6 +95,28 @@ static NTSTATUS FspFsctlFileSystemControl( Result = FspVolumeTransact(FsctlDeviceObject, Irp, IrpSp); break; case FSP_FSCTL_STOP: + case FSP_FSCTL_STOP0: + /* Fix GitHub issue #369 + * + * The original WinFsp protocol for shutting down a file system was to issue + * an FSP_FSCTL_STOP control code to the fsctl device. This would set the IOQ + * to the "stopped" state and would also cancel all active IRP's. Cancelation + * of IRP's would sometimes free buffers that may have still been in use by + * the user mode file system threads; hence access violation. + * + * To fix this problem a new control code FSP_FSCTL_STOP0 is introduced. The + * new file system shutdown protocol is backwards compatible with the original + * one and works as follows: + * + * - First the file system process issues an FSP_FSCTL_STOP0 control code which + * sets the IOQ to the "stopped" state but does NOT cancel IRP's. + * + * - Then the file system process waits for its dispatcher threads to complete + * (see FspFileSystemStopDispatcher). + * + * - Finally the file system process issues an FSP_FSCTL_STOP control code + * which stops the (already stopped) IOQ and cancels all IRP's. + */ if (0 != IrpSp->FileObject->FsContext2) Result = FspVolumeStop(FsctlDeviceObject, Irp, IrpSp); break; diff --git a/src/sys/ioq.c b/src/sys/ioq.c index f5d58e24..dbab2390 100644 --- a/src/sys/ioq.c +++ b/src/sys/ioq.c @@ -526,12 +526,12 @@ NTSTATUS FspIoqCreate( VOID FspIoqDelete(FSP_IOQ *Ioq) { - FspIoqStop(Ioq); + FspIoqStop(Ioq, TRUE); FspIoqEventFinalize(&Ioq->PendingIrpEvent); FspFree(Ioq); } -VOID FspIoqStop(FSP_IOQ *Ioq) +VOID FspIoqStop(FSP_IOQ *Ioq, BOOLEAN CancelIrps) { KIRQL Irql; KeAcquireSpinLock(&Ioq->SpinLock, &Irql); @@ -540,13 +540,16 @@ VOID FspIoqStop(FSP_IOQ *Ioq) FspIoqEventSet(&Ioq->PendingIrpEvent); /* equivalent to FspIoqPendingResetSynch(Ioq) */ KeReleaseSpinLock(&Ioq->SpinLock, Irql); - PIRP Irp; - while (0 != (Irp = IoCsqRemoveNextIrp(&Ioq->PendingIoCsq, 0))) - Ioq->CompleteCanceledIrp(Irp); - while (0 != (Irp = FspCsqRemoveNextIrp(&Ioq->ProcessIoCsq, 0))) - Ioq->CompleteCanceledIrp(Irp); - while (0 != (Irp = FspCsqRemoveNextIrp(&Ioq->RetriedIoCsq, 0))) - Ioq->CompleteCanceledIrp(Irp); + if (CancelIrps) + { + PIRP Irp; + while (0 != (Irp = IoCsqRemoveNextIrp(&Ioq->PendingIoCsq, 0))) + Ioq->CompleteCanceledIrp(Irp); + while (0 != (Irp = FspCsqRemoveNextIrp(&Ioq->ProcessIoCsq, 0))) + Ioq->CompleteCanceledIrp(Irp); + while (0 != (Irp = FspCsqRemoveNextIrp(&Ioq->RetriedIoCsq, 0))) + Ioq->CompleteCanceledIrp(Irp); + } } BOOLEAN FspIoqStopped(FSP_IOQ *Ioq) diff --git a/src/sys/volume.c b/src/sys/volume.c index 3c033683..432e712a 100644 --- a/src/sys/volume.c +++ b/src/sys/volume.c @@ -410,7 +410,7 @@ static VOID FspVolumeDeleteNoLock( IrpSp->FileObject->FsContext2 = 0; /* stop the I/O queue */ - FspIoqStop(FsvolDeviceExtension->Ioq); + FspIoqStop(FsvolDeviceExtension->Ioq, TRUE); /* do we have a virtual disk device or are we registered with fsmup? */ if (0 != FsvolDeviceExtension->FsvrtDeviceObject) @@ -1001,7 +1001,7 @@ NTSTATUS FspVolumeTransact( if (!FspFsctlTransactCanProduceRequest(Request, BufferEnd)) break; } - + if (0 >= LoopCount--) /* upper bound on loop guarantees forward progress! */ break; @@ -1056,13 +1056,37 @@ NTSTATUS FspVolumeStop( ASSERT(IRP_MJ_FILE_SYSTEM_CONTROL == IrpSp->MajorFunction); ASSERT(IRP_MN_USER_FS_REQUEST == IrpSp->MinorFunction); - ASSERT(FSP_FSCTL_STOP == IrpSp->Parameters.FileSystemControl.FsControlCode); + ASSERT( + FSP_FSCTL_STOP == IrpSp->Parameters.FileSystemControl.FsControlCode || + FSP_FSCTL_STOP0 == IrpSp->Parameters.FileSystemControl.FsControlCode); ASSERT(0 != IrpSp->FileObject->FsContext2); PDEVICE_OBJECT FsvolDeviceObject = IrpSp->FileObject->FsContext2; FSP_FSVOL_DEVICE_EXTENSION *FsvolDeviceExtension = FspFsvolDeviceExtension(FsvolDeviceObject); - FspIoqStop(FsvolDeviceExtension->Ioq); + /* Fix GitHub issue #369 + * + * The original WinFsp protocol for shutting down a file system was to issue + * an FSP_FSCTL_STOP control code to the fsctl device. This would set the IOQ + * to the "stopped" state and would also cancel all active IRP's. Cancelation + * of IRP's would sometimes free buffers that may have still been in use by + * the user mode file system threads; hence access violation. + * + * To fix this problem a new control code FSP_FSCTL_STOP0 is introduced. The + * new file system shutdown protocol is backwards compatible with the original + * one and works as follows: + * + * - First the file system process issues an FSP_FSCTL_STOP0 control code which + * sets the IOQ to the "stopped" state but does NOT cancel IRP's. + * + * - Then the file system process waits for its dispatcher threads to complete + * (see FspFileSystemStopDispatcher). + * + * - Finally the file system process issues an FSP_FSCTL_STOP control code + * which stops the (already stopped) IOQ and cancels all IRP's. + */ + FspIoqStop(FsvolDeviceExtension->Ioq, + FSP_FSCTL_STOP == IrpSp->Parameters.FileSystemControl.FsControlCode); return STATUS_SUCCESS; }