megasas & scsi-disk - interesting I/O errors

Hello.

I've an interesting case here with megasas virtual device.

TL;DR see summary at the end.

Initially I were testing a backport of CVE-2017-9503 fix to
qemu 2.8.1 (which is in debian stable), and while testing I
found out that the last patch from the fix makes megasas-
attached storage device to be malfunctioning in windows10.
Copying files from it quite soon makes any I/O operation
to return "invalid parameter" error, with a lot of events
from "disk" subsystem in windows event log, like this:

Found an error at device \Device\Harddisk0\DR0 during page i/o request (translated from ru, might be not exact)
Xml события:
<Event xmlns="http://schemas.microsoft.com/win/2004/08/events/event">
  <System>
    <Provider Name="disk" />
    <EventID Qualifiers="32772">51</EventID>
    <Level>3</Level>
    <Task>0</Task>
    <Keywords>0x80000000000000</Keywords>
    <TimeCreated SystemTime="2018-05-19T16:06:59.339940000Z" />
    <EventRecordID>591</EventRecordID>
    <Channel>System</Channel>
    <Computer>DESKTOP-KEG4VT4</Computer>
    <Security />
  </System>
  <EventData>
    <Data>\Device\Harddisk0\DR0</Data>

<Binary>040080000100000000000000330004802D0100000E0000C000000000000000000000000000000000C300000000000000FFFFFFFF010000005800000800010000FD200A1282012040001000003C00000000A0838F88E4FFFFB899548F88E4FFFF0000000000000000C007298E88E4FFFF0000000000000000A8F20000000000002A080000F2A800000800000000000000000000000000000000000000000000000000000000000000</Binary>
  </EventData>

I enabled megasas tracing but weren't able to trigger the prob
when tracing is turned on. So I _guess_ it is timing-related, but
who knows.

Only the last patch in the CVE-2017-9503 patches triggers the
problem. Reverting it makes guest work just fine, with one
interesting issue: there are _still_ 2 errors logged like
that, followed by 2 warnings from ntfs telling us it can't
write journal data and the filesystem might become corrupt
(the same ntfs messages are logged with that patch applied,
too).  The patch in question is this one:

commit 87e459a810d7b1ec1638085b5a80ea3d9b43119a
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu Jun 1 17:26:14 2017 +0200

    megasas: always store SCSIRequest* into MegasasCmd

    This ensures that the request is unref'ed properly, and avoids a
    segmentation fault in the new qtest testcase that is added.
    This is CVE-2017-9503.

    Reported-by: Zhangyanyu <zyy4013@stu.ouc.edu.cn>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Now the more interesting stuff. I noticed that ubuntu also
applied the same CVE-2017-9503 fixes but on top of 2.8.0
version. So I tested their qemu and found out that it works.
So there's one change in 2.8.1 which "helps" to trigger the
error condition: it is this change:

commit 1f8af0d186abf9ef775a74d41bf2852ed8d59b63
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Tue Jan 3 18:20:28 2017 +0100

    scsi-block: fix direction of BYTCHK test for VERIFY commands

    The direction is wrong; scsi_block_is_passthrough returns
    false for commands that *can* use sglists.

    Reported-by: Zhang Qian <zhangqian@sangfor.com.cn>
    Fixes: 8fdc7839e40f43a426bc7e858cf1dbfe315a3804
    Cc: qemu-stable@nongnu.org
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Reverting this one change from 2.8.1 makes the prob to go away
(with CVE-2017-9503 fixes applied).

Now the more interesting thing: this problem is also present in current
2.12.0 version and in all versions between 2.9 and 2.12...

To sum it all up:

2.8.0 + CVE-2017-9503 fix 87e459a810d = one or two i/o error
2.8.0 + CVE-2017-9503 fix 87e459a810d + 1f8af0d186abf9e = hdd unusable
up to current 2.12 = hdd unusable but depends on timings
up to current 2.12 with tracing = one or two i/o error

There's obviously something fishy going on here. I'd love to hear some
suggestions/hints about possible ways to debug this.

BTW, 2.12 is quite "stally", so to say, in various I/O operations, to
the point where working in win10 guest becomes almost impossible -
i/o stalls for 20..120 seconds every few minutes (cpu works, I can
move windows, spinners are spinning etc, until somethin else hits
i/o too). Besides that it is also generally slower.
I'll try to bisect that later.



Also while trying to find where the prob has been introduced I found out
an interesting pair of patches in this CVE-2017-9503 series.  See this
commit 36c327a69d723571f02a7691631667cdb1865ee1
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu Jun 1 17:23:13 2017 +0200

    megasas: do not read command more than once from frame

    Avoid TOC-TOU bugs by passing the frame_cmd down, and checking
    cmd->dcmd_opcode instead of cmd->frame->header.frame_cmd.

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

around trace_megasas_handle_scsi call:

     trace_megasas_handle_scsi(mfi_frame_desc[cmd->frame->header.frame_cmd],
-                              is_logical, cmd->frame->header.target_id,
+    trace_megasas_handle_scsi(mfi_frame_desc[frame_cmd], is_logical,
+                              cmd->frame->header.target_id,
                               cmd->frame->header.lun_id, sdev, cmd->iov_size);

After applying this, we'll get:

     trace_megasas_handle_scsi(mfi_frame_desc[cmd->frame->header.frame_cmd],
     trace_megasas_handle_scsi(mfi_frame_desc[frame_cmd], is_logical,
                               cmd->frame->header.target_id,
                               cmd->frame->header.lun_id, sdev, cmd->iov_size);

which obviously wont compile.

Subsequent patch (b356807fcdfc45583c) fixes this.
But the thing is quite.. fun anyway.

Thanks!

/mjt