Re: [RFC PATCH vhost] vhost-vdpa: Fix invalid irq bypass unregister

[Dragos Tatulea <dtatulea@nvidia.com>]

(Re-sending. I messed up the previous message, sorry about that.)

On 06.08.24 04:57, Jason Wang wrote:
> On Mon, Aug 5, 2024 at 11:59 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>
>> On 05.08.24 05:17, Jason Wang wrote:
>>> On Fri, Aug 2, 2024 at 2:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>>
>>>> On Fri, 2024-08-02 at 11:29 +0800, Jason Wang wrote:
>>>>> On Thu, Aug 1, 2024 at 11:38 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>>>>
>>>>>> The following workflow triggers the crash referenced below:
>>>>>>
>>>>>> 1) vhost_vdpa_unsetup_vq_irq() unregisters the irq bypass producer
>>>>>>    but the producer->token is still valid.
>>>>>> 2) vq context gets released and reassigned to another vq.
>>>>>
>>>>> Just to make sure I understand here, which structure is referred to as
>>>>> "vq context" here? I guess it's not call_ctx as it is a part of the vq
>>>>> itself.
>>>>>
>>>>>> 3) That other vq registers it's producer with the same vq context
>>>>>>    pointer as token in vhost_vdpa_setup_vq_irq().
>>>>>
>>>>> Or did you mean when a single eventfd is shared among different vqs?
>>>>>
>>>> Yes, that's what I mean: vq->call_ctx.ctx which is a eventfd_ctx.
>>>>
>>>> But I don't think it's shared in this case, only that the old eventfd_ctx value
>>>> is lingering in producer->token. And this old eventfd_ctx is assigned now to
>>>> another vq.
>>>
>>> Just to make sure I understand the issue. The eventfd_ctx should be
>>> still valid until a new VHOST_SET_VRING_CALL().
>>>
>> I think it's not about the validity of the eventfd_ctx. More about
>> the lingering ctx value of the producer after vhost_vdpa_unsetup_vq_irq().
> 
> Probably, but
> 
>> That value is the eventfd ctx, but it could be anything else really...
> 
> I mean we hold a refcnt of the eventfd so it should be valid until the
> next set_vring_call() or vhost_dev_cleanup().
> 
> But I do spot some possible issue:
> 
> 1) We swap and assign new ctx in vhost_vring_ioctl():
> 
>                 swap(ctx, vq->call_ctx.ctx);
> 
> 2) and old ctx will be put there as well:
> 
>                 if (!IS_ERR_OR_NULL(ctx))
>                         eventfd_ctx_put(ctx);
> 
> 3) but in vdpa, we try to unregister the producer with the new token:
> 
> static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>                            void __user *argp)
> {
> ...
>         r = vhost_vring_ioctl(&v->vdev, cmd, argp);
> ...
>         switch (cmd) {
> ...
>         case VHOST_SET_VRING_CALL:
>                 if (vq->call_ctx.ctx) {
>                         cb.callback = vhost_vdpa_virtqueue_cb;
>                         cb.private = vq;
>                         cb.trigger = vq->call_ctx.ctx;
>                 } else {
>                         cb.callback = NULL;
>                         cb.private = NULL;
>                         cb.trigger = NULL;
>                 }
>                 ops->set_vq_cb(vdpa, idx, &cb);
>                 vhost_vdpa_setup_vq_irq(v, idx);
> 
> in vhost_vdpa_setup_vq_irq() we had:
> 
>         irq_bypass_unregister_producer(&vq->call_ctx.producer);
> 
> here the producer->token still points to the old one...
> 
> Is this what you have seen?
Yup. That is the issue. The unregister already happened at
vhost_vdpa_unsetup_vq_irq(). So this second unregister will
work on an already unregistered element due to the token still
being set.

> 
>>
>>
>>> I may miss something but the only way to assign exactly the same
>>> eventfd_ctx value to another vq is where the guest tries to share the
>>> MSI-X vector among virtqueues, then qemu will use a single eventfd as
>>> the callback for multiple virtqueues. If this is true:
>>>
>> I don't think this is the case. I see the issue happening when running qemu vdpa
>> live migration tests on the same host. From a vdpa device it's basically a device
>> starting on a VM over and over.
>>
>>> For bypass registering, only the first registering can succeed as the
>>> following registering will fail because the irq bypass manager already
>>> had exactly the same producer token.
>>> For registering, all unregistering can succeed:
>>>
>>> 1) the first unregistering will do the real job that unregister the token
>>> 2) the following unregistering will do nothing by iterating the
>>> producer token list without finding a match one
>>>
>>> Maybe you can show me the userspace behaviour (ioctls) when you see this?
>>>
>> Sure, what would you need? qemu traces?
> 
> Yes, that would be helpful.
> 
Will try to get them.

