* [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
@ 2021-01-27 20:44 Eugenio Pérez
2021-01-28 2:32 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Pérez @ 2021-01-27 20:44 UTC (permalink / raw)
To: qemu-devel; +Cc: maxime.coquelin, Jason Wang, Michael S. Tsirkin
Not registering this can lead to vhost_backend_handle_iotlb_msg and
vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
stop.
This causes a try to access dev->vdev->dma_as with vdev == NULL.
Reproduced rebooting a guest with testpmd in txonly forward mode.
#0 0x0000559ffff94394 in vhost_device_iotlb_miss (
dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
at ../hw/virtio/vhost.c:1013
#1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
at ../hw/virtio/vhost-backend.c:411
#2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
imsg=imsg@entry=0x7ffddcfd32c0)
at ../hw/virtio/vhost-backend.c:404
#3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
at ../hw/virtio/vhost-user.c:1464
#4 0x000055a0000c541b in aio_dispatch_handler (
ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
at ../util/aio-posix.c:329
Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
hw/virtio/vhost-user.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2fdd5daf74..a49b2229fb 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -238,6 +238,7 @@ struct vhost_user {
/* Shared between vhost devs of the same virtio device */
VhostUserState *user;
int slave_fd;
+ bool iotlb_enabled;
NotifierWithReturn postcopy_notifier;
struct PostCopyFD postcopy_fd;
uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
@@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
switch (hdr.request) {
case VHOST_USER_SLAVE_IOTLB_MSG:
- ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
+ if (likely(u->iotlb_enabled)) {
+ ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
+ } else {
+ ret = -EFAULT;
+ }
break;
case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
ret = vhost_user_slave_handle_config_change(dev);
@@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
{
- /* No-op as the receive channel is not dedicated to IOTLB messages. */
+ struct vhost_user *u = dev->opaque;
+ u->iotlb_enabled = enabled;
}
static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
--
2.27.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-27 20:44 [PATCH] vhost-user: Check for iotlb callback in iotlb_miss Eugenio Pérez
@ 2021-01-28 2:32 ` Jason Wang
2021-01-28 9:37 ` Eugenio Perez Martin
0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2021-01-28 2:32 UTC (permalink / raw)
To: Eugenio Pérez, qemu-devel; +Cc: maxime.coquelin, Michael S. Tsirkin
On 2021/1/28 上午4:44, Eugenio Pérez wrote:
> Not registering this can lead to vhost_backend_handle_iotlb_msg and
> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
> stop.
>
> This causes a try to access dev->vdev->dma_as with vdev == NULL.
Hi Eugenio:
What condition can we get this? Did you mean we receive IOTLB_MISS
before vhost_dev_start()?
If yes, it looks to me a bug somewhere else.
Thanks
>
> Reproduced rebooting a guest with testpmd in txonly forward mode.
> #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
> at ../hw/virtio/vhost.c:1013
> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
> at ../hw/virtio/vhost-backend.c:411
> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
> imsg=imsg@entry=0x7ffddcfd32c0)
> at ../hw/virtio/vhost-backend.c:404
> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
> at ../hw/virtio/vhost-user.c:1464
> #4 0x000055a0000c541b in aio_dispatch_handler (
> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
> at ../util/aio-posix.c:329
>
> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> hw/virtio/vhost-user.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 2fdd5daf74..a49b2229fb 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -238,6 +238,7 @@ struct vhost_user {
> /* Shared between vhost devs of the same virtio device */
> VhostUserState *user;
> int slave_fd;
> + bool iotlb_enabled;
> NotifierWithReturn postcopy_notifier;
> struct PostCopyFD postcopy_fd;
> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
>
> switch (hdr.request) {
> case VHOST_USER_SLAVE_IOTLB_MSG:
> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> + if (likely(u->iotlb_enabled)) {
> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> + } else {
> + ret = -EFAULT;
> + }
> break;
> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
> ret = vhost_user_slave_handle_config_change(dev);
> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
>
> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> {
> - /* No-op as the receive channel is not dedicated to IOTLB messages. */
> + struct vhost_user *u = dev->opaque;
> + u->iotlb_enabled = enabled;
> }
>
> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-28 2:32 ` Jason Wang
@ 2021-01-28 9:37 ` Eugenio Perez Martin
2021-01-29 3:29 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Perez Martin @ 2021-01-28 9:37 UTC (permalink / raw)
To: Jason Wang; +Cc: Maxime Coquelin, qemu-level, Michael S. Tsirkin
Hi Jason.
On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/1/28 上午4:44, Eugenio Pérez wrote:
> > Not registering this can lead to vhost_backend_handle_iotlb_msg and
> > vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
> > stop.
> >
> > This causes a try to access dev->vdev->dma_as with vdev == NULL.
>
>
> Hi Eugenio:
>
> What condition can we get this? Did you mean we receive IOTLB_MISS
> before vhost_dev_start()?
>
In the VM reboot case, yes, between vhost_dev_stop() and
vhost_dev_start(). But I can reproduce the bug in VM shutdown too,
with no vhost_dev_start expected.
I didn't try to send IOTLB_MISS before starting vhost_dev, but this
patch should solve that problem too.
I think we can get this with whatever malicious/buggy vhost-user
backend sending unexpected iotlb misses, but I didn't test so deeply.
> If yes, it looks to me a bug somewhere else.
Where should I look for it?
Thanks!
>
> Thanks
>
>
> >
> > Reproduced rebooting a guest with testpmd in txonly forward mode.
> > #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
> > dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
> > at ../hw/virtio/vhost.c:1013
> > #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
> > imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
> > at ../hw/virtio/vhost-backend.c:411
> > #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
> > imsg=imsg@entry=0x7ffddcfd32c0)
> > at ../hw/virtio/vhost-backend.c:404
> > #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
> > at ../hw/virtio/vhost-user.c:1464
> > #4 0x000055a0000c541b in aio_dispatch_handler (
> > ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
> > at ../util/aio-posix.c:329
> >
> > Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > hw/virtio/vhost-user.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 2fdd5daf74..a49b2229fb 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -238,6 +238,7 @@ struct vhost_user {
> > /* Shared between vhost devs of the same virtio device */
> > VhostUserState *user;
> > int slave_fd;
> > + bool iotlb_enabled;
> > NotifierWithReturn postcopy_notifier;
> > struct PostCopyFD postcopy_fd;
> > uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
> > @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
> >
> > switch (hdr.request) {
> > case VHOST_USER_SLAVE_IOTLB_MSG:
> > - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> > + if (likely(u->iotlb_enabled)) {
> > + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> > + } else {
> > + ret = -EFAULT;
> > + }
> > break;
> > case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
> > ret = vhost_user_slave_handle_config_change(dev);
> > @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
> >
> > static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> > {
> > - /* No-op as the receive channel is not dedicated to IOTLB messages. */
> > + struct vhost_user *u = dev->opaque;
> > + u->iotlb_enabled = enabled;
> > }
> >
> > static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-28 9:37 ` Eugenio Perez Martin
@ 2021-01-29 3:29 ` Jason Wang
2021-01-29 7:22 ` Eugenio Perez Martin
0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2021-01-29 3:29 UTC (permalink / raw)
To: Eugenio Perez Martin; +Cc: Maxime Coquelin, qemu-level, Michael S. Tsirkin
On 2021/1/28 下午5:37, Eugenio Perez Martin wrote:
> Hi Jason.
>
> On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/1/28 上午4:44, Eugenio Pérez wrote:
>>> Not registering this can lead to vhost_backend_handle_iotlb_msg and
>>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
>>> stop.
>>>
>>> This causes a try to access dev->vdev->dma_as with vdev == NULL.
>>
>> Hi Eugenio:
>>
>> What condition can we get this? Did you mean we receive IOTLB_MISS
>> before vhost_dev_start()?
>>
> In the VM reboot case, yes, between vhost_dev_stop() and
> vhost_dev_start(). But I can reproduce the bug in VM shutdown too,
> with no vhost_dev_start expected.
>
> I didn't try to send IOTLB_MISS before starting vhost_dev, but this
> patch should solve that problem too.
>
> I think we can get this with whatever malicious/buggy vhost-user
> backend sending unexpected iotlb misses, but I didn't test so deeply.
I see.
>
>> If yes, it looks to me a bug somewhere else.
> Where should I look for it?
So I winder whether or not we can simply ignore IOTLB message if vhost
device is not started.
Thanks
>
> Thanks!
>
>> Thanks
>>
>>
>>> Reproduced rebooting a guest with testpmd in txonly forward mode.
>>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
>>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
>>> at ../hw/virtio/vhost.c:1013
>>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
>>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
>>> at ../hw/virtio/vhost-backend.c:411
>>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
>>> imsg=imsg@entry=0x7ffddcfd32c0)
>>> at ../hw/virtio/vhost-backend.c:404
>>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
>>> at ../hw/virtio/vhost-user.c:1464
>>> #4 0x000055a0000c541b in aio_dispatch_handler (
>>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
>>> at ../util/aio-posix.c:329
>>>
>>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>> hw/virtio/vhost-user.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index 2fdd5daf74..a49b2229fb 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -238,6 +238,7 @@ struct vhost_user {
>>> /* Shared between vhost devs of the same virtio device */
>>> VhostUserState *user;
>>> int slave_fd;
>>> + bool iotlb_enabled;
>>> NotifierWithReturn postcopy_notifier;
>>> struct PostCopyFD postcopy_fd;
>>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
>>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
>>>
>>> switch (hdr.request) {
>>> case VHOST_USER_SLAVE_IOTLB_MSG:
>>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
>>> + if (likely(u->iotlb_enabled)) {
>>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
>>> + } else {
>>> + ret = -EFAULT;
>>> + }
>>> break;
>>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
>>> ret = vhost_user_slave_handle_config_change(dev);
>>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
>>>
>>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
>>> {
>>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */
>>> + struct vhost_user *u = dev->opaque;
>>> + u->iotlb_enabled = enabled;
>>> }
>>>
>>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-29 3:29 ` Jason Wang
@ 2021-01-29 7:22 ` Eugenio Perez Martin
2021-01-29 7:29 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Perez Martin @ 2021-01-29 7:22 UTC (permalink / raw)
To: Jason Wang; +Cc: Maxime Coquelin, qemu-level, Michael S. Tsirkin
On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/1/28 下午5:37, Eugenio Perez Martin wrote:
> > Hi Jason.
> >
> > On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/1/28 上午4:44, Eugenio Pérez wrote:
> >>> Not registering this can lead to vhost_backend_handle_iotlb_msg and
> >>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
> >>> stop.
> >>>
> >>> This causes a try to access dev->vdev->dma_as with vdev == NULL.
> >>
> >> Hi Eugenio:
> >>
> >> What condition can we get this? Did you mean we receive IOTLB_MISS
> >> before vhost_dev_start()?
> >>
> > In the VM reboot case, yes, between vhost_dev_stop() and
> > vhost_dev_start(). But I can reproduce the bug in VM shutdown too,
> > with no vhost_dev_start expected.
> >
> > I didn't try to send IOTLB_MISS before starting vhost_dev, but this
> > patch should solve that problem too.
> >
> > I think we can get this with whatever malicious/buggy vhost-user
> > backend sending unexpected iotlb misses, but I didn't test so deeply.
>
>
> I see.
>
>
> >
> >> If yes, it looks to me a bug somewhere else.
> > Where should I look for it?
>
>
> So I winder whether or not we can simply ignore IOTLB message if vhost
> device is not started.
>
Do you mean like an early return in vhost_device_iotlb_miss? That was
my first first option, but this seems cleaner to me. I'm ok with both
options.
Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ?
Thanks!
> Thanks
>
>
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>
> >>> Reproduced rebooting a guest with testpmd in txonly forward mode.
> >>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
> >>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
> >>> at ../hw/virtio/vhost.c:1013
> >>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
> >>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
> >>> at ../hw/virtio/vhost-backend.c:411
> >>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
> >>> imsg=imsg@entry=0x7ffddcfd32c0)
> >>> at ../hw/virtio/vhost-backend.c:404
> >>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
> >>> at ../hw/virtio/vhost-user.c:1464
> >>> #4 0x000055a0000c541b in aio_dispatch_handler (
> >>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
> >>> at ../util/aio-posix.c:329
> >>>
> >>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>> hw/virtio/vhost-user.c | 10 ++++++++--
> >>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>> index 2fdd5daf74..a49b2229fb 100644
> >>> --- a/hw/virtio/vhost-user.c
> >>> +++ b/hw/virtio/vhost-user.c
> >>> @@ -238,6 +238,7 @@ struct vhost_user {
> >>> /* Shared between vhost devs of the same virtio device */
> >>> VhostUserState *user;
> >>> int slave_fd;
> >>> + bool iotlb_enabled;
> >>> NotifierWithReturn postcopy_notifier;
> >>> struct PostCopyFD postcopy_fd;
> >>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
> >>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
> >>>
> >>> switch (hdr.request) {
> >>> case VHOST_USER_SLAVE_IOTLB_MSG:
> >>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> >>> + if (likely(u->iotlb_enabled)) {
> >>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> >>> + } else {
> >>> + ret = -EFAULT;
> >>> + }
> >>> break;
> >>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
> >>> ret = vhost_user_slave_handle_config_change(dev);
> >>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
> >>>
> >>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> >>> {
> >>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */
> >>> + struct vhost_user *u = dev->opaque;
> >>> + u->iotlb_enabled = enabled;
> >>> }
> >>>
> >>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-29 7:22 ` Eugenio Perez Martin
@ 2021-01-29 7:29 ` Jason Wang
2021-01-29 9:12 ` Eugenio Perez Martin
0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2021-01-29 7:29 UTC (permalink / raw)
To: Eugenio Perez Martin; +Cc: Maxime Coquelin, qemu-level, Michael S. Tsirkin
On 2021/1/29 下午3:22, Eugenio Perez Martin wrote:
> On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/1/28 下午5:37, Eugenio Perez Martin wrote:
>>> Hi Jason.
>>>
>>> On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2021/1/28 上午4:44, Eugenio Pérez wrote:
>>>>> Not registering this can lead to vhost_backend_handle_iotlb_msg and
>>>>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
>>>>> stop.
>>>>>
>>>>> This causes a try to access dev->vdev->dma_as with vdev == NULL.
>>>> Hi Eugenio:
>>>>
>>>> What condition can we get this? Did you mean we receive IOTLB_MISS
>>>> before vhost_dev_start()?
>>>>
>>> In the VM reboot case, yes, between vhost_dev_stop() and
>>> vhost_dev_start(). But I can reproduce the bug in VM shutdown too,
>>> with no vhost_dev_start expected.
>>>
>>> I didn't try to send IOTLB_MISS before starting vhost_dev, but this
>>> patch should solve that problem too.
>>>
>>> I think we can get this with whatever malicious/buggy vhost-user
>>> backend sending unexpected iotlb misses, but I didn't test so deeply.
>>
>> I see.
>>
>>
>>>> If yes, it looks to me a bug somewhere else.
>>> Where should I look for it?
>>
>> So I winder whether or not we can simply ignore IOTLB message if vhost
>> device is not started.
>>
> Do you mean like an early return in vhost_device_iotlb_miss?
Yes or probably a little bit earlier in vhost_backend_handle_iotlb_msg()
which somehow a warn there.
Anyhow it's meaningless to process IOTLB message in this case and we
don't need to introduce a dedicated variable for this.
Thanks
> That was
> my first first option, but this seems cleaner to me. I'm ok with both
> options.
>
> Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ?
>
> Thanks!
>
>> Thanks
>>
>>
>>> Thanks!
>>>
>>>> Thanks
>>>>
>>>>
>>>>> Reproduced rebooting a guest with testpmd in txonly forward mode.
>>>>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
>>>>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
>>>>> at ../hw/virtio/vhost.c:1013
>>>>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
>>>>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
>>>>> at ../hw/virtio/vhost-backend.c:411
>>>>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
>>>>> imsg=imsg@entry=0x7ffddcfd32c0)
>>>>> at ../hw/virtio/vhost-backend.c:404
>>>>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
>>>>> at ../hw/virtio/vhost-user.c:1464
>>>>> #4 0x000055a0000c541b in aio_dispatch_handler (
>>>>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
>>>>> at ../util/aio-posix.c:329
>>>>>
>>>>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>> hw/virtio/vhost-user.c | 10 ++++++++--
>>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>>> index 2fdd5daf74..a49b2229fb 100644
>>>>> --- a/hw/virtio/vhost-user.c
>>>>> +++ b/hw/virtio/vhost-user.c
>>>>> @@ -238,6 +238,7 @@ struct vhost_user {
>>>>> /* Shared between vhost devs of the same virtio device */
>>>>> VhostUserState *user;
>>>>> int slave_fd;
>>>>> + bool iotlb_enabled;
>>>>> NotifierWithReturn postcopy_notifier;
>>>>> struct PostCopyFD postcopy_fd;
>>>>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
>>>>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
>>>>>
>>>>> switch (hdr.request) {
>>>>> case VHOST_USER_SLAVE_IOTLB_MSG:
>>>>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
>>>>> + if (likely(u->iotlb_enabled)) {
>>>>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
>>>>> + } else {
>>>>> + ret = -EFAULT;
>>>>> + }
>>>>> break;
>>>>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
>>>>> ret = vhost_user_slave_handle_config_change(dev);
>>>>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
>>>>>
>>>>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
>>>>> {
>>>>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */
>>>>> + struct vhost_user *u = dev->opaque;
>>>>> + u->iotlb_enabled = enabled;
>>>>> }
>>>>>
>>>>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
2021-01-29 7:29 ` Jason Wang
@ 2021-01-29 9:12 ` Eugenio Perez Martin
0 siblings, 0 replies; 7+ messages in thread
From: Eugenio Perez Martin @ 2021-01-29 9:12 UTC (permalink / raw)
To: Jason Wang; +Cc: Maxime Coquelin, qemu-level, Michael S. Tsirkin
On Fri, Jan 29, 2021 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/1/29 下午3:22, Eugenio Perez Martin wrote:
> > On Fri, Jan 29, 2021 at 4:29 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/1/28 下午5:37, Eugenio Perez Martin wrote:
> >>> Hi Jason.
> >>>
> >>> On Thu, Jan 28, 2021 at 3:32 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2021/1/28 上午4:44, Eugenio Pérez wrote:
> >>>>> Not registering this can lead to vhost_backend_handle_iotlb_msg and
> >>>>> vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
> >>>>> stop.
> >>>>>
> >>>>> This causes a try to access dev->vdev->dma_as with vdev == NULL.
> >>>> Hi Eugenio:
> >>>>
> >>>> What condition can we get this? Did you mean we receive IOTLB_MISS
> >>>> before vhost_dev_start()?
> >>>>
> >>> In the VM reboot case, yes, between vhost_dev_stop() and
> >>> vhost_dev_start(). But I can reproduce the bug in VM shutdown too,
> >>> with no vhost_dev_start expected.
> >>>
> >>> I didn't try to send IOTLB_MISS before starting vhost_dev, but this
> >>> patch should solve that problem too.
> >>>
> >>> I think we can get this with whatever malicious/buggy vhost-user
> >>> backend sending unexpected iotlb misses, but I didn't test so deeply.
> >>
> >> I see.
> >>
> >>
> >>>> If yes, it looks to me a bug somewhere else.
> >>> Where should I look for it?
> >>
> >> So I winder whether or not we can simply ignore IOTLB message if vhost
> >> device is not started.
> >>
> > Do you mean like an early return in vhost_device_iotlb_miss?
>
>
> Yes or probably a little bit earlier in vhost_backend_handle_iotlb_msg()
> which somehow a warn there.
>
> Anyhow it's meaningless to process IOTLB message in this case and we
> don't need to introduce a dedicated variable for this.
>
Start a new series, since the patch title does not reflect the changes anymore:
https://patchew.org/QEMU/20210129090728.831208-1-eperezma@redhat.com/
Thanks!
> Thanks
>
>
> > That was
> > my first first option, but this seems cleaner to me. I'm ok with both
> > options.
> >
> > Or do you mean not to return -EFAULT but 0 if !u->iotlb_enabled ?
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>
> >>> Thanks!
> >>>
> >>>> Thanks
> >>>>
> >>>>
> >>>>> Reproduced rebooting a guest with testpmd in txonly forward mode.
> >>>>> #0 0x0000559ffff94394 in vhost_device_iotlb_miss (
> >>>>> dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
> >>>>> at ../hw/virtio/vhost.c:1013
> >>>>> #1 0x0000559ffff9ac31 in vhost_backend_handle_iotlb_msg (
> >>>>> imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
> >>>>> at ../hw/virtio/vhost-backend.c:411
> >>>>> #2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
> >>>>> imsg=imsg@entry=0x7ffddcfd32c0)
> >>>>> at ../hw/virtio/vhost-backend.c:404
> >>>>> #3 0x0000559fffeded7b in slave_read (opaque=0x55a0012f6680)
> >>>>> at ../hw/virtio/vhost-user.c:1464
> >>>>> #4 0x000055a0000c541b in aio_dispatch_handler (
> >>>>> ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
> >>>>> at ../util/aio-posix.c:329
> >>>>>
> >>>>> Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
> >>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>> ---
> >>>>> hw/virtio/vhost-user.c | 10 ++++++++--
> >>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>>>> index 2fdd5daf74..a49b2229fb 100644
> >>>>> --- a/hw/virtio/vhost-user.c
> >>>>> +++ b/hw/virtio/vhost-user.c
> >>>>> @@ -238,6 +238,7 @@ struct vhost_user {
> >>>>> /* Shared between vhost devs of the same virtio device */
> >>>>> VhostUserState *user;
> >>>>> int slave_fd;
> >>>>> + bool iotlb_enabled;
> >>>>> NotifierWithReturn postcopy_notifier;
> >>>>> struct PostCopyFD postcopy_fd;
> >>>>> uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
> >>>>> @@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
> >>>>>
> >>>>> switch (hdr.request) {
> >>>>> case VHOST_USER_SLAVE_IOTLB_MSG:
> >>>>> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> >>>>> + if (likely(u->iotlb_enabled)) {
> >>>>> + ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> >>>>> + } else {
> >>>>> + ret = -EFAULT;
> >>>>> + }
> >>>>> break;
> >>>>> case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
> >>>>> ret = vhost_user_slave_handle_config_change(dev);
> >>>>> @@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
> >>>>>
> >>>>> static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> >>>>> {
> >>>>> - /* No-op as the receive channel is not dedicated to IOTLB messages. */
> >>>>> + struct vhost_user *u = dev->opaque;
> >>>>> + u->iotlb_enabled = enabled;
> >>>>> }
> >>>>>
> >>>>> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-29 9:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 20:44 [PATCH] vhost-user: Check for iotlb callback in iotlb_miss Eugenio Pérez
2021-01-28 2:32 ` Jason Wang
2021-01-28 9:37 ` Eugenio Perez Martin
2021-01-29 3:29 ` Jason Wang
2021-01-29 7:22 ` Eugenio Perez Martin
2021-01-29 7:29 ` Jason Wang
2021-01-29 9:12 ` Eugenio Perez Martin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).