[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
From: |
Eugenio Perez Martin |
Subject: |
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss |
Date: |
Fri, 29 Jan 2021 10:12:58 +0100 |
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:
20210129090728.831208-1-eperezma@redhat.com/">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,
>