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 pointer, so we can now hit the bug above. This patch embeds the vhost_worker in the vhost_dev and moves the work list initialization back to vhost_dev_init, so we can just check the worker.vtsk pointer to check if VHOST_SET_OWNER has been done like before.
Unified · Split
| @@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) · 234 unmodified lines | |||
| 235 | { | 235 | { |
| 236 | struct vhost_flush_struct flush; | 236 | struct vhost_flush_struct flush; |
| 237 | 237 | ||
| 238 | if (dev->worker) { | 238 | if (dev->worker.vtsk) { |
| 239 | init_completion(&flush.wait_event); | 239 | init_completion(&flush.wait_event); |
| 240 | vhost_work_init(&flush.work, vhost_flush_work); | 240 | vhost_work_init(&flush.work, vhost_flush_work); |
| 241 | 241 | ||
| @@ -247,7 +247,7 @@ EXPORT_SYMBOL_GPL(vhost_dev_flush); · 5 unmodified lines | |||
| 247 | 247 | ||
| 248 | void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) | 248 | void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) |
| 249 | { | 249 | { |
| 250 | if (!dev->worker) | 250 | if (!dev->worker.vtsk) |
| 251 | return; | 251 | return; |
| 252 | 252 | ||
| 253 | if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) { | 253 | if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) { |
| @@ -255,8 +255,8 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) · 1 unmodified line | |||
| 255 | * sure it was not in the list. | 255 | * sure it was not in the list. |
| 256 | * test_and_set_bit() implies a memory barrier. | 256 | * test_and_set_bit() implies a memory barrier. |
| 257 | */ | 257 | */ |
| 258 | llist_add(&work->node, &dev->worker->work_list); | 258 | llist_add(&work->node, &dev->worker.work_list); |
| 259 | vhost_task_wake(dev->worker->vtsk); | 259 | vhost_task_wake(dev->worker.vtsk); |
| 260 | } | 260 | } |
| 261 | } | 261 | } |
| 262 | EXPORT_SYMBOL_GPL(vhost_work_queue); | 262 | EXPORT_SYMBOL_GPL(vhost_work_queue); |
| @@ -264,7 +264,7 @@ EXPORT_SYMBOL_GPL(vhost_work_queue); · 1 unmodified line | |||
| 264 | /* A lockless hint for busy polling code to exit the loop */ | 264 | /* A lockless hint for busy polling code to exit the loop */ |
| 265 | bool vhost_has_work(struct vhost_dev *dev) | 265 | bool vhost_has_work(struct vhost_dev *dev) |
| 266 | { | 266 | { |
| 267 | return dev->worker && !llist_empty(&dev->worker->work_list); | 267 | return !llist_empty(&dev->worker.work_list); |
| 268 | } | 268 | } |
| 269 | EXPORT_SYMBOL_GPL(vhost_has_work); | 269 | EXPORT_SYMBOL_GPL(vhost_has_work); |
| 270 | 270 | ||
| @@ -456,7 +456,8 @@ void vhost_dev_init(struct vhost_dev *dev, · 185 unmodified lines | |||
| 456 | dev->umem = NULL; | 456 | dev->umem = NULL; |
| 457 | dev->iotlb = NULL; | 457 | dev->iotlb = NULL; |
| 458 | dev->mm = NULL; | 458 | dev->mm = NULL; |
| 459 | dev->worker = NULL; | 459 | memset(&dev->worker, 0, sizeof(dev->worker)); |
| 460 | init_llist_head(&dev->worker.work_list); | ||
| 460 | dev->iov_limit = iov_limit; | 461 | dev->iov_limit = iov_limit; |
| 461 | dev->weight = weight; | 462 | dev->weight = weight; |
| 462 | dev->byte_weight = byte_weight; | 463 | dev->byte_weight = byte_weight; |
| @@ -530,47 +531,30 @@ static void vhost_detach_mm(struct vhost_dev *dev) · 67 unmodified lines | |||
| 530 | 531 | ||
| 531 | static void vhost_worker_free(struct vhost_dev *dev) | 532 | static void vhost_worker_free(struct vhost_dev *dev) |
| 532 | { | 533 | { |
| 533 | struct vhost_worker *worker = dev->worker; | 534 | if (!dev->worker.vtsk) |
| 534 | |||
| 535 | if (!worker) | ||
| 536 | return; | 535 | return; |
| 537 | 536 | ||
| 538 | dev->worker = NULL; | 537 | WARN_ON(!llist_empty(&dev->worker.work_list)); |
| 539 | WARN_ON(!llist_empty(&worker->work_list)); | 538 | vhost_task_stop(dev->worker.vtsk); |
| 540 | vhost_task_stop(worker->vtsk); | 539 | dev->worker.kcov_handle = 0; |
| 541 | kfree(worker); | 540 | dev->worker.vtsk = NULL; |
| 542 | } | 541 | } |
| 543 | 542 | ||
| 544 | static int vhost_worker_create(struct vhost_dev *dev) | 543 | static int vhost_worker_create(struct vhost_dev *dev) |
| 545 | { | 544 | { |
| 546 | struct vhost_worker *worker; | ||
| 547 | struct vhost_task *vtsk; | 545 | struct vhost_task *vtsk; |
| 548 | char name[TASK_COMM_LEN]; | 546 | char name[TASK_COMM_LEN]; |
| 549 | int ret; | ||
| 550 | |||
| 551 | worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT); | ||
| 552 | if (!worker) | ||
| 553 | return -ENOMEM; | ||
| 554 | 547 | ||
| 555 | dev->worker = worker; | ||
| 556 | worker->kcov_handle = kcov_common_handle(); | ||
| 557 | init_llist_head(&worker->work_list); | ||
| 558 | snprintf(name, sizeof(name), "vhost-%d", current->pid); | 548 | snprintf(name, sizeof(name), "vhost-%d", current->pid); |
| 559 | 549 | ||
| 560 | vtsk = vhost_task_create(vhost_worker, worker, name); | 550 | vtsk = vhost_task_create(vhost_worker, &dev->worker, name); |
| 561 | if (!vtsk) { | 551 | if (!vtsk) |
| 562 | ret = -ENOMEM; | 552 | return -ENOMEM; |
| 563 | goto free_worker; | ||
| 564 | } | ||
| 565 | 553 | ||
| 566 | worker->vtsk = vtsk; | 554 | dev->worker.kcov_handle = kcov_common_handle(); |
| 555 | dev->worker.vtsk = vtsk; | ||
| 567 | vhost_task_start(vtsk); | 556 | vhost_task_start(vtsk); |
| 568 | return 0; | 557 | return 0; |
| 569 | |||
| 570 | free_worker: | ||
| 571 | kfree(worker); | ||
| 572 | dev->worker = NULL; | ||
| 573 | return ret; | ||
| 574 | } | 558 | } |
| 575 | 559 | ||
| 576 | /* Caller should have device mutex */ | 560 | /* Caller should have device mutex */ |
| @@ -154,7 +154,7 @@ struct vhost_dev { · 153 unmodified lines | |||
| 154 | struct vhost_virtqueue **vqs; | 154 | struct vhost_virtqueue **vqs; |
| 155 | int nvqs; | 155 | int nvqs; |
| 156 | struct eventfd_ctx *log_ctx; | 156 | struct eventfd_ctx *log_ctx; |
| 157 | struct vhost_worker *worker; | 157 | struct vhost_worker worker; |
| 158 | struct vhost_iotlb *umem; | 158 | struct vhost_iotlb *umem; |
| 159 | struct vhost_iotlb *iotlb; | 159 | struct vhost_iotlb *iotlb; |
| 160 | spinlock_t iotlb_lock; | 160 | spinlock_t iotlb_lock; |