netdev - Re: [RFC PATCH] vhost_vdpa: assign irq bypass producer token correctly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ede5a20f-0314-4281-9100-89a265ff6411@nvidia.com>
Date: Mon, 12 Aug 2024 13:21:54 +0200
From: Dragos Tatulea <dtatulea@...dia.com>
To: Jason Wang <jasowang@...hat.com>
Cc: mst@...hat.com, lingshan.zhu@...el.com, kvm@...r.kernel.org,
 virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] vhost_vdpa: assign irq bypass producer token
 correctly



On 12.08.24 08:49, Jason Wang wrote:
> On Mon, Aug 12, 2024 at 1:47 PM Jason Wang <jasowang@...hat.com> wrote:
>>
>> On Fri, Aug 9, 2024 at 2:04 AM Dragos Tatulea <dtatulea@...dia.com> wrote:
>>>
>>>
>>>
>>> On 08.08.24 10:20, Jason Wang wrote:
>>>> We used to call irq_bypass_unregister_producer() in
>>>> vhost_vdpa_setup_vq_irq() which is problematic as we don't know if the
>>>> token pointer is still valid or not.
>>>>
>>>> Actually, we use the eventfd_ctx as the token so the life cycle of the
>>>> token should be bound to the VHOST_SET_VRING_CALL instead of
>>>> vhost_vdpa_setup_vq_irq() which could be called by set_status().
>>>>
>>>> Fixing this by setting up  irq bypass producer's token when handling
>>>> VHOST_SET_VRING_CALL and un-registering the producer before calling
>>>> vhost_vring_ioctl() to prevent a possible use after free as eventfd
>>>> could have been released in vhost_vring_ioctl().
>>>>
>>>> Fixes: 2cf1ba9a4d15 ("vhost_vdpa: implement IRQ offloading in vhost_vdpa")
>>>> Signed-off-by: Jason Wang <jasowang@...hat.com>
>>>> ---
>>>> Note for Dragos: Please check whether this fixes your issue. I
>>>> slightly test it with vp_vdpa in L2.
>>>> ---
>>>>  drivers/vhost/vdpa.c | 12 +++++++++---
>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>> index e31ec9ebc4ce..388226a48bcc 100644
>>>> --- a/drivers/vhost/vdpa.c
>>>> +++ b/drivers/vhost/vdpa.c
>>>> @@ -209,11 +209,9 @@ static void vhost_vdpa_setup_vq_irq(struct vhost_vdpa *v, u16 qid)
>>>>       if (irq < 0)
>>>>               return;
>>>>
>>>> -     irq_bypass_unregister_producer(&vq->call_ctx.producer);
>>>>       if (!vq->call_ctx.ctx)
>>>>               return;
>>>>
>>>> -     vq->call_ctx.producer.token = vq->call_ctx.ctx;
>>>>       vq->call_ctx.producer.irq = irq;
>>>>       ret = irq_bypass_register_producer(&vq->call_ctx.producer);
>>>>       if (unlikely(ret))
>>>> @@ -709,6 +707,12 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>>>>                       vq->last_avail_idx = vq_state.split.avail_index;
>>>>               }
>>>>               break;
>>>> +     case VHOST_SET_VRING_CALL:
>>>> +             if (vq->call_ctx.ctx) {
>>>> +                     vhost_vdpa_unsetup_vq_irq(v, idx);
>>>> +                     vq->call_ctx.producer.token = NULL;
>>>> +             }
>>>> +             break;
>>>>       }
>>>>
>>>>       r = vhost_vring_ioctl(&v->vdev, cmd, argp);
>>>> @@ -747,13 +751,14 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>>>>                       cb.callback = vhost_vdpa_virtqueue_cb;
>>>>                       cb.private = vq;
>>>>                       cb.trigger = vq->call_ctx.ctx;
>>>> +                     vq->call_ctx.producer.token = vq->call_ctx.ctx;
>>>> +                     vhost_vdpa_setup_vq_irq(v, idx);
>>>>               } else {
>>>>                       cb.callback = NULL;
>>>>                       cb.private = NULL;
>>>>                       cb.trigger = NULL;
>>>>               }
>>>>               ops->set_vq_cb(vdpa, idx, &cb);
>>>> -             vhost_vdpa_setup_vq_irq(v, idx);
>>>>               break;
>>>>
>>>>       case VHOST_SET_VRING_NUM:
>>>> @@ -1419,6 +1424,7 @@ static int vhost_vdpa_open(struct inode *inode, struct file *filep)
>>>>       for (i = 0; i < nvqs; i++) {
>>>>               vqs[i] = &v->vqs[i];
>>>>               vqs[i]->handle_kick = handle_vq_kick;
>>>> +             vqs[i]->call_ctx.ctx = NULL;
>>>>       }
>>>>       vhost_dev_init(dev, vqs, nvqs, 0, 0, 0, false,
>>>>                      vhost_vdpa_process_iotlb_msg);
>>>
>>> No more crashes, but now getting a lot of:
>>>  vhost-vdpa-X: vq Y, irq bypass producer (token 00000000a66e28ab) registration fails, ret =  -16
>>>
>>> ... seems like the irq_bypass_unregister_producer() that was removed
>>> might still be needed somewhere?
>>
My statement above was not quite correct. The error comes from the
VQ irq being registered twice:

1) VHOST_SET_VRING_CALL ioctl gets called for vq 0. VQ irq is unregistered
   (vhost_vdpa_unsetup_vq_irq() and re-registered (vhost_vdpa_setup_vq_irq())
   successfully. So far so good.

2) set status !DRIVER_OK -> DRIVER_OK happens. VQ irq setup is done
   once again (vhost_vdpa_setup_vq_irq()). As the producer unregister
   was removed in this patch, the register will complain because the producer
   token already exists.


>> Probably, but I didn't see this when testing vp_vdpa.
>>
>> When did you meet those warnings? Is it during the boot or migration?
During boot, on the first 2 VQs only (so before the QPs are resized).
Traffic does work though when the VM is booted.

> 
> Btw, it would be helpful to check if mlx5_get_vq_irq() works
> correctly. I believe it should return an error if the virtqueue
> interrupt is not allocated. After a glance at the code, it seems not
> straightforward to me.
> 
I think we're good on that front:
mlx5_get_vq_irq() returns EOPNOTSUPP if the vq irq is not allocated.


Thanks,
Dragos