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

Greg Kurz posted 10 patches 6 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180130173935.5172-1-groug@kaod.org
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test s390x passed
hw/9pfs/9p-synth.c         |  56 +++++++++
hw/9pfs/9p-synth.h         |  11 ++
hw/9pfs/9p.c               |  25 +++-
hw/9pfs/9p.h               |  10 +-
hw/9pfs/trace-events       |   1 +
hw/9pfs/virtio-9p-device.c |   8 +-
hw/9pfs/xen-9p-backend.c   |   3 +-
tests/libqos/virtio.c      |  25 ++--
tests/libqos/virtio.h      |   3 +-
tests/virtio-9p-test.c     | 293 ++++++++++++++++++++++++++++++++++++++-------
tests/virtio-blk-test.c    |  24 ++--
tests/virtio-net-test.c    |   6 +-
tests/virtio-scsi-test.c   |   3 +-
13 files changed, 386 insertions(+), 82 deletions(-)
[Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130
Posted by Greg Kurz 6 years, 1 month ago
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

 hw/9pfs/9p-synth.c         |  56 +++++++++
 hw/9pfs/9p-synth.h         |  11 ++
 hw/9pfs/9p.c               |  25 +++-
 hw/9pfs/9p.h               |  10 +-
 hw/9pfs/trace-events       |   1 +
 hw/9pfs/virtio-9p-device.c |   8 +-
 hw/9pfs/xen-9p-backend.c   |   3 +-
 tests/libqos/virtio.c      |  25 ++--
 tests/libqos/virtio.h      |   3 +-
 tests/virtio-9p-test.c     | 293 ++++++++++++++++++++++++++++++++++++++-------
 tests/virtio-blk-test.c    |  24 ++--
 tests/virtio-net-test.c    |   6 +-
 tests/virtio-scsi-test.c   |   3 +-
 13 files changed, 386 insertions(+), 82 deletions(-)
-- 
2.13.6


Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130
Posted by Peter Maydell 6 years, 1 month ago
On 30 January 2018 at 17:39, Greg Kurz <groug@kaod.org> 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

Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130
Posted by Greg Kurz 6 years, 1 month ago
On Wed, 31 Jan 2018 10:14:46 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 January 2018 at 17:39, Greg Kurz <groug@kaod.org> 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.
> 

... which is likely to happen if it points to some data coming from
a QTEST originated stream of bytes... :-\

> 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);
> 

Well, at the present time there's only a 32-bit value but this
may change. So I'd prefer to copy the bytes into an aligned
structure.

> 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.)
> 

The code at the other end is test code in QTEST, the goal of
which is to simulate a blocking 9P "write" request. Is it safe
to assume that this test code does "host-order" stores ?

> thanks
> -- PMM