Viostor: Fix a potential memory corruption caused by writing SRB result

[MartinDrab]

When passing SRB to the host, such as read or write request, the buffers to be read/written are passed in form of so-called scatter-gather lists (SG lists), containing physical addresses and lengths of the buffers. Initial SG lista are passed to viostor (and vioscsi as well) by Storport as part of the SRB. Viostor/vioscsi enlarges each SG list by two small buffers:

• one to specify the command for the host,
• one to receive the result.

However, if a SRB is completed as part of a bus reset request to the driver, the memory to rceive SRB's result no longer belongs to the SRB (since the SRB was completed). The host, however, does not know that, thus, when it completes its work, it writes the result to a memory location no longer owned by the driver.

This commit is a remedy for such a memory corruption. Memory for receiving results for SRB commands is now part of the adapter extension. It seems, that memory allocations during request processing (i.e. BuildIo callback) are not possible for miniports servicing boot disks, thus, the space reserved for receiving SRB results is limited. This means, there is an upper limit on number of SRBs being processed by the driver at once.

[YanVugenfirer]

Can one of the admins verify this patch?

[MartinDrab]

Can one of the admins verify this patch?

Hello,

it is still a draft. I rebased the commit from our release branch which means applying some recent changes including the clang formatting. So, I would like to test I did not break anything by the rebase. Of course, I would be happy if someone looks at my changes since my solutions has a drawback of limiting number of SRBs being processed by the driver at once.

Similar issue is present also in vioscsi and I plan to port my changes to that driver as well.

[kostyanf14]

@MartinDrab

Just to explain "Can one of the admins verify this patch?" comment.

According to the new Red Hat policy, we are stopping running CI by default for PRs from
non-Red Hat contributors.

What does this mean? When you send PR, the CI will not be triggered. The comment
“Can one of the admins verify this patch?” will be added to the PR that means CI
found this pull request. Only after someone from the Red Hat members (admins) reviews
this PR and allows CI to run on it, Jenkins will build the source and run HCK-CI.

[kostyanf14]

test this please

[MartinDrab]

@MartinDrab

Just to explain "Can one of the admins verify this patch?" comment.

According to the new Red Hat policy, we are stopping running CI by default for PRs from non-Red Hat contributors.

What does this mean? When you send PR, the CI will not be triggered. The comment “Can one of the admins verify this patch?” will be added to the PR that means CI found this pull request. Only after someone from the Red Hat members (admins) reviews this PR and allows CI to run on it, Jenkins will build the source and run HCK-CI.

Ah, I see, thank you for the explanation.

[benyamin-codez]

Thanks for sharing your patch, Martin.
It seems to be an interesting approach.

Just so we are on the same page, can you clarify / specify the following?

You mentioned:

Viostor/vioscsi enlarges each SG list by two small buffers:

• one to specify the command for the host,
• one to receive the result.

Can you please share what we call these in viostor and vioscsi, or otherwise give some line references..? A concern has been raised with me internally that the driver may not currently be StorPort compliant due to some of our historical manipulations of the SG list. There is presently some interest amongst my team to assess whether your patch might address this concern.

It seems, that memory allocations during request processing...

It is our understanding that all StorPort memory allocations should only be requested following initialisation and before HwStorFindAdapter completes. In any case, we did not observe you requesting an allocation, only physical addresses. We could request a larger SRB_EXTENSION during DriverEntry() by increasing the size of a relevant struct member(s)...

...there is an upper limit on number of SRBs being processed by the driver at once.

Have you been able to determine what the upper bound is, whether mathematically or in testing?

Have you undertaken any comparative performance testing with and without this patch?
We are quite curious to see what change there might be in performance.

We did see value in refactoring the several disparate SRB routines into _SendSRB().
However, I thought it prudent to note that rather than using both MessageId or QueueNumber as parameters, we typically limit use to only one of these, and then use the MESSAGE_TO_QUEUE() or QUEUE_TO_MESSAGE() macro inside the function to convert to the other where necessary (usually this is whichever one is the exception or least used).

