| Message ID | 20200329113359.30960-1-eperezma@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vhost: Reset batched descriptors on SET_VRING_BASE call | expand |
On Sun, Mar 29, 2020 at 01:33:53PM +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, 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. Not sure if this can be avoided. Question: why not reset the batch when private_data changes? At the moment both net and scsi poke at private data directly, if they do this through a wrapper we can use that to 1. check that vq mutex is taken properly 2. reset batching This seems like a slightly better API > > Also, change from https://lkml.org/lkml/2020/3/27/108 is not included, that avoids to update a variable in a loop where it can be updated once. > > This is meant to be applied on top of eccb852f1fe6bede630e2e4f1a121a81e34354ab in git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git, and some commits should be squashed with that series. Thanks a lot! I'll apply this for now so Christian can start testing, but I'd like the comment above addressed before I push this to Linus. > Eugenio Pérez (6): > tools/virtio: Add --batch option > tools/virtio: Add --batch=random option > tools/virtio: Add --reset=random > tools/virtio: Make --reset reset ring idx > vhost: Delete virtqueue batch_descs member > fixup! vhost: batching fetches > > drivers/vhost/test.c | 57 ++++++++++++++++ > drivers/vhost/test.h | 1 + > drivers/vhost/vhost.c | 12 +++- > drivers/vhost/vhost.h | 1 - > drivers/virtio/virtio_ring.c | 18 +++++ > include/linux/virtio.h | 2 + > tools/virtio/linux/virtio.h | 2 + > tools/virtio/virtio_test.c | 123 +++++++++++++++++++++++++++++++---- > 8 files changed, 201 insertions(+), 15 deletions(-) > > -- > 2.18.1
On Sun, Mar 29, 2020 at 1:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Mar 29, 2020 at 01:33:53PM +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, 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. Not sure if this can be avoided. > > Question: why not reset the batch when private_data changes? > At the moment both net and scsi poke at private data directly, > if they do this through a wrapper we can use that to > 1. check that vq mutex is taken properly > 2. reset batching > > This seems like a slightly better API > I didn't do that way because qemu could just SET_BACKEND to -1 and SET_BACKEND to the same one, with no call to SET_VRING. In this case, I think that qemu should not change the descriptors already pushed. I do agree with the interface to modify private_data properly (regarding the mutex). However, I can see how your proposal is safer, so we don't even need to check if private_data is != NULL when we have descriptors in the batch_descs array. Also, this ioctls should not be in the hot path, so we can change to that mode anyway. > > > > Also, change from https://lkml.org/lkml/2020/3/27/108 is not included, that avoids to update a variable in a loop where it can be updated once. > > > > This is meant to be applied on top of eccb852f1fe6bede630e2e4f1a121a81e34354ab in git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git, and some commits should be squashed with that series. > > Thanks a lot! I'll apply this for now so Christian can start testing, > but I'd like the comment above addressed before I push this to Linus. > > > Eugenio Pérez (6): > > tools/virtio: Add --batch option > > tools/virtio: Add --batch=random option > > tools/virtio: Add --reset=random > > tools/virtio: Make --reset reset ring idx > > vhost: Delete virtqueue batch_descs member > > fixup! vhost: batching fetches > > > > drivers/vhost/test.c | 57 ++++++++++++++++ > > drivers/vhost/test.h | 1 + > > drivers/vhost/vhost.c | 12 +++- > > drivers/vhost/vhost.h | 1 - > > drivers/virtio/virtio_ring.c | 18 +++++ > > include/linux/virtio.h | 2 + > > tools/virtio/linux/virtio.h | 2 + > > tools/virtio/virtio_test.c | 123 +++++++++++++++++++++++++++++++---- > > 8 files changed, 201 insertions(+), 15 deletions(-) > > > > -- > > 2.18.1 >
On Sun, Mar 29, 2020 at 02:19:55PM +0200, Eugenio Perez Martin wrote: > On Sun, Mar 29, 2020 at 1:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Sun, Mar 29, 2020 at 01:33:53PM +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, 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. Not sure if this can be avoided. > > > > Question: why not reset the batch when private_data changes? > > At the moment both net and scsi poke at private data directly, > > if they do this through a wrapper we can use that to > > 1. check that vq mutex is taken properly > > 2. reset batching > > > > This seems like a slightly better API > > > > I didn't do that way because qemu could just SET_BACKEND to -1 and > SET_BACKEND to the same one, with no call to SET_VRING. In this case, > I think that qemu should not change the descriptors already pushed. Well dropping the batch is always safe, batch is an optimization. > I > do agree with the interface to modify private_data properly (regarding > the mutex). > > However, I can see how your proposal is safer, so we don't even need > to check if private_data is != NULL when we have descriptors in the > batch_descs array. Also, this ioctls should not be in the hot path, so > we can change to that mode anyway. > > > > > > > Also, change from https://lkml.org/lkml/2020/3/27/108 is not included, that avoids to update a variable in a loop where it can be updated once. > > > > > > This is meant to be applied on top of eccb852f1fe6bede630e2e4f1a121a81e34354ab in git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git, and some commits should be squashed with that series. > > > > Thanks a lot! I'll apply this for now so Christian can start testing, > > but I'd like the comment above addressed before I push this to Linus. > > > > > Eugenio Pérez (6): > > > tools/virtio: Add --batch option > > > tools/virtio: Add --batch=random option > > > tools/virtio: Add --reset=random > > > tools/virtio: Make --reset reset ring idx > > > vhost: Delete virtqueue batch_descs member > > > fixup! vhost: batching fetches > > > > > > drivers/vhost/test.c | 57 ++++++++++++++++ > > > drivers/vhost/test.h | 1 + > > > drivers/vhost/vhost.c | 12 +++- > > > drivers/vhost/vhost.h | 1 - > > > drivers/virtio/virtio_ring.c | 18 +++++ > > > include/linux/virtio.h | 2 + > > > tools/virtio/linux/virtio.h | 2 + > > > tools/virtio/virtio_test.c | 123 +++++++++++++++++++++++++++++++---- > > > 8 files changed, 201 insertions(+), 15 deletions(-) > > > > > > -- > > > 2.18.1 > >
On 29.03.20 13:33, 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. I guess this could explain my problems that I have seen during reset? > > This series ammend this, 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. Not sure if this can be avoided. > > Also, change from https://lkml.org/lkml/2020/3/27/108 is not included, that avoids to update a variable in a loop where it can be updated once. > > This is meant to be applied on top of eccb852f1fe6bede630e2e4f1a121a81e34354ab in git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git, and some commits should be squashed with that series. > > Eugenio Pérez (6): > tools/virtio: Add --batch option > tools/virtio: Add --batch=random option > tools/virtio: Add --reset=random > tools/virtio: Make --reset reset ring idx > vhost: Delete virtqueue batch_descs member > fixup! vhost: batching fetches > > drivers/vhost/test.c | 57 ++++++++++++++++ > drivers/vhost/test.h | 1 + > drivers/vhost/vhost.c | 12 +++- > drivers/vhost/vhost.h | 1 - > drivers/virtio/virtio_ring.c | 18 +++++ > include/linux/virtio.h | 2 + > tools/virtio/linux/virtio.h | 2 + > tools/virtio/virtio_test.c | 123 +++++++++++++++++++++++++++++++---- > 8 files changed, 201 insertions(+), 15 deletions(-) >
On Mon, Mar 30, 2020 at 9:14 AM Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > On 29.03.20 13:33, 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. > > I guess this could explain my problems that I have seen during reset? > Yes, I think so. The series has a test that should reproduce more or less what you are seeing. However, it would be useful to reproduce on your system and to know what causes qemu to send the reset :). Thanks! > > > > This series ammend this, 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. Not sure if this can be avoided. > > > > Also, change from https://lkml.org/lkml/2020/3/27/108 is not included, that avoids to update a variable in a loop where it can be updated once. > > > > This is meant to be applied on top of eccb852f1fe6bede630e2e4f1a121a81e34354ab in git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git, and some commits should be squashed with that series. > > > > Eugenio Pérez (6): > > tools/virtio: Add --batch option > > tools/virtio: Add --batch=random option > > tools/virtio: Add --reset=random > > tools/virtio: Make --reset reset ring idx > > vhost: Delete virtqueue batch_descs member > > fixup! vhost: batching fetches > > > > drivers/vhost/test.c | 57 ++++++++++++++++ > > drivers/vhost/test.h | 1 + > > drivers/vhost/vhost.c | 12 +++- > > drivers/vhost/vhost.h | 1 - > > drivers/virtio/virtio_ring.c | 18 +++++ > > include/linux/virtio.h | 2 + > > tools/virtio/linux/virtio.h | 2 + > > tools/virtio/virtio_test.c | 123 +++++++++++++++++++++++++++++++---- > > 8 files changed, 201 insertions(+), 15 deletions(-) > > >
On 30.03.20 09:18, Eugenio Perez Martin wrote: > On Mon, Mar 30, 2020 at 9:14 AM Christian Borntraeger > <borntraeger@de.ibm.com> wrote: >> >> >> On 29.03.20 13:33, 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. >> >> I guess this could explain my problems that I have seen during reset? >> > > Yes, I think so. The series has a test that should reproduce more or > less what you are seeing. However, it would be useful to reproduce on > your system and to know what causes qemu to send the reset :). I do see SET_VRING_BASE in the debug output [228101.438630] [2113] vhost:vhost_vring_ioctl:1668: VHOST_GET_VRING_BASE [vq=00000000618905fc][s.index=1][s.num=42424][vq->avail_idx=42424][vq->last_avail_idx=42424][vq->ndescs=0][vq->first_desc=0] [228101.438631] CPU: 54 PID: 2113 Comm: qemu-system-s39 Not tainted 5.5.0+ #344 [228101.438632] Hardware name: IBM 3906 M04 704 (LPAR) [228101.438633] Call Trace: [228101.438634] [<00000004fc71c132>] show_stack+0x8a/0xd0 [228101.438636] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 [228101.438639] [<000003ff80377600>] vhost_vring_ioctl+0x668/0x848 [vhost] [228101.438640] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] [228101.438642] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 [228101.438643] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 [228101.438645] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 [228101.438646] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 [228103.682732] [2122] vhost:vhost_vring_ioctl:1653: VHOST_SET_VRING_BASE [vq=000000009e1ac3e7][s.index=0][s.num=0][vq->avail_idx=27875][vq->last_avail_idx=27709][vq->ndescs=65][vq->first_desc=22] [228103.682735] CPU: 44 PID: 2122 Comm: CPU 0/KVM Not tainted 5.5.0+ #344 [228103.682739] Hardware name: IBM 3906 M04 704 (LPAR) [228103.682741] Call Trace: [228103.682748] [<00000004fc71c132>] show_stack+0x8a/0xd0 [228103.682752] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 [228103.682761] [<000003ff80377422>] vhost_vring_ioctl+0x48a/0x848 [vhost] [228103.682764] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] [228103.682767] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 [228103.682769] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 [228103.682771] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 [228103.682773] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 [228103.682794] [2122] vhost:vhost_vring_ioctl:1653: VHOST_SET_VRING_BASE [vq=00000000618905fc][s.index=1][s.num=0][vq->avail_idx=42424][vq->last_avail_idx=42424][vq->ndescs=0][vq->first_desc=0] [228103.682795] CPU: 44 PID: 2122 Comm: CPU 0/KVM Not tainted 5.5.0+ #344 [228103.682797] Hardware name: IBM 3906 M04 704 (LPAR) [228103.682797] Call Trace: [228103.682799] [<00000004fc71c132>] show_stack+0x8a/0xd0 [228103.682801] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 [228103.682804] [<000003ff80377422>] vhost_vring_ioctl+0x48a/0x848 [vhost] [228103.682806] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] [228103.682808] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 [228103.682810] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 [228103.682812] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 [228103.682813] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 Isnt that triggered by resetting the virtio devices during system reboot?
On Mon, Mar 30, 2020 at 9:34 AM Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > > On 30.03.20 09:18, Eugenio Perez Martin wrote: > > On Mon, Mar 30, 2020 at 9:14 AM Christian Borntraeger > > <borntraeger@de.ibm.com> wrote: > >> > >> > >> On 29.03.20 13:33, 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. > >> > >> I guess this could explain my problems that I have seen during reset? > >> > > > > Yes, I think so. The series has a test that should reproduce more or > > less what you are seeing. However, it would be useful to reproduce on > > your system and to know what causes qemu to send the reset :). > > I do see SET_VRING_BASE in the debug output > [228101.438630] [2113] vhost:vhost_vring_ioctl:1668: VHOST_GET_VRING_BASE [vq=00000000618905fc][s.index=1][s.num=42424][vq->avail_idx=42424][vq->last_avail_idx=42424][vq->ndescs=0][vq->first_desc=0] > [228101.438631] CPU: 54 PID: 2113 Comm: qemu-system-s39 Not tainted 5.5.0+ #344 > [228101.438632] Hardware name: IBM 3906 M04 704 (LPAR) > [228101.438633] Call Trace: > [228101.438634] [<00000004fc71c132>] show_stack+0x8a/0xd0 > [228101.438636] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 > [228101.438639] [<000003ff80377600>] vhost_vring_ioctl+0x668/0x848 [vhost] > [228101.438640] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] > [228101.438642] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 > [228101.438643] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 > [228101.438645] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 > [228101.438646] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 > [228103.682732] [2122] vhost:vhost_vring_ioctl:1653: VHOST_SET_VRING_BASE [vq=000000009e1ac3e7][s.index=0][s.num=0][vq->avail_idx=27875][vq->last_avail_idx=27709][vq->ndescs=65][vq->first_desc=22] > [228103.682735] CPU: 44 PID: 2122 Comm: CPU 0/KVM Not tainted 5.5.0+ #344 > [228103.682739] Hardware name: IBM 3906 M04 704 (LPAR) > [228103.682741] Call Trace: > [228103.682748] [<00000004fc71c132>] show_stack+0x8a/0xd0 > [228103.682752] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 > [228103.682761] [<000003ff80377422>] vhost_vring_ioctl+0x48a/0x848 [vhost] > [228103.682764] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] > [228103.682767] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 > [228103.682769] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 > [228103.682771] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 > [228103.682773] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 > [228103.682794] [2122] vhost:vhost_vring_ioctl:1653: VHOST_SET_VRING_BASE [vq=00000000618905fc][s.index=1][s.num=0][vq->avail_idx=42424][vq->last_avail_idx=42424][vq->ndescs=0][vq->first_desc=0] > [228103.682795] CPU: 44 PID: 2122 Comm: CPU 0/KVM Not tainted 5.5.0+ #344 > [228103.682797] Hardware name: IBM 3906 M04 704 (LPAR) > [228103.682797] Call Trace: > [228103.682799] [<00000004fc71c132>] show_stack+0x8a/0xd0 > [228103.682801] [<00000004fd10e72a>] dump_stack+0x8a/0xb8 > [228103.682804] [<000003ff80377422>] vhost_vring_ioctl+0x48a/0x848 [vhost] > [228103.682806] [<000003ff80395fd4>] vhost_net_ioctl+0x4f4/0x570 [vhost_net] > [228103.682808] [<00000004fc9ccdd8>] do_vfs_ioctl+0x430/0x6f8 > [228103.682810] [<00000004fc9cd124>] ksys_ioctl+0x84/0xb0 > [228103.682812] [<00000004fc9cd1ba>] __s390x_sys_ioctl+0x2a/0x38 > [228103.682813] [<00000004fd12ff72>] system_call+0x2a6/0x2c8 > > > Isnt that triggered by resetting the virtio devices during system reboot? > Yes. I don't know exactly why qemu is sending them, but vhost should be able to "protect/continue" the same way it used to be before batching patches. Did you lose connectivity or experienced rebooting with this patches applied? Thanks!