Message-ID: <cc771916-62fe-4f6b-88d2-9c17dff65523@nvidia.com>
Date: Mon, 5 Aug 2024 17:58:50 +0200
From: Dragos Tatulea <dtatulea@...dia.com>
To: Jason Wang <jasowang@...hat.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "mst@...hat.com" <mst@...hat.com>, "eperezma@...hat.com"
 <eperezma@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH vhost] vhost-vdpa: Fix invalid irq bypass unregister

On 05.08.24 05:17, Jason Wang wrote:
> On Fri, Aug 2, 2024 at 2:51 PM Dragos Tatulea <dtatulea@...dia.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@...dia.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().
That value is the eventfd ctx, but it could be anything else really...


> 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?

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@...dia.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
>>
> 

Powered by blists - more mailing lists