On 6/6/23 4:49 AM, Stefano Garzarella wrote:
> On Mon, Jun 05, 2023 at 01:57:30PM -0500, Mike Christie wrote:
>> If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we
>> can race where:
>> 1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue
>> 2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create.
>> 3. vhost_worker_create will set the dev->worker pointer before setting
>> the worker->vtsk pointer.
>> 4. thread0's vhost_work_queue will see the dev->worker pointer is
>> set and try to call vhost_task_wake using not yet set worker->vtsk
>> pointer.
>> 5. We then crash since vtsk is NULL.
>>
>> Before commit 6e890c5d5021 ("vhost: use vhost_tasks for worker
>> threads"), we only had the worker pointer so we could just check it to
>> see if VHOST_SET_OWNER has been done. After that commit we have the
>> vhost_worker and vhost_task pointers, so we can now hit the bug above.
>>
>> This patch embeds the vhost_worker in the vhost_dev, so we can just
>> check the worker.vtsk pointer to check if VHOST_SET_OWNER has been done
>> like before.
>>
>> Fixes: 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>
> We should add:
>
> Reported-by: syzbot+d0d442c22fa8db45ff0e@xxxxxxxxxxxxxxxxxxxxxxxxx
Ok. Will do.
>> - }
>> + vtsk = vhost_task_create(vhost_worker, &dev->worker, name);
>> + if (!vtsk)
>> + return -ENOMEM;
>>
>> - worker->vtsk = vtsk;
>> + dev->worker.kcov_handle = kcov_common_handle();
>> + dev->worker.vtsk = vtsk;
>
> vhost_work_queue() is called by vhost_transport_send_pkt() without
> holding vhost_dev.mutex (like vhost_poll_queue() in several places).
>
> If vhost_work_queue() finds dev->worker.vtsk not NULL, how can we
> be sure that for example `work_list` has been initialized?
>
> Maybe I'm overthinking since we didn't have this problem before or the
> race is really short that it never happened.
Yeah, I dropped the READ/WRITE_ONCE use because I didn't think we needed
it for the vhost_vsock_start case, and for the case you mentioned above, I
wondered the same thing as you but was not sure so I added the same
behavior as before. When I read memory-barriers.txt, it sounds like we've
been getting lucky.
I'll add back the READ/WRITE_ONCE for vtsk access since that's what we are
keying off as signaling that the worker is ready to be used. I didn't see
any type of perf hit when using it, and from the memory-barriers.txt doc
it sounds like that's what we should be doing.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
