1. Patchew
  2. QEMU
  3. View series

[PATCH] vhost-user: Check for iotlb callback in iotlb_miss

Eugenio Pérez posted 1 patch 3 years, 3 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210127204449.745365-1-eperezma@redhat.com
hw/virtio/vhost-user.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Eugenio Pérez 3 years, 3 months ago 
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
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Jason Wang 3 years, 3 months ago 
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,
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Eugenio Perez Martin 3 years, 3 months ago 
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,
>
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Jason Wang 3 years, 3 months ago 
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,
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Eugenio Perez Martin 3 years, 3 months ago 
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,
>
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Jason Wang 3 years, 3 months ago 
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,
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
Posted by Eugenio Perez Martin 3 years, 3 months ago 
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,
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.