search

winfsp / winspd

Windows Storage Proxy Driver - User mode disk storage
https://winfsp.dev
Other
411 stars 52 forks source link

Fix max block count issue #5

Closed CzBiX closed 4 years ago

CzBiX commented 4 years ago

Fix issue when storage greater than 2TB(block size 512).

billziss-gh commented 4 years ago

Thank you for the PR. I am hoping to have a look at it in the coming weekend.

billziss-gh commented 4 years ago

I have updated WinSpd so that it can build with recent AppVeyor images. With these changes your PR was built and has passed all tests on AppVeyor.

I will review the PR later today.

CzBiX commented 4 years ago

When we return 0xFFFFFFFF as LBA for SCSIOP_READ_CAPACITY, system will issue another SCSIOP_READ_CAPACITY16 request to device.(see NOTE bellow) But the buffer size in request will be sizeof(READ_CAPACITY_DATA_EX), which is smaller than sizeof(READ_CAPACITY16_DATA). If we assert that the data structure is READ_CAPACITY16_DATA, and return SRB_STATUS_DATA_OVERRUN with SrbSetDataTransferLength call, system will think it's a error and will NOT retry with greater buffer size.

We have to support READ_CAPACITY_DATA_EX. If the buffer size smaller than expected(READ_CAPACITY_DATA or READ_CAPACITY_DATA_EX), that must be a SRB_STATUS_INVALID_REQUEST.

Hope I answered all your questions.

NOTE: I didn't change this since it works at now https://github.com/billziss-gh/winspd/blob/d5a23f0da24a345b9de323dd4ed63f7caec1c939/src/sys/scsi.c#L108

billziss-gh commented 4 years ago

When we return 0xFFFFFFFF as LBA for SCSIOP_READ_CAPACITY, system will issue another SCSIOP_READ_CAPACITY16 request to device.(see NOTE bellow) But the buffer size in request will be sizeof(READ_CAPACITY_DATA_EX), which is smaller than sizeof(READ_CAPACITY16_DATA). If we assert that the data structure is READ_CAPACITY16_DATA, and return SRB_STATUS_DATA_OVERRUN with SrbSetDataTransferLength call, system will think it's a error and will NOT retry with greater buffer size.

We have to support READ_CAPACITY_DATA_EX. If the buffer size smaller than expected(READ_CAPACITY_DATA or READ_CAPACITY_DATA_EX), that must be a SRB_STATUS_INVALID_REQUEST.

Thank you for your explanation of how this fix works. So if I understand correctly what happens is this:

I would very much like to reproduce this problem, so it would be very helpful if you let me know:

(Please note that in my tests on Windows 10, disks greater than 2T appear to work even without your patch. So perhaps I am testing this on the wrong OS or I am not setting up things properly to reproduce.)

Patch 1

I include below a minimal version of your patch. I believe the essence of your patch is that you handle both READ_CAPACITY_DATA_EX and READ_CAPACITY16_DATA and this is what the patch below attempts to capture. (Please note that this patch applies against the current master branch.)

diff --git a/src/sys/scsi.c b/src/sys/scsi.c
index 2c78e59..db50efa 100644
--- a/src/sys/scsi.c
+++ b/src/sys/scsi.c
@@ -505,10 +505,10 @@ static UCHAR SpdScsiReadCapacity(PVOID DeviceExtension, SPD_STORAGE_UNIT *Storag
     else
     {
         /* READ CAPACITY (16) */
-        if (sizeof(READ_CAPACITY16_DATA) > DataTransferLength)
+        if (sizeof(READ_CAPACITY_DATA_EX) > DataTransferLength)
             return SRB_STATUS_DATA_OVERRUN;

-        PREAD_CAPACITY16_DATA ReadCapacityData = DataBuffer;
+        PREAD_CAPACITY_DATA_EX ReadCapacityData = DataBuffer;
         UINT64 U64;
         UINT32 U32;
         U64 = StorageUnit->StorageUnitParams.BlockCount - 1;
@@ -525,10 +525,18 @@ static UCHAR SpdScsiReadCapacity(PVOID DeviceExtension, SPD_STORAGE_UNIT *Storag
         ((PUINT8)&ReadCapacityData->BytesPerBlock)[1] = (U32 >> 16) & 0xff;
         ((PUINT8)&ReadCapacityData->BytesPerBlock)[2] = (U32 >> 8) & 0xff;
         ((PUINT8)&ReadCapacityData->BytesPerBlock)[3] = U32 & 0xff;
-        if (StorageUnit->StorageUnitParams.UnmapSupported)
-            ReadCapacityData->LBPME = 1;

-        SrbSetDataTransferLength(Srb, sizeof(READ_CAPACITY16_DATA));
+        ULONG DataLength;
+        if (sizeof(READ_CAPACITY16_DATA) <= DataTransferLength)
+        {
+            if (StorageUnit->StorageUnitParams.UnmapSupported)
+                ((PREAD_CAPACITY16_DATA)ReadCapacityData)->LBPME = 1;
+            DataLength = sizeof(READ_CAPACITY16_DATA);
+        }
+        else
+            DataLength = sizeof(READ_CAPACITY_DATA_EX);
+
+        SrbSetDataTransferLength(Srb, DataLength);

         return SRB_STATUS_SUCCESS;
     }

In this version of your patch I have chosen to continue returning SRB_STATUS_DATA_OVERRUN rather than SRB_STATUS_INVALID_REQUEST. Unless I continue to misunderstand what is happening: it does not seem to be necessary to change the error code that we return when we receive a short buffer; what is important is that we handle both READ_CAPACITY_DATA_EX and READ_CAPACITY16_DATA.

Patch 2

OTOH, if returning SRB_STATUS_INVALID_REQUEST causes the system to always send us a READ_CAPACITY16_DATA buffer, perhaps the patch can be simplified further. This assumes that the system behaves as follows:

diff --git a/src/sys/scsi.c b/src/sys/scsi.c
index 2c78e59..f0361a7 100644
--- a/src/sys/scsi.c
+++ b/src/sys/scsi.c
@@ -506,7 +506,7 @@ static UCHAR SpdScsiReadCapacity(PVOID DeviceExtension, SPD_STORAGE_UNIT *Storag
     {
         /* READ CAPACITY (16) */
         if (sizeof(READ_CAPACITY16_DATA) > DataTransferLength)
-            return SRB_STATUS_DATA_OVERRUN;
+            return SRB_STATUS_INVALID_REQUEST;

         PREAD_CAPACITY16_DATA ReadCapacityData = DataBuffer;
         UINT64 U64;

Please let me know your thoughts. Thank you for your patience.

CzBiX commented 4 years ago

Windows version is 10.0.19041.508. The failed test code looks like this:

BlockLength = 512;
BlockCount = 0x100000000; //0xFFFFFFFF will works

Note: "failed" doesn't meaning failed to create device, it's the system failed to get capacity and will not read any block.

Let me do more tests.

CzBiX commented 4 years ago

In my test, neither SRB_STATUS_DATA_OVERRUN nor SRB_STATUS_INVALID_REQUEST will work. So we must support READ_CAPACITY_DATA_EX. I'll update PR to match your patch.

billziss-gh commented 4 years ago

I have been able to repro the problem and confirm that the PR fixes it. I have merged your PR into the master branch.

Thank you very much for investigating this problem and producing a patch for it. It is much appreciated.