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(-)
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
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
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
© 2016 - 2024 Red Hat, Inc.