For example, instead of:

VioStorVQLock(DeviceExtension, MessageId, &LockHandle, FALSE);
...
VioStorVQUnlock(DeviceExtension, MessageId, &LockHandle, FALSE);

...you might use:

VioStorVQLock(DeviceExtension, QUEUE_TO_MESSAGE(QueueNumber), &LockHandle, FALSE);
...
VioStorVQUnlock(DeviceExtension, QUEUE_TO_MESSAGE(QueueNumber), &LockHandle, FALSE);

... and call _SendSRB() using QueueNumber but not MessageId...

Can you please confirm the issue definitely exists in vioscsi...?
We had a theory as to an alternate cause for this in viostor...
...but this is unlikely to be valid if this issue also exists in vioscsi.

Also, does this PR also attempt to fix the issue that PR #1295 was supposed to fix?
I note Yan Wang's comment in issue #907 that PR #1295 had not solved the problem.

[MartinDrab]

Hello,

Can you please share what we call these in viostor and vioscsi, or otherwise give some line references..?

Look into VirtIoBuildIo for example. You can see how scatter-gather lists are copied from the SRB into the SRB extension. And just before the final return, there are these lines adding two extra elements (one to the beginning, one to the end):

   srbExt->sg[0].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.out_hdr, &dummy);
   srbExt->sg[0].length = sizeof(srbExt->vbr.out_hdr);

   srbExt->sg[sgElement].physAddr = StorPortGetPhysicalAddress(DeviceExtension, NULL, &srbExt->vbr.status, &dummy);
   srbExt->sg[sgElement].length = sizeof(srbExt->vbr.status);

As far as I know, this does not break compatibility with the StorPort, since elements of the original SG list are not modified, they are just passed as part of another list.

NOTE: I looked into the virtio-blk.c code in QEMU and I think that it is also necessary to also move the element with the command code to my STATUS_TABLE as well. Because if the driver completes the SRB as part of a bus reset, the physical address holding the command value gets "freed" too (meaning it is no longer owned by the driver) -- and if the request is still stored in the queue and not read by the host, the command value may get overwritten by anything.

I have a patch ready for this as well and will add it into this PR shortly.

Have you been able to determine what the upper bound is, whether mathematically or in testing?
Not precisely, although, I read somewhere at OSR that the limit is 254 requests on the upper code levels (e.g. the disk.sys driver). Well, I set the size of the hash table to three times of that. But basically, any number would do as long as it is a prime. The bad thing is it cannot be increased on-demand due to the allocation issue (although it probably can be read from the registry and the table allocated before the FindAdapter callback returns).

Have you undertaken any comparative performance testing with and without this patch?
We are quite curious to see what change there might be in performance
Not yet. Our primary concern is to resolve issues we ran into when using viostor/vioscsi with slow disk storage on the host. There may be a performance hit due to usage of the hash table; this is definitely slower than just storing necessary values in the SRB extension, however, I do not know whether these extra instructions make any difference in reality.

Can you please confirm the issue definitely exists in vioscsi
I think vioscsi is vulnerable as well since it:

• also adds that extra two elements into the SG list (to pass the command and result value),
• it may complete pending requests without informing the host (although that can be turned off through the registry).
  Also, if the bus reset is communicated to the driver through the BusReset callback (not through the special SRB), the driver just requests bus reset from the host and does not complete pending SRB. I uess, the host does the completion part on its side. This is good.

Basically, the main problem here is this:

1. the driver passes physical memory ranges to the host,
2. the driver may complete pending SRBs as reaction to bus reset without informing the host,
3. the host may write to that physical memory ranges although they are no longer owned by the driver.