Thanks,
Dragos
> Thanks
> 
>>
>> Thanks,
>> Dragos
>>
>>> Thanks
>>>
>>>>
>>>>>> 4) The original vq tries to unregister it's producer which it has
>>>>>>    already unlinked in step 1. irq_bypass_unregister_producer() will go
>>>>>>    ahead and unlink the producer once again. That happens because:
>>>>>>       a) The producer has a token.
>>>>>>       b) An element with that token is found. But that element comes
>>>>>>          from step 3.
>>>>>>
>>>>>> I see 3 ways to fix this:
>>>>>> 1) Fix the vhost-vdpa part. What this patch does. vfio has a different
>>>>>>    workflow.
>>>>>> 2) Set the token to NULL directly in irq_bypass_unregister_producer()
>>>>>>    after unlinking the producer. But that makes the API asymmetrical.
>>>>>> 3) Make irq_bypass_unregister_producer() also compare the pointer
>>>>>>    elements not just the tokens and do the unlink only on match.
>>>>>>
>>>>>> Any thoughts?
>>>>>>
>>>>>> Oops: general protection fault, probably for non-canonical address 0xdead000000000108: 0000 [#1] SMP
>>>>>> CPU: 8 PID: 5190 Comm: qemu-system-x86 Not tainted 6.10.0-rc7+ #6
>>>>>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
>>>>>> RIP: 0010:irq_bypass_unregister_producer+0xa5/0xd0
>>>>>> RSP: 0018:ffffc900034d7e50 EFLAGS: 00010246
>>>>>> RAX: dead000000000122 RBX: ffff888353d12718 RCX: ffff88810336a000
>>>>>> RDX: dead000000000100 RSI: ffffffff829243a0 RDI: 0000000000000000
>>>>>> RBP: ffff888353c42000 R08: ffff888104882738 R09: ffff88810336a000
>>>>>> R10: ffff888448ab2050 R11: 0000000000000000 R12: ffff888353d126a0
>>>>>> R13: 0000000000000004 R14: 0000000000000055 R15: 0000000000000004
>>>>>> FS:  00007f9df9403c80(0000) GS:ffff88852cc00000(0000) knlGS:0000000000000000
>>>>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> CR2: 0000562dffc6b568 CR3: 000000012efbb006 CR4: 0000000000772ef0
>>>>>> PKRU: 55555554
>>>>>> Call Trace:
>>>>>>  <TASK>
>>>>>>  ? die_addr+0x36/0x90
>>>>>>  ? exc_general_protection+0x1a8/0x390
>>>>>>  ? asm_exc_general_protection+0x26/0x30
>>>>>>  ? irq_bypass_unregister_producer+0xa5/0xd0
>>>>>>  vhost_vdpa_setup_vq_irq+0x5a/0xc0 [vhost_vdpa]
>>>>>>  vhost_vdpa_unlocked_ioctl+0xdcd/0xe00 [vhost_vdpa]
>>>>>>  ? vhost_vdpa_config_cb+0x30/0x30 [vhost_vdpa]
>>>>>>  __x64_sys_ioctl+0x90/0xc0
>>>>>>  do_syscall_64+0x4f/0x110
>>>>>>  entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>> RIP: 0033:0x7f9df930774f
>>>>>> RSP: 002b:00007ffc55013080 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>>>>>> RAX: ffffffffffffffda RBX: 0000562dfe134d20 RCX: 00007f9df930774f
>>>>>> RDX: 00007ffc55013200 RSI: 000000004008af21 RDI: 0000000000000011
>>>>>> RBP: 00007ffc55013200 R08: 0000000000000002 R09: 0000000000000000
>>>>>> R10: 0000000000000000 R11: 0000000000000246 R12: 0000562dfe134360
>>>>>> R13: 0000562dfe134d20 R14: 0000000000000000 R15: 00007f9df801e190
>>>>>>
>>>>>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>>>>>> ---
>>>>>>  drivers/vhost/vdpa.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>>>> index 478cd46a49ed..d4a7a3918d86 100644
>>>>>> --- a/drivers/vhost/vdpa.c
>>>>>> +++ b/drivers/vhost/vdpa.c
>>>>>> @@ -226,6 +226,7 @@ static void vhost_vdpa_unsetup_vq_irq(struct vhost_vdpa *v, u16 qid)
>>>>>>         struct vhost_virtqueue *vq = &v->vqs[qid];
>>>>>>
>>>>>>         irq_bypass_unregister_producer(&vq->call_ctx.producer);
>>>>>> +       vq->call_ctx.producer.token = NULL;
>>>>>>  }
>>>>>>
>>>>>>  static int _compat_vdpa_reset(struct vhost_vdpa *v)
>>>>>> --
>>>>>> 2.45.2
>>>>>>
>>>>>
>>>> Thanks
>>>>
>>>
>>
>