| Message ID | 1417105946-27374-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > We leak cpu mappings when 1st s/g is not exactly the > header. As we don't set ANY_LAYOUT, we can at this point > simply assert the correct length. > > This will have to be fixed once ANY_LAYOUT is set. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote: > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > We leak cpu mappings when 1st s/g is not exactly the > > header. As we don't set ANY_LAYOUT, we can at this point > > simply assert the correct length. > > > > This will have to be fixed once ANY_LAYOUT is set. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Untested: posting for early feedback. > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Stefan, you are going to merge this for 2.2? Peter, if Stefan is merging this one, we can take Jason's patch for -net and that should be enough for 2.2.
On Thu, 11/27 23:13, Michael S. Tsirkin wrote: > On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote: > > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > We leak cpu mappings when 1st s/g is not exactly the > > > header. As we don't set ANY_LAYOUT, we can at this point > > > simply assert the correct length. > > > > > > This will have to be fixed once ANY_LAYOUT is set. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > > > > Untested: posting for early feedback. > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > Stefan, you are going to merge this for 2.2? > Peter, if Stefan is merging this one, we can > take Jason's patch for -net and that should be > enough for 2.2. My test bot saw a failure of "make check": qemu-system-x86_64: virtio-blk request outhdr too long Broken pipe GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121 qemu-system-x86_64: virtio-blk request outhdr too long Broken pipe GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88 qemu-system-x86_64: virtio-blk request outhdr too long Broken pipe GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452 qemu-system-x86_64: virtio-blk request outhdr too long Broken pipe GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension. Task offloads will be emulated. make: *** [check-qtest-x86_64] Error 1 Fam
On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote: > On Thu, 11/27 23:13, Michael S. Tsirkin wrote: >> On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote: >> > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin >> <mst@redhat.com> wrote: >> > > We leak cpu mappings when 1st s/g is not exactly the >> > > header. As we don't set ANY_LAYOUT, we can at this point >> > > simply assert the correct length. >> > > >> > > This will have to be fixed once ANY_LAYOUT is set. >> > > >> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > > --- >> > > >> > > Untested: posting for early feedback. >> > >> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> >> Stefan, you are going to merge this for 2.2? >> Peter, if Stefan is merging this one, we can >> take Jason's patch for -net and that should be >> enough for 2.2. > > My test bot saw a failure of "make check": > > qemu-system-x86_64: virtio-blk request outhdr too long > Broken pipe > GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121 > qemu-system-x86_64: virtio-blk request outhdr too long > Broken pipe > GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88 > qemu-system-x86_64: virtio-blk request outhdr too long > Broken pipe > GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452 > qemu-system-x86_64: virtio-blk request outhdr too long > Broken pipe > GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f > [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio > extension. Task offloads will be emulated. > make: *** [check-qtest-x86_64] Error 1 > > Fam This probably because of the test itself. But anyway all kinds of guests (especially Windows drivers) need to be tested.
On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote: >> >> On Thu, 11/27 23:13, Michael S. Tsirkin wrote: >>> >>> On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote: >>> > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin <mst@redhat.com> >>> wrote: >>> > > We leak cpu mappings when 1st s/g is not exactly the >>> > > header. As we don't set ANY_LAYOUT, we can at this point >>> > > simply assert the correct length. >>> > > >>> > > This will have to be fixed once ANY_LAYOUT is set. >>> > > >>> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> > > --- >>> > > >>> > > Untested: posting for early feedback. >>> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> Stefan, you are going to merge this for 2.2? >>> Peter, if Stefan is merging this one, we can >>> take Jason's patch for -net and that should be >>> enough for 2.2. >> >> >> My test bot saw a failure of "make check": >> >> qemu-system-x86_64: virtio-blk request outhdr too long >> Broken pipe >> GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121 >> qemu-system-x86_64: virtio-blk request outhdr too long >> Broken pipe >> GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88 >> qemu-system-x86_64: virtio-blk request outhdr too long >> Broken pipe >> GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452 >> qemu-system-x86_64: virtio-blk request outhdr too long >> Broken pipe >> GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f >> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio extension. >> Task offloads will be emulated. >> make: *** [check-qtest-x86_64] Error 1 >> >> Fam > > > This probably because of the test itself. > > But anyway all kinds of guests (especially Windows drivers) need to be > tested. Right, the test case explicitly tests different descriptor layouts, even though virtio-blk-pci does not set the ANY_LAYOUT feature bit. Either the test case needs to check ANY_LAYOUT before using the 2-descriptor layout or it needs to expect QEMU to refuse (in this case exit(1), which is not very graceful). The quick fix is to skip the 2-descriptor layout tests and re-enable them once virtio-blk actually supports ANY_LAYOUT. Any objections? Stefan
El Fri, 28 Nov 2014 11:43:59 +0000 Stefan Hajnoczi <stefanha@gmail.com> escribió: > On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang <jasowang@redhat.com> > wrote: > > > > > > On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng <famz@redhat.com> wrote: > >> > >> On Thu, 11/27 23:13, Michael S. Tsirkin wrote: > >>> > >>> On Thu, Nov 27, 2014 at 07:21:35PM +0000, Stefan Hajnoczi wrote: > >>> > On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin > >>> > <mst@redhat.com> > >>> wrote: > >>> > > We leak cpu mappings when 1st s/g is not exactly the > >>> > > header. As we don't set ANY_LAYOUT, we can at this point > >>> > > simply assert the correct length. > >>> > > > >>> > > This will have to be fixed once ANY_LAYOUT is set. > >>> > > > >>> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> > > --- > >>> > > > >>> > > Untested: posting for early feedback. > >>> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> Stefan, you are going to merge this for 2.2? > >>> Peter, if Stefan is merging this one, we can > >>> take Jason's patch for -net and that should be > >>> enough for 2.2. > >> > >> > >> My test bot saw a failure of "make check": > >> > >> qemu-system-x86_64: virtio-blk request outhdr too long > >> Broken pipe > >> GTester: last random seed: R02S44c5a09d74c0603f0091ed20d1395121 > >> qemu-system-x86_64: virtio-blk request outhdr too long > >> Broken pipe > >> GTester: last random seed: R02Sfdf270c8e1ed75c1f2047e3f0fb89b88 > >> qemu-system-x86_64: virtio-blk request outhdr too long > >> Broken pipe > >> GTester: last random seed: R02Sd292c540929b963ed9d7d155f3db5452 > >> qemu-system-x86_64: virtio-blk request outhdr too long > >> Broken pipe > >> GTester: last random seed: R02Sfc775a34a844c39f376aa478eda7573f > >> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio > >> extension. Task offloads will be emulated. > >> make: *** [check-qtest-x86_64] Error 1 > >> > >> Fam > > > > > > This probably because of the test itself. > > > > But anyway all kinds of guests (especially Windows drivers) need to > > be tested. > > Right, the test case explicitly tests different descriptor layouts, > even though virtio-blk-pci does not set the ANY_LAYOUT feature bit. > > Either the test case needs to check ANY_LAYOUT before using the > 2-descriptor layout or it needs to expect QEMU to refuse (in this case > exit(1), which is not very graceful). > > The quick fix is to skip the 2-descriptor layout tests and re-enable > them once virtio-blk actually supports ANY_LAYOUT. Any objections? > > Stefan To be compliant with the specs, the test should check for ANY_LAYOUT feature before testing with 2 descriptor layout. I'll add it with the other changes pending for the test. Marc
On 28 November 2014 at 11:43, Stefan Hajnoczi <stefanha@gmail.com> wrote: > Right, the test case explicitly tests different descriptor layouts, > even though virtio-blk-pci does not set the ANY_LAYOUT feature bit. > > Either the test case needs to check ANY_LAYOUT before using the > 2-descriptor layout or it needs to expect QEMU to refuse (in this case > exit(1), which is not very graceful). > > The quick fix is to skip the 2-descriptor layout tests and re-enable > them once virtio-blk actually supports ANY_LAYOUT. Any objections? So what do we want to do with this for 2.2? We have I think two choices: (1) say that this isn't causing problems in practice, and defer all this to 2.3 (2) add something like this patch plus fix the 'make check' tests (but turning "maybe something misbehaves" into "qemu definitely blows up and exits" doesn't seem like a great improvement to me) I started looking at virtio-blk initially because I wasn't sure if we should fix the virtio-net issue in the core virtio code. But since we've decided not to do that, whether virtio-blk's problems are release-blockers or not is something that we can decide on their own merits. My current thought is that we don't need to address this for 2.2; is there something I'm missing that means we shouldn't defer to 2.3? thanks -- PMM
On Fri, Nov 28, 2014 at 04:14:35PM +0000, Peter Maydell wrote: > On 28 November 2014 at 11:43, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > Right, the test case explicitly tests different descriptor layouts, > > even though virtio-blk-pci does not set the ANY_LAYOUT feature bit. > > > > Either the test case needs to check ANY_LAYOUT before using the > > 2-descriptor layout or it needs to expect QEMU to refuse (in this case > > exit(1), which is not very graceful). > > > > The quick fix is to skip the 2-descriptor layout tests and re-enable > > them once virtio-blk actually supports ANY_LAYOUT. Any objections? > > So what do we want to do with this for 2.2? We have I think > two choices: > (1) say that this isn't causing problems in practice, and defer all > this to 2.3 > (2) add something like this patch plus fix the 'make check' tests > (but turning "maybe something misbehaves" into "qemu definitely > blows up and exits" doesn't seem like a great improvement to me) > > I started looking at virtio-blk initially because I wasn't sure > if we should fix the virtio-net issue in the core virtio code. > But since we've decided not to do that, whether virtio-blk's > problems are release-blockers or not is something that we can > decide on their own merits. > > My current thought is that we don't need to address this for 2.2; > is there something I'm missing that means we shouldn't defer to 2.3? > > thanks > -- PMM The result of this is host mapping leak. What effect does this have? Can this DOS host? If not, I agree.
On 30 November 2014 at 16:43, Michael S. Tsirkin <mst@redhat.com> wrote: > The result of this is host mapping leak. > What effect does this have? Can this DOS host? I don't think we can DOS the host here. If Xen, we crash (but you can't use virtio-blk with Xen anyway) Otherwise, if you managed to get address_space_map() to hand you the bounce-buffer (by asking for dma to something other than RAM) then we'll either hit an assertion or just end up never allowing dma to/from non-RAM ever again for this guest. The usual case would be that this was dma to/from ram, in which case it's harmless if the virtio-backend never wrote to the memory, and will fail to update dirty bitmaps for migration etc if the backend did write. In any case I think that none of these outcomes are worse than the "exit(1)" the current patch proposes. -- PMM
On Mon, Dec 01, 2014 at 12:07:07PM +0000, Peter Maydell wrote: > On 30 November 2014 at 16:43, Michael S. Tsirkin <mst@redhat.com> wrote: > > The result of this is host mapping leak. > > What effect does this have? Can this DOS host? > > I don't think we can DOS the host here. > > If Xen, we crash (but you can't use virtio-blk with Xen anyway) > Otherwise, if you managed to get address_space_map() to hand you > the bounce-buffer (by asking for dma to something other than RAM) > then we'll either hit an assertion or just end up never allowing > dma to/from non-RAM ever again for this guest. > The usual case would be that this was dma to/from ram, in > which case it's harmless if the virtio-backend never wrote to > the memory, and will fail to update dirty bitmaps for migration > etc if the backend did write. > > In any case I think that none of these outcomes are worse > than the "exit(1)" the current patch proposes. > > -- PMM Fair enough. Pls disregard the patch then, and we'll fix it properly for 2.3 when we set ANY_LAYOUT.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index b19b102..1404b3f 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -381,6 +381,12 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) exit(1); } + /* We don't advertize ANY_LAYOUT, so first s/g is exactly the header. */ + if (iov[0].iov_len != sizeof(req->out)) { + error_report("virtio-blk request outhdr too long"); + exit(1); + } + iov_discard_front(&iov, &out_num, sizeof(req->out)); if (in_num < 1 ||
We leak cpu mappings when 1st s/g is not exactly the header. As we don't set ANY_LAYOUT, we can at this point simply assert the correct length. This will have to be fixed once ANY_LAYOUT is set. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Untested: posting for early feedback. hw/block/virtio-blk.c | 6 ++++++ 1 file changed, 6 insertions(+)