Vhost did not reset properly the batched descriptors on SET_VRING_BASE
event. Because of that, is possible to return an invalid descriptor to
the guest.
This series ammend this, resetting them every time backend changes, and
creates a test to assert correct behavior. To do that, they need to
expose a new function in virtio_ring, virtqueue_reset_free_head, only
on test code.
Another useful thing would be to check if mutex is properly get in
vq private_data accessors. Not sure if mutex debug code allow that,
similar to C++ unique lock::owns_lock. Not acquiring in the function
because caller code holds the mutex in order to perform more actions.
v3:
* Rename accesors functions.
* Make scsi and test use the accesors too.
v2:
* Squashed commits.
* Create vq private_data accesors (mst).
This is meant to be applied on top of
c4f1c41a6094582903c75c0dcfacb453c959d457 in
git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
Eugenio Pérez (5):
vhost: Create accessors for virtqueues private_data
tools/virtio: Add --batch option
tools/virtio: Add --batch=random option
tools/virtio: Add --reset=random
tools/virtio: Make --reset reset ring idx
Michael S. Tsirkin (3):
vhost: option to fetch descriptors through an independent struct
vhost: use batched version by default
vhost: batching fetches
drivers/vhost/net.c | 28 ++--
drivers/vhost/scsi.c | 14 +-
drivers/vhost/test.c | 69 ++++++++-
drivers/vhost/test.h | 1 +
drivers/vhost/vhost.c | 271 +++++++++++++++++++++++------------
drivers/vhost/vhost.h | 44 +++++-
drivers/vhost/vsock.c | 14 +-
drivers/virtio/virtio_ring.c | 29 ++++
tools/virtio/linux/virtio.h | 2 +
tools/virtio/virtio_test.c | 123 ++++++++++++++--
10 files changed, 456 insertions(+), 139 deletions(-)
--
2.18.1
On Tue, Mar 31, 2020 at 09:27:56PM +0200, Eugenio P?rez wrote:
> Vhost did not reset properly the batched descriptors on SET_VRING_BASE
> event. Because of that, is possible to return an invalid descriptor to
> the guest.
>
> This series ammend this, resetting them every time backend changes, and
> creates a test to assert correct behavior. To do that, they need to
> expose a new function in virtio_ring, virtqueue_reset_free_head, only
> on test code.
>
> Another useful thing would be to check if mutex is properly get in
> vq private_data accessors. Not sure if mutex debug code allow that,
> similar to C++ unique lock::owns_lock. Not acquiring in the function
> because caller code holds the mutex in order to perform more actions.
I pushed vhost branch with patch 1, pls send patches on top of that!
> v3:
> * Rename accesors functions.
> * Make scsi and test use the accesors too.
>
> v2:
> * Squashed commits.
> * Create vq private_data accesors (mst).
>
> This is meant to be applied on top of
> c4f1c41a6094582903c75c0dcfacb453c959d457 in
> git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
>
> Eugenio P?rez (5):
> vhost: Create accessors for virtqueues private_data
> tools/virtio: Add --batch option
> tools/virtio: Add --batch=random option
> tools/virtio: Add --reset=random
> tools/virtio: Make --reset reset ring idx
>
> Michael S. Tsirkin (3):
> vhost: option to fetch descriptors through an independent struct
> vhost: use batched version by default
> vhost: batching fetches
>
> drivers/vhost/net.c | 28 ++--
> drivers/vhost/scsi.c | 14 +-
> drivers/vhost/test.c | 69 ++++++++-
> drivers/vhost/test.h | 1 +
> drivers/vhost/vhost.c | 271 +++++++++++++++++++++++------------
> drivers/vhost/vhost.h | 44 +++++-
> drivers/vhost/vsock.c | 14 +-
> drivers/virtio/virtio_ring.c | 29 ++++
> tools/virtio/linux/virtio.h | 2 +
> tools/virtio/virtio_test.c | 123 ++++++++++++++--
> 10 files changed, 456 insertions(+), 139 deletions(-)
>
> --
> 2.18.1
On 31.03.20 21:27, Eugenio Pérez wrote:
> Vhost did not reset properly the batched descriptors on SET_VRING_BASE
> event. Because of that, is possible to return an invalid descriptor to
> the guest.
>
> This series ammend this, resetting them every time backend changes, and
> creates a test to assert correct behavior. To do that, they need to
> expose a new function in virtio_ring, virtqueue_reset_free_head, only
> on test code.
>
> Another useful thing would be to check if mutex is properly get in
> vq private_data accessors. Not sure if mutex debug code allow that,
> similar to C++ unique lock::owns_lock. Not acquiring in the function
> because caller code holds the mutex in order to perform more actions.
>
> v3:
> * Rename accesors functions.
> * Make scsi and test use the accesors too.
>
> v2:
> * Squashed commits.
> * Create vq private_data accesors (mst).
>
> This is meant to be applied on top of
> c4f1c41a6094582903c75c0dcfacb453c959d457 in
> git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
A quick test on s390 looks good.
On Wed, Apr 1, 2020 at 9:19 AM Christian Borntraeger
<[email protected]> wrote:
>
> On 31.03.20 21:27, Eugenio Pérez wrote:
> > Vhost did not reset properly the batched descriptors on SET_VRING_BASE
> > event. Because of that, is possible to return an invalid descriptor to
> > the guest.
> >
> > This series ammend this, resetting them every time backend changes, and
> > creates a test to assert correct behavior. To do that, they need to
> > expose a new function in virtio_ring, virtqueue_reset_free_head, only
> > on test code.
> >
> > Another useful thing would be to check if mutex is properly get in
> > vq private_data accessors. Not sure if mutex debug code allow that,
> > similar to C++ unique lock::owns_lock. Not acquiring in the function
> > because caller code holds the mutex in order to perform more actions.
>
>
>
> >
> > v3:
> > * Rename accesors functions.
> > * Make scsi and test use the accesors too.
> >
> > v2:
> > * Squashed commits.
> > * Create vq private_data accesors (mst).
> >
> > This is meant to be applied on top of
> > c4f1c41a6094582903c75c0dcfacb453c959d457 in
> > git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
>
>
> A quick test on s390 looks good.
>
Really good to know :).
Would it be possible to investigate when qemu launches the offending ioctls?
Thanks!
On 01.04.20 20:40, Eugenio Perez Martin wrote:
> On Wed, Apr 1, 2020 at 9:19 AM Christian Borntraeger
> <[email protected]> wrote:
>>
>> On 31.03.20 21:27, Eugenio Pérez wrote:
>>> Vhost did not reset properly the batched descriptors on SET_VRING_BASE
>>> event. Because of that, is possible to return an invalid descriptor to
>>> the guest.
>>>
>>> This series ammend this, resetting them every time backend changes, and
>>> creates a test to assert correct behavior. To do that, they need to
>>> expose a new function in virtio_ring, virtqueue_reset_free_head, only
>>> on test code.
>>>
>>> Another useful thing would be to check if mutex is properly get in
>>> vq private_data accessors. Not sure if mutex debug code allow that,
>>> similar to C++ unique lock::owns_lock. Not acquiring in the function
>>> because caller code holds the mutex in order to perform more actions.
>>
>>
>>
>>>
>>> v3:
>>> * Rename accesors functions.
>>> * Make scsi and test use the accesors too.
>>>
>>> v2:
>>> * Squashed commits.
>>> * Create vq private_data accesors (mst).
>>>
>>> This is meant to be applied on top of
>>> c4f1c41a6094582903c75c0dcfacb453c959d457 in
>>> git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git.
>>
>>
>> A quick test on s390 looks good.
>>
>
> Really good to know :).
>
> Would it be possible to investigate when qemu launches the offending ioctls?
During guest reboot. This is obvious, no?
>> Would it be possible to investigate when qemu launches the offending ioctls?
>
> During guest reboot. This is obvious, no?
>
For example during reboot we do re-setup the virt queues:
#1 0x00000000010f3e7a in vhost_kernel_set_vring_base (dev=0x21f5f30, ring=0x3ff84d74e88) at /home/cborntra/REPOS/qemu/hw/virtio/vhost-backend.c:126
#2 0x00000000010f2f92 in vhost_virtqueue_start (idx=0, vq=0x21f6180, vdev=0x241d570, dev=0x21f5f30) at /home/cborntra/REPOS/qemu/hw/virtio/vhost.c:1016
#3 vhost_dev_start (hdev=hdev@entry=0x21f5f30, vdev=vdev@entry=0x241d570) at /home/cborntra/REPOS/qemu/hw/virtio/vhost.c:1646
#4 0x00000000011c265a in vhost_net_start_one (dev=0x241d570, net=0x21f5f30) at /home/cborntra/REPOS/qemu/hw/net/vhost_net.c:236
#5 vhost_net_start (dev=dev@entry=0x241d570, ncs=0x2450f40, total_queues=total_queues@entry=1) at /home/cborntra/REPOS/qemu/hw/net/vhost_net.c:338
#6 0x00000000010cfdfe in virtio_net_vhost_status (status=15 '\017', n=0x241d570) at /home/cborntra/REPOS/qemu/hw/net/virtio-net.c:250
#7 virtio_net_set_status (vdev=0x241d570, status=<optimized out>) at /home/cborntra/REPOS/qemu/hw/net/virtio-net.c:331
#8 0x00000000010eaef4 in virtio_set_status (vdev=vdev@entry=0x241d570, val=<optimized out>) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1956
#9 0x000000000110ba78 in virtio_ccw_cb (sch=0x2422c30, ccw=...) at /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:509
#10 0x00000000011053fc in css_interpret_ccw (sch=sch@entry=0x2422c30, ccw_addr=<optimized out>, suspend_allowed=suspend_allowed@entry=false) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1108
#11 0x000000000110557c in sch_handle_start_func_virtual (sch=0x2422c30) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1162
#12 do_subchannel_work_virtual (sch=0x2422c30) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1256
#13 0x0000000001168592 in ioinst_handle_ssch (cpu=cpu@entry=0x234b920, reg1=<optimized out>, ipb=<optimized out>, ra=ra@entry=0) at /home/cborntra/REPOS/qemu/target/s390x/ioinst.c:218
#14 0x0000000001170012 in handle_b2 (ipa1=<optimized out>, run=0x3ff97880000, cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1279
#15 handle_instruction (run=0x3ff97880000, cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1664
#16 handle_intercept (cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1747
#17 kvm_arch_handle_exit (cs=cs@entry=0x234b920, run=run@entry=0x3ff97880000) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1937
#18 0x00000000010972dc in kvm_cpu_exec (cpu=cpu@entry=0x234b920) at /home/cborntra/REPOS/qemu/accel/kvm/kvm-all.c:2445
#19 0x00000000010784f6 in qemu_kvm_cpu_thread_fn (arg=0x234b920) at /home/cborntra/REPOS/qemu/cpus.c:1246
#20 qemu_kvm_cpu_thread_fn (arg=arg@entry=0x234b920) at /home/cborntra/REPOS/qemu/cpus.c:1218
#21 0x00000000013891fa in qemu_thread_start (args=0x2372f30) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:519
#22 0x000003ff93809ed6 in start_thread () from target:/lib64/libpthread.so.0
#23 0x000003ff93705e46 in thread_start () from target:/lib64/libc.so.6
On Wed, Apr 1, 2020 at 9:13 PM Christian Borntraeger
<[email protected]> wrote:
>
> >> Would it be possible to investigate when qemu launches the offending ioctls?
> >
> > During guest reboot. This is obvious, no?
> >
>
Got it. I thought you received it after the reboot AND once you sent
the firsts packets, since you said that some pings worked. But now
that this point is clear, I think we can move forward :). Thank you
very much!
>
> For example during reboot we do re-setup the virt queues:
>
> #1 0x00000000010f3e7a in vhost_kernel_set_vring_base (dev=0x21f5f30, ring=0x3ff84d74e88) at /home/cborntra/REPOS/qemu/hw/virtio/vhost-backend.c:126
> #2 0x00000000010f2f92 in vhost_virtqueue_start (idx=0, vq=0x21f6180, vdev=0x241d570, dev=0x21f5f30) at /home/cborntra/REPOS/qemu/hw/virtio/vhost.c:1016
> #3 vhost_dev_start (hdev=hdev@entry=0x21f5f30, vdev=vdev@entry=0x241d570) at /home/cborntra/REPOS/qemu/hw/virtio/vhost.c:1646
> #4 0x00000000011c265a in vhost_net_start_one (dev=0x241d570, net=0x21f5f30) at /home/cborntra/REPOS/qemu/hw/net/vhost_net.c:236
> #5 vhost_net_start (dev=dev@entry=0x241d570, ncs=0x2450f40, total_queues=total_queues@entry=1) at /home/cborntra/REPOS/qemu/hw/net/vhost_net.c:338
> #6 0x00000000010cfdfe in virtio_net_vhost_status (status=15 '\017', n=0x241d570) at /home/cborntra/REPOS/qemu/hw/net/virtio-net.c:250
> #7 virtio_net_set_status (vdev=0x241d570, status=<optimized out>) at /home/cborntra/REPOS/qemu/hw/net/virtio-net.c:331
> #8 0x00000000010eaef4 in virtio_set_status (vdev=vdev@entry=0x241d570, val=<optimized out>) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1956
> #9 0x000000000110ba78 in virtio_ccw_cb (sch=0x2422c30, ccw=...) at /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:509
> #10 0x00000000011053fc in css_interpret_ccw (sch=sch@entry=0x2422c30, ccw_addr=<optimized out>, suspend_allowed=suspend_allowed@entry=false) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1108
> #11 0x000000000110557c in sch_handle_start_func_virtual (sch=0x2422c30) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1162
> #12 do_subchannel_work_virtual (sch=0x2422c30) at /home/cborntra/REPOS/qemu/hw/s390x/css.c:1256
> #13 0x0000000001168592 in ioinst_handle_ssch (cpu=cpu@entry=0x234b920, reg1=<optimized out>, ipb=<optimized out>, ra=ra@entry=0) at /home/cborntra/REPOS/qemu/target/s390x/ioinst.c:218
> #14 0x0000000001170012 in handle_b2 (ipa1=<optimized out>, run=0x3ff97880000, cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1279
> #15 handle_instruction (run=0x3ff97880000, cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1664
> #16 handle_intercept (cpu=0x234b920) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1747
> #17 kvm_arch_handle_exit (cs=cs@entry=0x234b920, run=run@entry=0x3ff97880000) at /home/cborntra/REPOS/qemu/target/s390x/kvm.c:1937
> #18 0x00000000010972dc in kvm_cpu_exec (cpu=cpu@entry=0x234b920) at /home/cborntra/REPOS/qemu/accel/kvm/kvm-all.c:2445
> #19 0x00000000010784f6 in qemu_kvm_cpu_thread_fn (arg=0x234b920) at /home/cborntra/REPOS/qemu/cpus.c:1246
> #20 qemu_kvm_cpu_thread_fn (arg=arg@entry=0x234b920) at /home/cborntra/REPOS/qemu/cpus.c:1218
> #21 0x00000000013891fa in qemu_thread_start (args=0x2372f30) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:519
> #22 0x000003ff93809ed6 in start_thread () from target:/lib64/libpthread.so.0
> #23 0x000003ff93705e46 in thread_start () from target:/lib64/libc.so.6
>