Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130

[Peter Maydell]

On 30 January 2018 at 17:39, Greg Kurz <address@hidden> wrote:
> The following changes since commit 30d9fefe1aca1e92c785214aa9201fd7c2287d56:
>
>   Merge remote-tracking branch 
> 'remotes/kraxel/tags/input-20180129-v2-pull-request' into staging (2018-01-29 
> 15:52:27 +0000)
>
> are available in the git repository at:
>
>   https://github.com/gkurz/qemu.git tags/for-upstream
>
> for you to fetch changes up to 0dfef72861261bdfd30f2cc53d61c12c097af11a:
>
>   tests/virtio-9p: explicitly handle potential integer overflows (2018-01-30 
> 15:28:56 +0100)
>
> ----------------------------------------------------------------
> This series is mostly about 9p request cancellation. It fixes a
> long standing bug (read "specification violation") where the server
> would send an invalid response when the client has cancelled an
> in-flight request. This was causing annoying spurious EINTR returns
> in linux. The fix comes with some related testing in QTEST.
>
> Other patches are code cleanup and improvements.
>
> ----------------------------------------------------------------
> Greg Kurz (9):
>       9pfs: drop v9fs_register_transport()
>       tests: virtio-9p: move request tag to the test functions
>       tests: virtio-9p: wait for completion in the test code
>       tests: virtio-9p: use the synth backend
>       tests: virtio-9p: add LOPEN operation test
>       tests: virtio-9p: add WRITE operation test
>       libqos/virtio: return length written into used descriptor
>       tests: virtio-9p: add FLUSH operation test
>       tests/virtio-9p: explicitly handle potential integer overflows
>
> Keno Fischer (1):
>       9pfs: Correctly handle cancelled requests

Hi. This fails 'make check' on sparc hosts:

TEST: tests/virtio-9p-test... (pid=21410)
  /ppc64/virtio/9p/pci/nop:                                            OK
  /ppc64/virtio/9p/pci/config:                                         OK
  /ppc64/virtio/9p/pci/fs/version/basic:                               OK
  /ppc64/virtio/9p/pci/fs/attach/basic:                                OK
  /ppc64/virtio/9p/pci/fs/walk/basic:                                  OK
  /ppc64/virtio/9p/pci/fs/walk/no_slash:                               OK
  /ppc64/virtio/9p/pci/fs/walk/dotdot_from_root:                       OK
  /ppc64/virtio/9p/pci/fs/lopen/basic:                                 OK
  /ppc64/virtio/9p/pci/fs/write/basic:                                 OK
  /ppc64/virtio/9p/pci/fs/flush/success:
Broken pipe
FAIL
GTester: last random seed: R02S3152e3829aa0155ecb8ed934af62a54f
(pid=21546)
  /ppc64/virtio/9p/pci/fs/flush/ignored:
Broken pipe
FAIL
GTester: last random seed: R02Sd8d854ef25a0afc00dfe43cdab8f8bc7
(pid=21552)
FAIL: tests/virtio-9p-test

On x86 with the clang runtime sanitizer it gives errors about
misaligned loads:

/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:531:15:
runtime error: member access within misaligned address 0x7fb933502017
for type 'QtestV9fsSynthFlushData', which requires 4 byte alignment
0x7fb933502017: note: pointer points here
 04 00 00 00 0a  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^
/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:531:15:
runtime error: load of misaligned address 0x7fb933502017 for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x7fb933502017: note: pointer points here
 04 00 00 00 0a  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^
/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:532:22:
runtime error: member access within misaligned address 0x7fb933502017
for type 'QtestV9fsSynthFlushData', which requires 4 byte alignment
0x7fb933502017: note: pointer points here
 04 00 00 00 0a  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^
/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:532:22:
runtime error: load of misaligned address 0x7fb933502017 for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x7fb933502017: note: pointer points here
 04 00 00 00 0a  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^
/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:531:15:
runtime error: member access within misaligned address 0x7f33bb102017
for type 'QtestV9fsSynthFlushData', which requires 4 byte alignment
0x7f33bb102017: note: pointer points here
 04 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^
/home/petmay01/linaro/qemu-for-merges/hw/9pfs/9p-synth.c:531:15:
runtime error: load of misaligned address 0x7f33bb102017 for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x7f33bb102017: note: pointer points here
 04 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
00 00 00 00 00 00 00  00 00 00
             ^

which is probably what is causing the sparc failure.

This code looks suspicious:

static ssize_t v9fs_synth_qtest_flush_write(void *buf, int len, off_t offset,
                                            void *arg)
{
    QtestV9fsSynthFlushData *data = buf;

    assert(len == sizeof(*data));

    if (data->usec_timeout) {

as you can't in general cast an arbitrary pointer into a
structure and use it like that.

Given that the only thing in the data stream is a 32-bit value,
it would be simpler just to say
  uint32_t usec_timeout = ldl_XX_p(buf);

I put 'XX' there because the other thing that looks weird here
is the handling of endianness. The code as written is doing
a "load a host-order 32-bit value", which would be ldl_he_p().
But should a buffer in the 9pfs code really have anything
in host-order rather than a fixed-by-protocol order?
Using ldl_le_p() or ldl_be_p() seems more plausible. (Whatever
code is generating this byte stream might also need attention.)

thanks
-- PMM