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

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

On Friday 02 January 2009 08:10:37 Marcelo Tosatti wrote:
> On Wed, Dec 31, 2008 at 01:43:54PM +0800, Sheng Yang wrote:
> > On Wednesday 31 December 2008 00:45:51 Marcelo Tosatti wrote:
> > > On Tue, Dec 30, 2008 at 10:14:09AM +0800, Sheng Yang wrote:
> > > > > There is one remaining issue:
> > > > > kvm_assigned_dev_interrupt_work_handler can re-enable the interrupt
> > > > > for KVM_ASSIGNED_DEV_GUEST_MSI case. Perhaps you need a new flag to
> > > > > indicate shutdown (so the host IRQ won't be reenabled).
> > > >
> > > > Is it already covered by disable_irq_no_sync() before
> > > > cancel_work_sync()? I've noted this in my comment: the irq may be
> > > > disabled nested(once for MSI and twice for INTx), but I think it's
> > > > fine for we're going to free it.
> > >
> > > The problem is that the irq can be re-enabled in
> > > kvm_assigned_dev_interrupt_work_handler:
> > >
> > >
> > > context 1               |    context 2
> > >
> > > disable_irq_nosync
> > >                             kvm_assigned_dev_interrupt_work_handler
> > >                             enable_irq
> > > cancel_work_sync
> > >
> > > free_irq
> >
> > Um... My understanding is a little different...
> >
> > Before context1 execute disable_irq_nosync(), in irq handler,
> > disable_irq_nosync() would also been executed. So only one enable_irq()
> > is not really enable irq here, which can cover the window at all. That's
> > what I means nested disable irq...
>
> You're right. There have been two disable_irq calls.
>
> OK, looks good to me.

So Avi, can you check in the patchset?

Thanks.

-- 
regards
Yang, Sheng