Re: [PATCH 15/15] KVM: Fix racy in kvm_free_assigned_irq

[Sheng Yang <sheng@linux.intel.com>]

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 <sheng@linux.intel.com>
> ---

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 <sheng@linux.intel.com>
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 <sheng@linux.intel.com>
---
 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