[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130 |
Date: |
Wed, 31 Jan 2018 17:30:06 +0100 |
On Wed, 31 Jan 2018 10:14:46 +0000
Peter Maydell <address@hidden> wrote:
> 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.
>
... 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
- [Qemu-devel] [PULL 05/10] tests: virtio-9p: use the synth backend, (continued)
- [Qemu-devel] [PULL 05/10] tests: virtio-9p: use the synth backend, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 10/10] tests/virtio-9p: explicitly handle potential integer overflows, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 08/10] libqos/virtio: return length written into used descriptor, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 07/10] tests: virtio-9p: add WRITE operation test, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 01/10] 9pfs: drop v9fs_register_transport(), Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 02/10] 9pfs: Correctly handle cancelled requests, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 09/10] tests: virtio-9p: add FLUSH operation test, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 03/10] tests: virtio-9p: move request tag to the test functions, Greg Kurz, 2018/01/30
- [Qemu-devel] [PULL 04/10] tests: virtio-9p: wait for completion in the test code, Greg Kurz, 2018/01/30
- Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130, Peter Maydell, 2018/01/31
- Re: [Qemu-devel] [PULL 00/10] 9p patches for 2.12 20180130,
Greg Kurz <=