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

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

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?

-- 
regards
Yang, Sheng