On Thu, Dec 25, 2008 at 05:09:39PM +0800, Sheng Yang wrote: > Thanks to Marcelo's observation, The following code have potential issue: > > if (cancel_work_sync(&assigned_dev->interrupt_work)) > kvm_put_kvm(kvm); > > In fact, cancel_work_sync() would return true either work struct is only > scheduled or the callback of work struct is executed. This code only > consider the former situation. > > Also, we have a window between cancel_work_sync() and free_irq. This patch > fixs > them two. > > Signed-off-by: Sheng Yang <[email protected]> > ---
Still got trouble here. Something can happened after kfifo_len check. > + if (kfifo_len(assigned_dev->irq_fifo) == 0 && > + (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)) > + disable_irq_nosync(assigned_dev->host_irq); > + > + if (cancel_work_sync(&assigned_dev->interrupt_work) && > + kfifo_len(assigned_dev->irq_fifo) != 0) > kvm_put_kvm(kvm); I think just disable_irq for all is OK, though it would result in nested disable sometime. How about this: -- From: Sheng Yang <[email protected]> Date: Thu, 25 Dec 2008 19:45:03 +0800 Subject: [PATCH 15/15] KVM: Fix racy in kvm_free_assigned_irq Thanks to Marcelo's observation, The following code have potential issue: if (cancel_work_sync(&assigned_dev->interrupt_work)) kvm_put_kvm(kvm); In fact, cancel_work_sync() would return true either work struct is only scheduled or the callback of work struct is executed. This code only consider the former situation. Also, we have a window between cancel_work_sync() and free_irq. This patch fixs them two. Signed-off-by: Sheng Yang <[email protected]> --- virt/kvm/kvm_main.c | 25 +++++++++++++++++++++---- 1 files changed, 21 insertions(+), 4 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 1863942..f4859b8 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -186,10 +186,27 @@ static void kvm_free_assigned_irq(struct kvm *kvm, if (!assigned_dev->irq_requested_type) return; - if (cancel_work_sync(&assigned_dev->interrupt_work)) - /* We had pending work. That means we will have to take - * care of kvm_put_kvm. - */ + /* + * We need to ensure: kvm_put_kvm() paired with kvm_get_kvm() in + * kvm_assigned_dev_intr, and no more interrupt after we cancelled + * current one. + * + * Here we have two possiblities for cancel_work_sync() return true: + * 1. The work is scheduled, but callback haven't been called. We need + * to call kvm_put_kvm() here. And IRQ is already disabled without + * doubt. + * + * 2. The callback have executed, here we don't need to call + * kvm_put_kvm(), but we may need to disable irq(e.g. for MSI). + * + * We judge the two condition according to if we have pending IRQs in + * irq_fifo. And we disable irq here anyway, and it may resulted in + * nested disable, but it's fine, for we are going to free it. + */ + disable_irq_nosync(assigned_dev->host_irq); + + if (cancel_work_sync(&assigned_dev->interrupt_work) && + kfifo_len(assigned_dev->irq_fifo) != 0) kvm_put_kvm(kvm); free_irq(assigned_dev->host_irq, (void *)assigned_dev); -- 1.5.4.5 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
