Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On 30 November 2014 at 16:43, Michael S. Tsirkin m...@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
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Mon, Dec 01, 2014 at 12:07:07PM +, Peter Maydell wrote: On 30 November 2014 at 16:43, Michael S. Tsirkin m...@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.
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Fri, Nov 28, 2014 at 04:14:35PM +, Peter Maydell wrote: On 28 November 2014 at 11:43, Stefan Hajnoczi stefa...@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.
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang jasow...@redhat.com wrote: On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng f...@redhat.com wrote: On Thu, 11/27 23:13, Michael S. Tsirkin wrote: On Thu, Nov 27, 2014 at 07:21:35PM +, Stefan Hajnoczi wrote: On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@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
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
El Fri, 28 Nov 2014 11:43:59 + Stefan Hajnoczi stefa...@gmail.com escribió: On Fri, Nov 28, 2014 at 7:05 AM, Jason Wang jasow...@redhat.com wrote: On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng f...@redhat.com wrote: On Thu, 11/27 23:13, Michael S. Tsirkin wrote: On Thu, Nov 27, 2014 at 07:21:35PM +, Stefan Hajnoczi wrote: On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@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
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On 28 November 2014 at 11:43, Stefan Hajnoczi stefa...@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
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@redhat.com
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Thu, Nov 27, 2014 at 07:21:35PM +, Stefan Hajnoczi wrote: On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@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. -- MST
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Thu, 11/27 23:13, Michael S. Tsirkin wrote: On Thu, Nov 27, 2014 at 07:21:35PM +, Stefan Hajnoczi wrote: On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@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
Re: [Qemu-devel] [PATCH RFC for-2.2] virtio-blk: force 1st s/g to match header
On Fri, Nov 28, 2014 at 9:16 AM, Fam Zheng f...@redhat.com wrote: On Thu, 11/27 23:13, Michael S. Tsirkin wrote: On Thu, Nov 27, 2014 at 07:21:35PM +, Stefan Hajnoczi wrote: On Thu, Nov 27, 2014 at 4:33 PM, Michael S. Tsirkin m...@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 m...@redhat.com --- Untested: posting for early feedback. Reviewed-by: Stefan Hajnoczi stefa...@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.