Remove (2) and things should get much better. It can be done for vioscsi via a registry change and I also proposed this possibility for viostor (#1305) as well.

My PRs only deal with potential memory corruption. The right way to fix the issue (on my opinion) would be to:

• never complete pending requests on driver side (maybe only when a surprise remove or something like that is in progress),
• when bus reset is requested, just pass the request to the host which must complete all pending requests on its side (driver gets notified about that for every such request via its interrupt routine),
• when the driver passes a request to the host, the driver ALWAYS gets informed about that request's fate (which I hope is true as of now).

AFAIK, there is currently no way for viostor to signal bus reset to the host, so a modification on host side is required (or I am missing something).

[MartinDrab]

OK, I moved also the physical memory used to transfer command header (blk_outhdr) to the Status table (adapter extension rather than the SRB extension) (commit e35ba95). So, the memory corruption should be resolved.

[kostyanf14]

test this please

[vrozenfe]

@MartinDrab
Hi Martin. Can we please keep this series on hold? I just trying to add iommu/dma v3 support at the moment. And it we need to change the DMA buffers allocation for that. The work is not done yet, but I can post some rough patch for review and comments,
And also try can keep our works in sync.

All the best,
Vadim.

[wangyan0507]

Also, does this PR also attempt to fix the issue that PR #1295 was supposed to fix? I note Yan Wang's comment in issue #907 that PR #1295 had not solved the problem.

@benyamin-codez @MartinDrab
The current PR doesn't fix the issue #907 which cause by the driver reclaimed the memory for indirect descriptor table after the bus reset process.

Thanks,
Yan Wang.

[benyamin-codez]

@MartinDrab @vrozenfe @wangyan0507

Thank you for your posts.

Martin, I just wanted to say we are on the same page regarding the wrapping of the StorPort provided SG list, sgList, with either request and response (in vioscsi) or the header and status (in viostor) to produce our VIO_SG list.

Vadim, I agree it would indeed be helpful if you could share some of your IOMMU / DMAv3 work for the reasons you mentioned. There appears to be a few issues related to SG list mechanics at present, and I'm sure sharing your work would assist anyone scoping assessment or development work in this space.

Obviously we have a problem if StorPort destroys the SRB artefacts while we are still relying on data stored in the extension, including in the SG list. Having said that, if the SRB has been completed, then we shouldn't have any need for the SG list, whether this is the StorPort generated addresses at sgList or the VIO_SG in whichever extension we use (vioscsi creates a pointer with POINTER_ALIGN to produce srbExt->psgl, whereas viostor just uses the SG directly as srbExt->sg), so the SG list mechanics may not be the best place to resolve this issue anyway. I would think the same would also apply to any reliance on the data in an indirect descriptor table.

So absent a solution in the SG list mechanics, if we were to focus on the SRB completion mechanics during reset instead, there are a couple of perhaps prudent observations:

Martin mentioned the BusReset callback [HW_RESET_BUS]. Here we observe some differences:

// viostor
BOOLEAN
VirtIoResetBus(IN PVOID DeviceExtension, IN ULONG PathId)
{
    UNREFERENCED_PARAMETER(PathId);
    PADAPTER_EXTENSION adaptExt; // <-- Orphan...?
    adaptExt = (PADAPTER_EXTENSION)DeviceExtension; // <-- Orphan...?

    CompletePendingRequests(DeviceExtension);
    return TRUE;

// vioscsi
BOOLEAN
VioScsiResetBus(IN PVOID DeviceExtension, IN ULONG PathId)
{
    UNREFERENCED_PARAMETER(PathId);

    // CompletePendingRequestsOnReset(DeviceExtension); // <-- This line is perhaps missing
    return DeviceReset(DeviceExtension);
}

Here, I suggest that vioscsi should also call CompletePendingRequestsOnReset() before returning with the status of the DeviceReset() call. Should we perhaps also implement a DeviceReset() function suitable for use with viostor...? Presently we just return TRUE... If we were so inclined, we could also make use of PathId to target our resets...

My other observation, which I have mentioned elsewhere, is something common to both vioscsi and viostor, namely that after successfully completing pending requests, the SRB status is posthumously updated with SRB_SET_SRB_STATUS(Srb, SRB_STATUS_SUCCESS), which seems odd to me, as the completion notification has already been sent, but perhaps these are necessary mechanics..!?!?

I'm a couple of days away from running up my gdb lab to test reset mechanics. My ability to use the lab will also be diminished due to other commitments, so it may be a week (or more) before I can share definitive results.
Let me know if any of you beat me to it though..! 8^d

Best regards,
Ben

P.S.: Just a further note wrt the mechanics of the BusReset callback [HW_RESET_BUS], we should probably also determine whether or not we should honour any adaptExt->action_on_reset (as specified in the registry).

[MartinDrab]

Here, I suggest that vioscsi should also call CompletePendingRequestsOnReset() before returning with the status of the DeviceReset() call.

Well, I do not think this is the right solution (although the documentation states that you should complete pending SRBs on bus reset). Let's say, for example, a bus reset callback is invoked and there are 5 pending SRBs, so we complete them. Meanwhile, some of them may also get completed by the host but this information does not reach Storport (it gets informed about SRB_STATUS_BUS_RESET instead) which AFAIK menas SortPort resends the same request again (but the documentation is not entirely clear about that outside the StartIo callback). So, even if all of my memory corruption requests are applied, it may happen operations are esxecuted multiple times by the host.

Should we perhaps also implement a DeviceReset() function

Yes but AFAIK QEMU's code related to viostor (virtio-blk.c) does not epect such a request (unlike vioscsi), thus, I am afraid we cannot achieve this only from the driver side. It would be great to have such mechanism since then the following scenario might be possible:

• a bus reset arrives,
• the driver sends a bus-reset request to the host (without completing pending SRBs),
• the host completes pending SRBs and let's the driver know through na interrupt,
• the driver completes its SRBs in a nice and orderly manner.

(at least I hope such a scenario would be possible).

something common to both vioscsi and viostor, namely that after successfully completing pending requests, the SRB status is posthumously updated with SRB_SET_SRB_STATUS(Srb, SRB_STATUS_SUCCESS), which seems odd to me, as the completion notification has already been sent, but perhaps these are necessary mechanics..!?!?

Such behavior is an error if it happens after StorPortNotification(...RequestComplete). Although I thought this is no longer the case after #1295 and #1295. But I may be mistaken of course.

P.S.: Just a further note wrt the mechanics of the BusReset callback [HW_RESET_BUS], we should probably also determine whether or not we should honour any adaptExt->action_on_reset (as specified in the registry).

I am slowly starting to believe that someone run into the same/similar issues in the past and action_on_reset was a quick solution for him/her.

The current PR doesn't fix the issue #907

I agree. This PR is not a reaction/solution to #907.

[benyamin-codez]

Thanks for the follow up, Martin.

...something common to both vioscsi and viostor, namely that after successfully completing pending requests, the SRB status is posthumously updated with SRB_SET_SRB_STATUS(Srb, SRB_STATUS_SUCCESS), which seems odd to me, as the completion notification has already been sent, but perhaps these are necessary mechanics..!?!?

Such behavior is an error if it happens after StorPortNotification(...RequestComplete). Although I thought this is no longer the case after PR #1295 and [#1296]. But I may be mistaken of course.

So I think I missed the mark with this one. This is just setting the SRB status of the SRB that called for the reset, not the pending SRBs - unless it also completes itself... 8^O That would probably be bad, yes...? It's probably worth a check via trace, or failing that, in the debugger...

Here, I suggest that vioscsi should also call CompletePendingRequestsOnReset() before returning with the status of the DeviceReset() call.

Well, I do not think this is the right solution (although the documentation states that you should complete pending SRBs on bus reset ...

In vioscsi we have the adaptExt->action_on_reset action VioscsiResetCompleteRequests (the default) where we call CompletePendingRequestsOnReset() to process all SRBs in all virtqueues. The last step (before decrementing the counter and returning) is a call to CompleteRequest() where all paths lead to StorPortNotification(RequestComplete, ...) before returning.

This all occurs under StorPortPause (for 10 seconds) and a spinlock, i.e. each SRB is set to SRB_STATUS_BUS_RESET and StorPort is notified, and once this occurs for all pending SRBs, the spinlock is lifted and StorPort is resumed.

It was my understanding that StorPort would then retry the transfer, but perhaps this is the host (the requesting process) instead. Regardless, at some point StorPort may report an exception to the host and the host may retry the request anyway.

There could potentially be a problem if we timeout (exceed 10s) and StorPort resumes even though we are still holding a spinlock. Although it probably would be a rare event to ever exceed the timeout, we should probably test without the StorPortPause() timeout value (and / or with one that exceeds the global or miniport I/O timeout values) and see what happens if we effectively make StorPort pause indefinitely.

@wangyan0507, during pending request completion viostor also has a 10s StorPortPause() timeout. I note the 20s delay in your latest reproducer for issue #907 and your global timeout value of 6s, and that there might be a race condition introduced with your test. Just something to consider - it might be perfectly ok though (noting the default global timeout is 60s).

The current PR doesn't fix the issue #907

I agree. This PR is not a reaction/solution to #907.

Yes, I was just highlighting they seem to share a common factor, i.e. SRB operations under / following reset...

I am slowly starting to believe that someone run into the same/similar issues in the past and action_on_reset was a quick solution for him/her.

The solution does seem to have a bit of a work-around vibe, especially with the bugcheck in the mix too. It is possible our pending request completion mechanics might not be using the correct method... We should probably research this.

I think it worth suggesting that part of the problem might also be that we aren't considering the multi-tier reset mechanics present in StorPort. Presently we just:

case SRB_FUNCTION_RESET_BUS:
case SRB_FUNCTION_RESET_DEVICE:
case SRB_FUNCTION_RESET_LOGICAL_UNIT:
{
    // none or some reset action
    return true;
}

So, e.g. when StorPort requests a bus or device reset, having a LUN reset occur instead, might result in undefined behaviour, and this might be the case for other combinations also. This could be due to a legacy inheritance from the SCSI Port driver, as we see in DeviceReset() that we set cmd->req.tmf.subtype = VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET.

Anyway, if we keep poking around, I'm sure we'll work it out.

Should we perhaps also implement a DeviceReset() function suitable for use with viostor...?

Yes but AFAIK QEMU's code related to viostor (virtio-blk.c) does not expect such a request...

Yes, we might need to consider this one further...
@vrozenfe, do you know how this might be achieved using existing interfaces...?

Best regards,
Ben

[vrozenfe]

IOMMU/DMA v3 support for vioscsi - WIP
https://github.com/vrozenfe/kvm-guest-drivers-windows/tree/iommu
QEMU command line:

SER='-serial tcp:127.0.0.1:4445'
VGA='-vga std'
FLAGS=',hv_stimer,hv_synic,hv_vpindex,hv_relaxed,hv_spinlocks=0xfff,hv_vapic,hv_time,hv_frequencies,hv_runtime,hv_tlbflush,hv_reenlightenment,hv_stimer_direct,hv_ipi,+kvm_pv_unhalt'
DSK='/home/vrozenfe/work/images/data1.qcow2'
sudo $QEMU
-cpu host$FLAGS
-m 2G -smp 4,maxcpus=4,cores=4,threads=1,sockets=1
-usb -device usb-tablet,id=tablet0
-device e1000,mac=42:83:66:33:22:18,id=idJVpmsF,netdev=id23ZUK6
-netdev tap,id=id23ZUK6,vhost=on,script=/etc/qemu-ifup
-boot c
-uuid 5b959bc1-e44a-4419-97b4-da6fe8fb7062
-rtc driftfix=slew
-global kvm-pit.lost_tick_policy=discard
-monitor stdio
-name IOMMU-VIOSCSI
$VGA
$SER
-enable-kvm
-device virtio-blk-pci,id=image1,drive=drive_image1,bootindex=0
-drive id=drive_image1,if=none,snapshot=off,aio=threads,cache=none,format=qcow2,file=$IMG
-machine q35,kernel-irqchip=split
-device intel-iommu,intremap=on,aw-bits=48,caching-mode=off,device-iotlb=on,eim=on
-drive file=$DSK,if=none,format=qcow2,id=virtio_scsi_0002_02
-device virtio-scsi-pci,id=scsi1,iommu_platform=on,ats=on,disable-legacy=on,disable-modern=off
-device scsi-hd,drive=virtio_scsi_0002_02,serial=01scsi0003,bootindex=-1,rotation_rate=0 \

can survive both configurations
HKR, "Parameters", DmaRemappingCompatible,0x00010001,0
,and
HKR, "Parameters", DmaRemappingCompatible,0x00010001,1

Doesn't work on WS2016 and pre 1803 Win10s.
Probably doesn’t work for non-indirect configurations.
I haven't tried it as a system disk yet (install/dump/hibernate modes). I guess IOMMU doesn't work in those modes, and we need to fall back to "normal" allocations.

Regarding the viostor reset – I was experimenting with that in the past for my own research. In my opinion, it can be a challenging task. If we’re talking about QEMU only, then resetting Virtio queues might be enough. But in the case of FPGA IP-backed Virtio, we would probably need a complete HBA reset (and IIRC, we don’t have an interface for that in QEMU or the Virtio-block spec). However, if we're speaking about QEMU configurations, it's important to note that Virtio-blk and Virtio-scsi are two different devices. If you need a SCSI-like device, use virtio-scsi, while virtio-blk historically was a solution for a local image on host-attached storage. You shouldn’t expect long delays (like 30 or 60 seconds) for such a configuration. Honestly, I’m still unsure if we need the bus reset logic for the virtio-blk driver, but I definitely don’t know all the use cases for which the virtio-blk device is used.

In any case, please keep in mind that virtio-blk and virtio-scsi are distinct devices, and I personally see no reason to copy the same code or features from one driver to the other without considering the actual use cases.

All the best,
Vadim

BTW, do you think it would be useful to set up an NG or chat room (like on Slack or something similar)?

[benyamin-codez]

Thanks, Vadim.
That is helpful.
I've already got a few notes for future contributions...!
q8^{d>~~

BTW, do you think it would be useful to set up an NG or chat room (like on Slack or something similar)?

I think at the very least we should use a discussion, but I'm open to alternatives.

A discussion for reset behaviour might be in order too.
We can perhaps then glean the highlights into a wiki entry - maybe as part of an architecture series...

Re virtio-blk, it looks like a relevant patch was posted to the kernel late last year as part of series focused on improvements to FLR handling. I see Stefan is aware of it so we might see that downstream at some point.

Have we tried any alternatives such as after completing pending requests, then calling VirtIoHwReinitialize()..?

IIRC, a HBA reset starts by writing a zero to the status register, before acking and asserting the suitable driver.
Perhaps we can force this to be triggered...

...while virtio-blk historically was a solution for a local image on host-attached storage.

With new feature sets and active development, e.g. IOThread VQ Mapping, and the existing bug fixes in the pipeline here, that could very well change...

In any case, please keep in mind that virtio-blk and virtio-scsi are distinct devices, and I personally see no reason to copy the same code or features from one driver to the other without considering the actual use cases.

I think the "without considering the actual use cases" part is really important here. Having said that, and given that both drivers are now Storport miniport drivers, a carefully managed consolidation of the code base between the two drivers would certainly make their differences more conspicuous and meaningful.

This should not only improve RAS, TCO and manageability, but it has flow on effects too, e.g. writing automated QE code becomes more economical and manageable, amongst other advantages. It would likely also assist with migrating to other storage-based VirtIO devices, e.g. virtio-nvme or virtio-crystal5d, notably if they are StorPort based... ;^D

Best Regards,
Ben

[wangyan0507]

@wangyan0507, during pending request completion viostor also has a 10s StorPortPause() timeout. I note the 20s delay in your latest reproducer for issue #907 and your global timeout value of 6s, and that there might be a race condition introduced with your test. Just something to consider - it might be perfectly ok though (noting the default global timeout is 60s).

@benyamin-codez If I set the global timeout value to 60s, and the sleep time bigger than 60s(e.g. 100s), it will also cause this issue.

Best Regards,
Yan Wang