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

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

On Monday 29 December 2008 23:20:57 Marcelo Tosatti wrote:
> On Mon, Dec 29, 2008 at 08:23:28PM +0800, Sheng Yang wrote:
> > On Monday 29 December 2008 13:42:22 Amit Shah wrote:
> > > On Sun, Dec 28, 2008 at 07:24:02PM +0800, Sheng Yang wrote:
> > > > On Sat, Dec 27, 2008 at 06:06:26PM -0200, Marcelo Tosatti wrote:
> > > > > On Fri, Dec 26, 2008 at 10:30:07AM +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.
> > > > >
> > > > > Why not simply drop the reference inc / dec from irq handler/work
> > > > > function?
> > > >
> > > > Sorry, I don't know the answer. After checking the code, I also think
> > > > it's a little strange to increase refernce count here, and I think we
> > > > won't suppose work_handler can release the kvm struct.
> > >
> > > At the time of developing that code, this was my observation:
> > >
> > > I see from the call chain kvm_put_kvm->...->kvm_arch_destroy_vm, no
> > > locks are taken to actually destroy the vm. We can't be in ioctls,
> > > sure. But shouldn't the mutex be taken to ensure there's nothing else
> > > going on while destroying?
> > >
> > > At least with the workqueue model, we could be called asynchronously in
> > > kernel context and I would have held the mutex and about to inject
> > > interrupts while everything is being wiped off underneath. However, the
> > > workqueue model tries its best to schedule the work on the same CPU,
> > > though we can't use that guarantee to ensure things will be fine.
> > >
> > > ---
> > > So I had to get a ref to the current vm till we had any pending work
> > > scheduled. I think I put in comments in the code, but sadly most of my
> > > comments we stripped out before the merge.
> >
> > Not quite understand...
> >
> > The free assigned device in the destroy path of VM, so as free irq. And
> > we got cancel_work_sync() in free irq which can sync with the execution
> > of scheduled work. And now before cancel_work_sync(), we disable the
> > interrupt so that no more schedule work happen again. So after
> > cancel_work_sync(), everything(I think it's irq handler and schedule work
> > here) asynchronously should quiet down.
> >
> > Or I miss something?
>
> Thats right. As long as you disable the irq and cancel pending work
> before freeing the data structures those paths use.
>
> 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.

-- 
regards
Yang, Sheng