Re: [Qemu-devel] [Qemu-block] Guest unresponsive after Virtqueue size exceeded error

[Natanael Copa]

On Fri, 22 Feb 2019 14:04:20 +0000
Stefan Hajnoczi <address@hidden> wrote:

> On Fri, Feb 22, 2019 at 12:57 PM Fernando Casas Schössow
> <address@hidden> wrote:
> 
> I have CCed Natanael Copa, qemu package maintainer in Alpine Linux.

Hi!

...

> Richard: Commit 7db2145a6826b14efceb8dd64bfe6ad8647072eb ("bswap: Add
> host endian unaligned access functions") introduced the unaligned
> memory access functions in question here.  Please see below for
> details on the bug - basically QEMU code assumes they are atomic, but
> that is not guaranteed :(.  Any ideas for how to fix this?
> 
> Natanael: It seems likely that the qemu package in Alpine Linux
> suffers from a compilation issue resulting in a broken QEMU.  It may
> be necessary to leave the compiler optimization flag alone in APKBUILD
> to work around this problem.
> 
> Here are the details.  QEMU relies on the compiler turning
> memcpy(&dst, &src, 2) turning into a load instruction in
> include/qemu/bswap.h:lduw_he_p() (explanation below):
> 
> /* Any compiler worth its salt will turn these memcpy into native unaligned
>    operations.  Thus we don't need to play games with packed attributes, or
>    inline byte-by-byte stores.  */
> 
> static inline int lduw_he_p(const void *ptr)
> {
>     uint16_t r;
>     memcpy(&r, ptr, sizeof(r));
>     return r;
> }
> 
> Here is the disassembly snippet of virtqueue_pop() from Fedora 29 that
> shows the load instruction:
> 
>   398166:       0f b7 42 02             movzwl 0x2(%rdx),%eax
>   39816a:       66 89 43 32             mov    %ax,0x32(%rbx)
> 
> Here is the instruction sequence in the Alpine Linux binary:
> 
>   455562:    ba 02 00 00 00           mov    $0x2,%edx
>   455567:    e8 74 24 f3 ff           callq  3879e0 <address@hidden>
>   45556c:    0f b7 44 24 42           movzwl 0x42(%rsp),%eax
>   455571:    66 41 89 47 32           mov    %ax,0x32(%r15)
> 
> It's calling memcpy instead of using a load instruction.

My first reaction to this is: If the intention is to not actually call
memcpy function, then maybe memcpy should not be used in the C code in
first place?
 
> Fernando found that QEMU's virtqueue_pop() function sees bogus values
> when loading a 16-bit guest RAM location.  Paolo figured out that the
> bogus value can be produced by memcpy() when another thread is
> updating the 16-bit memory location simultaneously.  This is a race
> condition between one thread loading the 16-bit value and another
> thread storing it (in this case a guest vcpu thread).  Sometimes
> memcpy() may load one old byte and one new byte, resulting in a bogus
> value.
> 
> The symptom that Fernando experienced is a "Virtqueue size exceeded"
> error message from QEMU and then the virtio-blk or virtio-scsi device
> stops working.  This issue potentially affects other device emulation
> code in QEMU as well, not just virtio devices.
> 
> For the time being, I suggest tweaking the APKBUILD so the memcpy() is
> not generated.  Hopefully QEMU can make the code more portable in the
> future so the compiler always does the expected thing, but this may
> not be easily possible.

I suspect this happens due to the Alpine toolchain will enable
_FORTIFY_SOURCE=2 by default and the way this is implemented via 
fortify-headers:
http://git.2f30.org/fortify-headers/file/include/string.h.html#l39

Try build with -U_FORTIFY_SOURCE

> 
> Stefan