Mike Christie
2023-May-31 15:15 UTC
[syzbot] [kvm?] [net?] [virt?] general protection fault in vhost_work_queue
On 5/31/23 2:27 AM, Stefano Garzarella wrote:> On Tue, May 30, 2023 at 6:30?PM <michael.christie at oracle.com> wrote: >> >> On 5/30/23 11:17 AM, Stefano Garzarella wrote: >>> On Tue, May 30, 2023 at 11:09:09AM -0500, Mike Christie wrote: >>>> On 5/30/23 11:00 AM, Stefano Garzarella wrote: >>>>> I think it is partially related to commit 6e890c5d5021 ("vhost: use >>>>> vhost_tasks for worker threads") and commit 1a5f8090c6de ("vhost: move >>>>> worker thread fields to new struct"). Maybe that commits just >>>>> highlighted the issue and it was already existing. >>>> >>>> See my mail about the crash. Agree with your analysis about worker->vtsk >>>> not being set yet. It's a bug from my commit where I should have not set >>>> it so early or I should be checking for >>>> >>>> if (dev->worker && worker->vtsk) >>>> >>>> instead of >>>> >>>> if (dev->worker) >>> >>> Yes, though, in my opinion the problem may persist depending on how the >>> instructions are reordered. >> >> Ah ok. >> >>> >>> Should we protect dev->worker() with an RCU to be safe? >> >> For those multiple worker patchsets Jason had asked me about supporting >> where we don't have a worker while we are swapping workers around. To do >> that I had added rcu around the dev->worker. I removed it in later patchsets >> because I didn't think anyone would use it. >> >> rcu would work for your case and for what Jason had requested. > > Yeah, so you already have some patches? > > Do you want to send it to solve this problem? >Yeah, I'll break them out and send them later today when I can retest rebased patches.
Mike Christie
2023-May-31 16:27 UTC
[syzbot] [kvm?] [net?] [virt?] general protection fault in vhost_work_queue
On 5/31/23 10:15 AM, Mike Christie wrote:>>> rcu would work for your case and for what Jason had requested. >> Yeah, so you already have some patches? >> >> Do you want to send it to solve this problem? >> > Yeah, I'll break them out and send them later today when I can retest > rebased patches. >Just one question. Do you core vhost developers consider RCU more complex or switching to READ_ONCE/WRITE_ONCE? I am asking because for this immediate regression fix we could just switch to the latter like below to just fix the crash if we think that is more simple. I think RCU is just a little more complex/invasive because it will have the extra synchronize_rcu calls. diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index a92af08e7864..03fd47a22a73 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) { struct vhost_flush_struct flush; - if (dev->worker) { + if (READ_ONCE(dev->worker.vtsk)) { init_completion(&flush.wait_event); vhost_work_init(&flush.work, vhost_flush_work); @@ -247,7 +247,9 @@ EXPORT_SYMBOL_GPL(vhost_dev_flush); void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) { - if (!dev->worker) + struct vhost_task *vtsk = READ_ONCE(dev->worker.vtsk); + + if (!vtsk) return; if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) { @@ -255,8 +257,8 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) * sure it was not in the list. * test_and_set_bit() implies a memory barrier. */ - llist_add(&work->node, &dev->worker->work_list); - wake_up_process(dev->worker->vtsk->task); + llist_add(&work->node, &dev->worker.work_list); + wake_up_process(vtsk->task); } } EXPORT_SYMBOL_GPL(vhost_work_queue); @@ -264,7 +266,7 @@ EXPORT_SYMBOL_GPL(vhost_work_queue); /* A lockless hint for busy polling code to exit the loop */ bool vhost_has_work(struct vhost_dev *dev) { - return dev->worker && !llist_empty(&dev->worker->work_list); + return !llist_empty(&dev->worker.work_list); } EXPORT_SYMBOL_GPL(vhost_has_work); @@ -468,7 +470,7 @@ void vhost_dev_init(struct vhost_dev *dev, dev->umem = NULL; dev->iotlb = NULL; dev->mm = NULL; - dev->worker = NULL; + memset(&dev->worker, 0, sizeof(dev->worker)); dev->iov_limit = iov_limit; dev->weight = weight; dev->byte_weight = byte_weight; @@ -542,46 +544,38 @@ static void vhost_detach_mm(struct vhost_dev *dev) static void vhost_worker_free(struct vhost_dev *dev) { - struct vhost_worker *worker = dev->worker; + struct vhost_task *vtsk = READ_ONCE(dev->worker.vtsk); - if (!worker) + if (!vtsk) return; - dev->worker = NULL; - WARN_ON(!llist_empty(&worker->work_list)); - vhost_task_stop(worker->vtsk); - kfree(worker); + vhost_task_stop(vtsk); + WARN_ON(!llist_empty(&dev->worker.work_list)); + WRITE_ONCE(dev->worker.vtsk, NULL); } static int vhost_worker_create(struct vhost_dev *dev) { - struct vhost_worker *worker; struct vhost_task *vtsk; char name[TASK_COMM_LEN]; int ret; - worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT); - if (!worker) - return -ENOMEM; - - dev->worker = worker; - worker->kcov_handle = kcov_common_handle(); - init_llist_head(&worker->work_list); + dev->worker.kcov_handle = kcov_common_handle(); + init_llist_head(&dev->worker.work_list); snprintf(name, sizeof(name), "vhost-%d", current->pid); - vtsk = vhost_task_create(vhost_worker, worker, name); + vtsk = vhost_task_create(vhost_worker, &dev->worker, name); if (!vtsk) { ret = -ENOMEM; goto free_worker; } - worker->vtsk = vtsk; + WRITE_ONCE(dev->worker.vtsk, vtsk); vhost_task_start(vtsk); return 0; free_worker: - kfree(worker); - dev->worker = NULL; + WRITE_ONCE(dev->worker.vtsk, NULL); return ret; } diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 0308638cdeee..305ec8593d46 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -154,7 +154,7 @@ struct vhost_dev { struct vhost_virtqueue **vqs; int nvqs; struct eventfd_ctx *log_ctx; - struct vhost_worker *worker; + struct vhost_worker worker; struct vhost_iotlb *umem; struct vhost_iotlb *iotlb; spinlock_t iotlb_lock;