lkml.org 
[lkml]   [2013]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH] kvm: fix a race when closing irq eventfd
On 2013/2/18 12:02, Alex Williamson wrote:
> On Mon, 2013-02-18 at 11:13 +0800, Li Zefan wrote:
>> While trying to fix a race when closing cgroup eventfd, I took a look
>> at how kvm deals with this problem, and I found it doesn't.
>>
>> I may be wrong, as I don't know kvm code, so correct me if I'm.
>>
>> /*
>> * Race-free decouple logic (ordering is critical)
>> */
>> static void
>> irqfd_shutdown(struct work_struct *work)
>>
>> I don't think it's race-free!
>>
>> static int
>> irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
>> {
>> ...
>> * We cannot race against the irqfd going away since the
>> * other side is required to acquire wqh->lock, which we hold
>> */
>> if (irqfd_is_active(irqfd))
>> irqfd_deactivate(irqfd);
>> }
>>
>> In kvm_irqfd_deassign() and kvm_irqfd_release() where irqfds are freed,
>> wqh->lock is not acquired!
>>
>> So here is the race:
>>
>> CPU0 CPU1
>> ----------------------------------- ---------------------------------
>> kvm_irqfd_release()
>> spin_lock(kvm->irqfds.lock);
>> ...
>> irqfd_deactivate(irqfd);
>> list_del_init(&irqfd->list);
>> spin_unlock(kvm->irqfd.lock);
>> ...
>> close(eventfd)
>> irqfd_wakeup();
>
> irqfd_wakeup is assumed to be called with wqh->lock held
>

I'm aware of this.

As I said, kvm_irqfd_deassign() and kvm_irqfd_release() are not acquiring
wqh->lock.

>> irqfd_shutdown();
>
> eventfd_ctx_remove_wait_queue has to acquire wqh->lock to complete or
> else irqfd_shutdown never makes it to the kfree. So in your scenario
> this cpu0 spins here until cpu1 completes.
>
>> remove_waitqueue(irqfd->wait);
>> kfree(irqfd);
>> spin_lock(kvm->irqfd.lock);
>> if (!list_empty(&irqfd->list))
>
> We don't take this branch because we already did list_del_init above,
> which makes irqfd->list empty.
>

It doesn't matter if the list is empty or not.

The point is, irqfd has been kfreed, so the if statement is simply not safe!

>> irqfd_deactivate(irqfd);
>> list_del_init(&irqfd->list);
>> spin_unlock(kvm->irqfd.lock);
>>
>> Look, we're accessing irqfd though it has already been freed!
>
> Unless the irqfd_wakeup path isn't acquiring wqh->lock, it looks
> race-free to me. Thanks,
>
> Alex
>
> .
>



\
 
 \ /
  Last update: 2013-02-18 05:21    [W:0.045 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site