On 13.08.24 08:26, Jason Wang wrote:
> On Mon, Aug 12, 2024 at 7:22 PM Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
>>
>>
>>
>> On 12.08.24 08:49, Jason Wang wrote:
>>> On Mon, Aug 12, 2024 at 1:47 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>>>>
>>>> On Fri, Aug 9, 2024 at 2:04 AM Dragos Tatulea <dtatulea@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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.
>
> I see. I think it's probably too tricky to try to register/unregister
> a producer during set_vring_call if DRIVER_OK is not set.
>
> Does it work if we only do vhost_vdpa_unsetup/setup_vq_irq() if
> DRIVER_OK is set in vhost_vdpa_vring_ioctl() (on top of this patch)?
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 388226a48bcc..ab441b8ccd2e 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -709,7 +709,9 @@ static long vhost_vdpa_vring_ioctl(struct
> vhost_vdpa *v, unsigned int cmd,
> break;
> case VHOST_SET_VRING_CALL:
> if (vq->call_ctx.ctx) {
> - vhost_vdpa_unsetup_vq_irq(v, idx);
> + if (ops->get_status(vdpa) &
> + VIRTIO_CONFIG_S_DRIVER_OK)
> + vhost_vdpa_unsetup_vq_irq(v, idx);
> vq->call_ctx.producer.token = NULL;
I was wondering if it's safe to set NULL also for !DRIVER_OK case,
but then I noticed that the !DRIVER_OK transition doesn't set .token to
NULL so this is actually beneficial. Did I get it right?
> }
> break;
> @@ -752,7 +754,9 @@ static long vhost_vdpa_vring_ioctl(struct
> vhost_vdpa *v, unsigned int cmd,
> 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);
> + if (ops->get_status(vdpa) &
> + VIRTIO_CONFIG_S_DRIVER_OK)
> + vhost_vdpa_setup_vq_irq(v, idx);
> } else {
> cb.callback = NULL;
> cb.private = NULL;
>
Yup, this works.
I do understand the fix, but I don't fully understand why this is
better than setting the .token to NULL in vhost_vdpa_unsetup_vq_irq()
and keeping the token logic inside the vhost_vdpa_setup/unsetup_vq_irq()
API.
In any case, if you send this fix:
Tested-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
Thanks,
Dragos
>>
>>
>>>> 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.
>
> Right.
>
>>
>>>
>>> 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.
>
> Good to know that.
>
> Thanks
>
>>
>>
>> Thanks,
>> Dragos
>>
>
