kvm: fix a race when closing irq eventfd

RFC PATCH In review Li Zefan 2 participants 5 messages Feb 18, 2013

Subsystems (3)

VFS KVM cgroup

lore

Share & tools

lore

Li Zefan

author

2013-02-18 03:13 UTC #0 lore raw

RFC PATCH 1/1 kvm: fix a race when closing irq eventfd

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_shutdown();
      remove_waitqueue(irqfd->wait);
      kfree(irqfd);
					    spin_lock(kvm->irqfd.lock);
					      if (!list_empty(&irqfd->list))
						irqfd_deactivate(irqfd);
						  list_del_init(&irqfd->list);
					    spin_unlock(kvm->irqfd.lock);

Look, we're accessing irqfd though it has already been freed!

Here's a fix I have. Please enlighten me with a better fix.

Signed-off-by Li Zefan <[email protected]>

---
 fs/eventfd.c            | 34 ++++++++++++++--------------------
 include/linux/eventfd.h | 17 +++++++++++++++++
 virt/kvm/eventfd.c      | 45 +++++++++++++++++++++++++++++++++++----------
 3 files changed, 66 insertions(+), 30 deletions(-)

fs/eventfd.c +14 ~~-20~~

index 35470d9..cda8a4c 100644
@@ -22,21 +22,6 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 
-struct eventfd_ctx {
-	struct kref kref;
-	wait_queue_head_t wqh;
-	/*
-	 * Every time that a write(2) is performed on an eventfd, the
-	 * value of the __u64 being written is added to "count" and a
-	 * wakeup is performed on "wqh". A read(2) will return the "count"
-	 * value to userspace, and will reset "count" to zero. The kernel
-	 * side eventfd_signal() also, adds to the "count" counter and
-	 * issue a wakeup.
-	 */
-	__u64 count;
-	unsigned int flags;
-};
-
 /**
  * eventfd_signal - Adds @n to the eventfd counter.
  * @ctx: [in] Pointer to the eventfd context.

@@ -153,20 +138,29 @@ static void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt)
  * This is used to atomically remove a wait queue entry from the eventfd wait
  * queue head, and read/reset the counter value.
  */
-int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, wait_queue_t *wait,
+int eventfd_ctx_remove_wait_queue_locked(struct eventfd_ctx *ctx, wait_queue_t *wait,
 				  __u64 *cnt)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&ctx->wqh.lock, flags);
 	eventfd_ctx_do_read(ctx, cnt);
 	__remove_wait_queue(&ctx->wqh, wait);
 	if (*cnt != 0 && waitqueue_active(&ctx->wqh))
 		wake_up_locked_poll(&ctx->wqh, POLLOUT);
-	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
 
 	return *cnt != 0 ? 0 : -EAGAIN;
 }
+EXPORT_SYMBOL_GPL(eventfd_ctx_remove_wait_queue_locked);
+
+int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, wait_queue_t *wait,
+				  __u64 *cnt)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&ctx->wqh.lock, flags);
+	ret = eventfd_ctx_remove_wait_queue_locked(ctx, wait, cnt);
+	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+	return ret;
+}
 EXPORT_SYMBOL_GPL(eventfd_ctx_remove_wait_queue);
 
 /**

include/linux/eventfd.h +17 -0

index 3c3ef19..61085ac 100644
@@ -26,6 +26,21 @@
 #define EFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
 #define EFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS | EFD_SEMAPHORE)
 
+struct eventfd_ctx {
+	struct kref kref;
+	wait_queue_head_t wqh;
+	/*
+	 * Every time that a write(2) is performed on an eventfd, the
+	 * value of the __u64 being written is added to "count" and a
+	 * wakeup is performed on "wqh". A read(2) will return the "count"
+	 * value to userspace, and will reset "count" to zero. The kernel
+	 * side eventfd_signal() also, adds to the "count" counter and
+	 * issue a wakeup.
+	 */
+	__u64 count;
+	unsigned int flags;
+};
+
 #ifdef CONFIG_EVENTFD
 
 struct file *eventfd_file_create(unsigned int count, int flags);

@@ -38,6 +53,8 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n);
 ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt);
 int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, wait_queue_t *wait,
 				  __u64 *cnt);
+int eventfd_ctx_remove_wait_queue_locked(struct eventfd_ctx *ctx, wait_queue_t *wait,
+				  __u64 *cnt);
 
 #else /* CONFIG_EVENTFD */

virt/kvm/eventfd.c +35 ~~-10~~

101 lines not on this page · raw message

Signature · 1 line

1.8.0.2

Alex Williamson

2013-02-18 04:02 UTC #1 lore raw

Quoted 34 lines · Li Zefan

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);
...

Quoted 6 lines

  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

Quoted 1 line

    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.

Quoted 4 lines

      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.

Quoted 5 lines

						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

Li Zefan

author

2013-02-18 04:09 UTC #2 lore raw

Quoted 42 lines · Alex Williamson

Quoted 34 lines · Li Zefan
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);
...
Quoted 6 lines
  irqfd_deactivate(irqfd);
    list_del_init(&irqfd->list);
  spin_unlock(kvm->irqfd.lock);
  ...
					close(eventfd)
					  irqfd_wakeup();

Quoted 2 lines

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.

Quoted 6 lines

Quoted 1 line
    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.

Quoted 8 lines

Quoted 4 lines
      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!

Quoted 13 lines

Quoted 5 lines
						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
.

Alex Williamson

2013-02-18 04:25 UTC #3 lore raw

Quoted 43 lines · Li Zefan

Quoted 42 lines · Alex Williamson
Quoted 40 lines · Li Zefan
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();

Quoted 7 lines

Quoted 2 lines
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.

They do when they call eventfd_ctx_remove_wait_queue. The irqfd is enabled until that point and the list_del_init prevents multiple paths from calling irqfd_deactivate.

Quoted 11 lines

Quoted 11 lines
Quoted 1 line
    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.
Quoted 4 lines
      remove_waitqueue(irqfd->wait);
      kfree(irqfd);
					    spin_lock(kvm->irqfd.lock);
					      if (!list_empty(&irqfd->list))

Quoted 5 lines

Quoted 3 lines
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.

Note that this is not kvm->irqfds.items, we're testing whether the individual irqfd is detached from the list.

Quoted 1 line

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

It cannot be kfreed. As noted above the cpu0 path stops trying to acquire wqh->lock which already owned by cpu1. The call to eventfd_ctx_remove_wait_queue atomically removes the wait queue once the wqh->lock is acquired, so after that point we're ok to kfree it. Thanks,

Alex

Quoted 18 lines

Quoted 4 lines
Quoted 4 lines
						irqfd_deactivate(irqfd);
						  list_del_init(&irqfd->list);
					    spin_unlock(kvm->irqfd.lock);
Quoted 9 lines
Quoted 1 line
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
.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Li Zefan

author

2013-02-18 04:31 UTC #4 lore raw

In reply to Li Zefan

Quoted 43 lines · Li Zefan

Quoted 42 lines · Alex Williamson
Quoted 40 lines · Li Zefan
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();

Quoted 14 lines

Quoted 2 lines
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.
Quoted 6 lines
Quoted 1 line
    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.

Oh you're right, this is not obvious. Thanks for the explanation.

Now I'll go to see how to fix cgroup.

Quoted 27 lines

Quoted 8 lines
Quoted 4 lines
      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!
Quoted 13 lines
Quoted 5 lines
						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
.