Re: [Qemu-devel] [Bug 1191606] Re: qemu crashes with iscsi initiator (libiscsi) when using virtio

[ronnie sahlberg]

On Thu, Jun 20, 2013 at 7:47 AM, Laszlo Ersek <address@hidden> wrote:
> On 06/20/13 15:33, ronnie sahlberg wrote:
>> http://pastebin.com/EuwZPna1
>>
>> Last few thousand lines from the log with your patch.
>>
>>
>> The crash happens immediately after qemu has called out to iscsi_ioctl
>> with SG_IO to read the serial numbers vpd page.
>> We get the reply back from the target but as soon as ioctl_cb returns we 
>> crash.
>> If you comment out SG_IO in iscsi_ioctl then the crash does not happen
>> (but the qemu does nto get serial number either)
>>
>>
>> I will look more into it tonight.
>
>   virtqueue_map_sg: mapped gpa=00000000790a9000 at hva=0x7f0cb10a9000 for 
> length=4, is_write=1  (out: data)
>   virtqueue_map_sg: mapped gpa=000000007726fc70 at hva=0x7f0caf26fc70 for 
> length=96, is_write=1 (out: sense)
>   virtqueue_map_sg: mapped gpa=00000000764e5aa0 at hva=0x7f0cae4e5aa0 for 
> length=16, is_write=1 (out: errors, data_len, sense_len, residual)
>   virtqueue_map_sg: mapped gpa=00000000764e5adc at hva=0x7f0cae4e5adc for 
> length=1, is_write=1  (out: status)
>   virtqueue_map_sg: mapped gpa=00000000764e5a90 at hva=0x7f0cae4e5a90 for 
> length=16, is_write=0 (in: type, ioprio, sector)
>   virtqueue_map_sg: mapped gpa=000000007ab80578 at hva=0x7f0cb2b80578 for 
> length=6, is_write=0  (in: cmd)
>   virtio_blk_handle_request: type=0x00000002
>   virtqueue_fill: unmapping hva=0x7f0c24008000 for length=4, access_len=1, 
> is_write=1
>   Bad ram pointer 0x7f0c24008000
>
> This looks related, in virtio_blk_handle_scsi():
>
>     } else if (req->elem.in_num > 3) {
>         /*
>          * If we have more than 3 input segments the guest wants to actually
>          * read data.
>          */
>         hdr.dxfer_direction = SG_DXFER_FROM_DEV;
>         hdr.iovec_count = req->elem.in_num - 3;
>         for (i = 0; i < hdr.iovec_count; i++)
>             hdr.dxfer_len += req->elem.in_sg[i].iov_len;
>
>         hdr.dxferp = req->elem.in_sg;
>     } else {
>
> This sets
> - "hdr.iovec_count" to 1,
> - "hdr.dxfer_len" to 4,
> - "hdr.dxferp" as shown above,
>
> For "struct sg_io_hdr" (which is the type of "hdr"), the typedef &
> documentation are in <include/scsi/sg.h>:
>
>     unsigned short iovec_count; /* [i] 0 implies no scatter gather */
>
>     void __user *dxferp;        /* [i], [*io] points to data transfer memory
>                                               or scatter gather list */
>
> Now what we're seeing is a corruption of "req->elem.in_sg[0].iov_base",
> whose address equals that of "req->elem.in_sg" (it's at offset 0 in the
> struct at subscript #0 in the array).
>
>   virtqueue_map_sg: mapped gpa=00000000790a9000 at hva=0x7f0cb10a9000 for 
> length=4, is_write=1
>   [...]
>   virtio_blk_handle_request: type=0x00000002
>   virtqueue_fill: unmapping hva=0x7f0c24008000 for length=4, access_len=1, 
> is_write=1
>   Bad ram pointer 0x7f0c24008000
>
> First I don't understand how access_len can only be "1". But, in any
> case, if the "req->elem.in_sg[0].iov_base" pointer is stored in
> little-endian order, and the kernel (or iscsi_scsi_command_async()?) for
> whatever reason misinterprets "hdr.dxferp" to point at an actual receive
> buffer (instead of an iovec array), that would be consistent with the
> symptoms:

Ah, that makes sense.

block.iscsi.c   (https://github.com/qemu/qemu/blob/master/block/iscsi.c)
does assume that ioh->dxferp is a pointer to the buffer and that there
is no scatter gather.
See lines  745-749.

I did not know that ioctl() could take a scatter/gather list.


I cant test now  but if I understand right then
lines 745-749 should be replaced with something that does

* check ioh->iovec_count IF if it zero then there is no scatter gather
and ioh->dxferp points to a buffer,  so just do what we do today.
* IF iovec_count is > 0  then dxferp is NOT a pointer to a buffer but
a pointer to an array of iovec then
traverse the iovec array and add these as buffers to the task just
like we do for readv. For example similar to the loop to add the
iovecs in lines 449-453


I will try this tonight.


>
>   0x7f0cb10a9000 <--- original value of req->elem.in_sg[0].iov_base
>   0x7f0c24008000 <--- corrupted value
>         ^^^^^^^^ <--- 4 low bytes overwritten by SCSI data
>
> Laszl