5.13-rt1 + KVM = WARNING: at fs/eventfd.c:74 eventfd_signal()

Hey

I use kvm-vm for regular development, and while using the kernel-rt v5.13-rt1 (the latest) on the host, and a regular kernel on the guest, after a while, this happens:

[ 1723.404979] ------------[ cut here ]------------
[ 1723.404981] WARNING: CPU: 12 PID: 2554 at fs/eventfd.c:74 eventfd_signal+0x7e/0x90

[ 1723.405055] RIP: 0010:eventfd_signal+0x7e/0x90
[ 1723.405059] Code: 01 00 00 00 be 03 00 00 00 4c 89 ef e8 5b ec d9 ff 65 ff 0d e4 34 c9 5a 4c 89 ef e8 ec a8 86 00 4c 89 e0 5b 5d 41 5c 41 5d c3 <0f> 0b 45 31 e4 5b 5d 4c 89 e0 41 5c 41 5d c3 0f 1f 00 0f 1f 44 00
[ 1723.405078]  vhost_tx_batch.constprop.0+0x7d/0xc0 [vhost_net]
[ 1723.405083]  handle_tx_copy+0x15b/0x5c0 [vhost_net]
[ 1723.405088]  ? __vhost_add_used_n+0x200/0x200 [vhost]
[ 1723.405092]  handle_tx+0xa5/0xe0 [vhost_net]
[ 1723.405095]  vhost_worker+0x93/0xd0 [vhost]
[ 1723.405099]  kthread+0x186/0x1a0
[ 1723.405103]  ? __kthread_parkme+0xa0/0xa0
[ 1723.405105]  ret_from_fork+0x22/0x30
[ 1723.405110] ---[ end trace 0000000000000002 ]---

Quoted 9 lines · Daniel Bristot de Oliveira

Hey

I use kvm-vm for regular development, and while using the kernel-rt v5.13-rt1 (the latest) on the host, and a regular kernel on the guest, after a while, this happens:

[ 1723.404979] ------------[ cut here ]------------
[ 1723.404981] WARNING: CPU: 12 PID: 2554 at fs/eventfd.c:74 eventfd_signal+0x7e/0x90

Quoted 11 lines

[ 1723.405055] RIP: 0010:eventfd_signal+0x7e/0x90
[ 1723.405059] Code: 01 00 00 00 be 03 00 00 00 4c 89 ef e8 5b ec d9 ff 65 ff 0d e4 34 c9 5a 4c 89 ef e8 ec a8 86 00 4c 89 e0 5b 5d 41 5c 41 5d c3 <0f> 0b 45 31 e4 5b 5d 4c 89 e0 41 5c 41 5d c3 0f 1f 00 0f 1f 44 00
[ 1723.405078]  vhost_tx_batch.constprop.0+0x7d/0xc0 [vhost_net]
[ 1723.405083]  handle_tx_copy+0x15b/0x5c0 [vhost_net]
[ 1723.405088]  ? __vhost_add_used_n+0x200/0x200 [vhost]
[ 1723.405092]  handle_tx+0xa5/0xe0 [vhost_net]
[ 1723.405095]  vhost_worker+0x93/0xd0 [vhost]
[ 1723.405099]  kthread+0x186/0x1a0
[ 1723.405103]  ? __kthread_parkme+0xa0/0xa0
[ 1723.405105]  ret_from_fork+0x22/0x30
[ 1723.405110] ---[ end trace 0000000000000002 ]---

The WARN has this comment above:

        /*
         * Deadlock or stack overflow issues can happen if we recurse here
         * through waitqueue wakeup handlers. If the caller users potentially
         * nested waitqueues with custom wakeup handlers, then it should
         * check eventfd_signal_count() before calling this function. If
         * it returns true, the eventfd_signal() call should be deferred to a
         * safe context.
         */

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

Paolo

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines · Jason Wang

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock. Something like this (untested!!):

--------------- 8< ---------------
From: Paolo Bonzini <[email protected]>
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT. If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN. To avoid this, protect the percpu variable with a local_lock.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

fs/eventfd.c +7 -1

index e265b6dd4f34..7d27b6e080ea 100644
@@ -12,6 +12,7 @@
 #include <linux/fs.h>
 #include <linux/sched/signal.h>
 #include <linux/kernel.h>
+#include <linux/local_lock.h>
 #include <linux/slab.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>

@@ -25,6 +26,7 @@
 #include <linux/idr.h>
 #include <linux/uio.h>
+static local_lock_t eventfd_wake_lock = INIT_LOCAL_LOCK(eventfd_wake_lock);
 DEFINE_PER_CPU(int, eventfd_wake_count);
 static DEFINE_IDA(eventfd_ida);

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
 	 * it returns true, the eventfd_signal() call should be deferred to a
 	 * safe context.
 	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	local_lock(&eventfd_wake_lock);
+	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+		local_unlock(&eventfd_wake_lock);
 		return 0;
+	}
 	spin_lock_irqsave(&ctx->wqh.lock, flags);
 	this_cpu_inc(eventfd_wake_count);

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
 		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
 	this_cpu_dec(eventfd_wake_count);
 	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+	local_unlock(&eventfd_wake_lock);
 	return n;
 }

Quoted 13 lines

--------------- 8< ---------------
From: Paolo Bonzini <[email protected]>
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT. If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN. To avoid this, protect the percpu variable with a local_lock.

Quoted 6 lines

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Makes sense ...

Acked-by Michael S. Tsirkin <[email protected]>

want to send this to the windriver guys so they can test? Here's the list from that thread:

Quoted 8 lines · Jason Wang

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

--------------- 8< ---------------
From: Paolo Bonzini <[email protected]>
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT.  If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN.  To avoid this, protect the percpu variable with a local_lock.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

fs/eventfd.c +1 -0

index e265b6dd4f34..7d27b6e080ea 100644
@@ -12,6 +12,7 @@
  #include <linux/fs.h>
  #include <linux/sched/signal.h>
  #include <linux/kernel.h>
+#include <linux/local_lock.h>
  #include <linux/slab.h>
  #include <linux/list.h>
  #include <linux/spinlock.h>

@@ -25,6 +26,7 @@
  #include <linux/idr.h>
  #include <linux/uio.h>

+static local_lock_t eventfd_wake_lock = INIT_LOCAL_LOCK(eventfd_wake_lock); DEFINE_PER_CPU(int, eventfd_wake_count);

 static DEFINE_IDA(eventfd_ida);

fs/eventfd.c +4 -1

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
       * it returns true, the eventfd_signal() call should be deferred to a
       * safe context.
       */
-    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+    local_lock(&eventfd_wake_lock);
+    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+        local_unlock(&eventfd_wake_lock);
          return 0;
+    }

     spin_lock_irqsave(&ctx->wqh.lock, flags);
     this_cpu_inc(eventfd_wake_count);

fs/eventfd.c +1 -0

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
          wake_up_locked_poll(&ctx->wqh, EPOLLIN);
      this_cpu_dec(eventfd_wake_count);
      spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+    local_unlock(&eventfd_wake_lock);

     return n;
 }

Quoted 9 lines · Jason Wang

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

--------------- 8< ---------------
From: Paolo Bonzini <[email protected]>
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT.  If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN.  To avoid this, protect the percpu variable with a local_lock.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

fs/eventfd.c +2 -0

index e265b6dd4f34..7d27b6e080ea 100644
@@ -12,6 +12,7 @@
  #include <linux/fs.h>
  #include <linux/sched/signal.h>
  #include <linux/kernel.h>
+#include <linux/local_lock.h>
  #include <linux/slab.h>
  #include <linux/list.h>
  #include <linux/spinlock.h>

@@ -25,6 +26,7 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
+static local_lock_t eventfd_wake_lock = 
 INIT_LOCAL_LOCK(eventfd_wake_lock);
  DEFINE_PER_CPU(int, eventfd_wake_count);
 

 static DEFINE_IDA(eventfd_ida);

fs/eventfd.c +5 -1

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
       * it returns true, the eventfd_signal() call should be deferred to a
       * safe context.
       */
-    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+    local_lock(&eventfd_wake_lock);
+    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+        local_unlock(&eventfd_wake_lock);
          return 0;
+    }
 
      spin_lock_irqsave(&ctx->wqh.lock, flags);
      this_cpu_inc(eventfd_wake_count);

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
          wake_up_locked_poll(&ctx->wqh, EPOLLIN);
      this_cpu_dec(eventfd_wake_count);
      spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+    local_unlock(&eventfd_wake_lock);
 
      return n;
  }

Quoted 4 lines

This obviously does not fly with PREEMPT_RT.  If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN.  To avoid this, protect the percpu variable with a local_lock.

But local_lock only disable migration not preemption.

Or anything I missed here?

Thanks

Quoted 51 lines

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

fs/eventfd.c +2 -0

index e265b6dd4f34..7d27b6e080ea 100644
@@ -12,6 +12,7 @@
  #include <linux/fs.h>
  #include <linux/sched/signal.h>
  #include <linux/kernel.h>
+#include <linux/local_lock.h>
  #include <linux/slab.h>
  #include <linux/list.h>
  #include <linux/spinlock.h>

@@ -25,6 +26,7 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
+static local_lock_t eventfd_wake_lock = 
 INIT_LOCAL_LOCK(eventfd_wake_lock);
  DEFINE_PER_CPU(int, eventfd_wake_count);
 

 static DEFINE_IDA(eventfd_ida);

fs/eventfd.c +5 -1

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
       * it returns true, the eventfd_signal() call should be deferred to a
       * safe context.
       */
-    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+    local_lock(&eventfd_wake_lock);
+    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+        local_unlock(&eventfd_wake_lock);
          return 0;
+    }
 
      spin_lock_irqsave(&ctx->wqh.lock, flags);
      this_cpu_inc(eventfd_wake_count);

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
          wake_up_locked_poll(&ctx->wqh, EPOLLIN);
      this_cpu_dec(eventfd_wake_count);
      spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+    local_unlock(&eventfd_wake_lock);
 
      return n;
  }

Quoted 5 lines · Jason Wang

Quoted 4 lines

This obviously does not fly with PREEMPT_RT.  If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN.  To avoid this, protect the percpu variable with a local_lock.

Quoted 1 line

But local_lock only disable migration not preemption.

On mainline PREEMPT_RT, local_lock is an array of per-CPU spinlocks. When two eventfd_signals run on the same CPU and one is preempted, the spinlocks avoid that the second sees eventfd_wake_count > 0.

Thanks,

Paolo

Quoted 60 lines

Or anything I missed here?

Thanks

Quoted 53 lines

Reported-by Daniel Bristot de Oliveira <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

fs/eventfd.c +2 -0

index e265b6dd4f34..7d27b6e080ea 100644
@@ -12,6 +12,7 @@
  #include <linux/fs.h>
  #include <linux/sched/signal.h>
  #include <linux/kernel.h>
+#include <linux/local_lock.h>
  #include <linux/slab.h>
  #include <linux/list.h>
  #include <linux/spinlock.h>

@@ -25,6 +26,7 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
+static local_lock_t eventfd_wake_lock = 
 INIT_LOCAL_LOCK(eventfd_wake_lock);
  DEFINE_PER_CPU(int, eventfd_wake_count);
 

 static DEFINE_IDA(eventfd_ida);

fs/eventfd.c +4 -1

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, 
 __u64 n)
       * it returns true, the eventfd_signal() call should be deferred to a
       * safe context.
       */
-    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+    local_lock(&eventfd_wake_lock);
+    if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+        local_unlock(&eventfd_wake_lock);
          return 0;
+    }
 
      spin_lock_irqsave(&ctx->wqh.lock, flags);

     this_cpu_inc(eventfd_wake_count);

fs/eventfd.c +1 -0

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, 
 __u64 n)
          wake_up_locked_poll(&ctx->wqh, EPOLLIN);
      this_cpu_dec(eventfd_wake_count);
      spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+    local_unlock(&eventfd_wake_lock);
 
      return n;

 }

Quoted 8 lines · Jason Wang

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

Quoted 13 lines · Paolo Bonzini

Quoted 7 lines · Jason Wang

Quoted 4 lines

This was added in 2020, so it's unlikely to be the direct cause of the change.  What is a known-good version for the host?

Since it is not KVM stuff, I'm CCing Michael and Jason.

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

Quoted 2 lines

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

the lock needs to be per-pcu... but so far, so good. I will continue using the system in the next days to see if it blows on another way.

The patch looks like this now:

------------------------- 8< ---------------------
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT. If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN. To avoid this, protect the percpu variable with a local_lock.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Suggested-by Paolo Bonzini <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Co-developed-by Paolo Bonzini <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Co-developed-by Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

---
 fs/eventfd.c            | 27 ++++++++++++++++++++++-----
 include/linux/eventfd.h |  7 +------
 2 files changed, 23 insertions(+), 11 deletions(-)

fs/eventfd.c +22 -5

index e265b6dd4f34..9754fcd38690 100644
@@ -12,6 +12,7 @@
 #include <linux/fs.h>
 #include <linux/sched/signal.h>
 #include <linux/kernel.h>
+#include <linux/local_lock.h>
 #include <linux/slab.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>

@@ -25,8 +26,6 @@
 #include <linux/idr.h>
 #include <linux/uio.h>
 
-DEFINE_PER_CPU(int, eventfd_wake_count);
-
 static DEFINE_IDA(eventfd_ida);
 
 struct eventfd_ctx {

@@ -45,6 +44,20 @@ struct eventfd_ctx {
 	int id;
 };
 
+struct event_fd_recursion {
+	local_lock_t lock;
+	int count;
+};
+
+static DEFINE_PER_CPU(struct event_fd_recursion, event_fd_recursion) = {
+	.lock = INIT_LOCAL_LOCK(lock),
+};
+
+bool eventfd_signal_count(void)
+{
+	return this_cpu_read(event_fd_recursion.count);
+}
+
 /**
  * eventfd_signal - Adds @n to the eventfd counter.
  * @ctx: [in] Pointer to the eventfd context.

@@ -71,18 +84,22 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
 	 * it returns true, the eventfd_signal() call should be deferred to a
 	 * safe context.
 	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	local_lock(&event_fd_recursion.lock);
+	if (WARN_ON_ONCE(this_cpu_read(event_fd_recursion.count))) {
+		local_unlock(&event_fd_recursion.lock);
 		return 0;
+	}
 
 	spin_lock_irqsave(&ctx->wqh.lock, flags);
-	this_cpu_inc(eventfd_wake_count);
+	this_cpu_inc(event_fd_recursion.count);
 	if (ULLONG_MAX - ctx->count < n)
 		n = ULLONG_MAX - ctx->count;
 	ctx->count += n;
 	if (waitqueue_active(&ctx->wqh))
 		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
-	this_cpu_dec(eventfd_wake_count);
+	this_cpu_dec(event_fd_recursion.count);
 	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+	local_unlock(&event_fd_recursion.lock);
 
 	return n;
 }

include/linux/eventfd.h +0 -0

index fa0a524baed0..ca89d6c409c1 100644
@@ -43,12 +43,7 @@ int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx,

wait_queue_entry_t *w

include/linux/eventfd.h +1 -6

 				  __u64 *cnt);
 void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt);
 
-DECLARE_PER_CPU(int, eventfd_wake_count);
-
-static inline bool eventfd_signal_count(void)
-{
-	return this_cpu_read(eventfd_wake_count);
-}
+bool eventfd_signal_count(void);
 
 #else /* CONFIG_EVENTFD */

It was to increase the recursion depth to 2 so that vhost_worker and kvm_vcpu_ioctl syscall could work in parallel

Quoted 2 lines · He Zhe

It was to increase the recursion depth to 2 so that vhost_worker and kvm_vcpu_ioctl syscall could work in parallel

The count is per-CPU, so parallel operations cannot cause it to become 2.  Your patch might fix calls from ioeventfd to vhost_worker to another eventfd, but not *parallel* operation of KVM and vhost (except on PREEMPT_RT).

You should identify the exact callstack that caused the warning for vDUSE, and document that one in the commit message, so that reviewers can understand the issue.

Paolo

The following was provided in this thread. The commit log contains the call traces that I met and fixed back to Apr. 2020.

https://lore.kernel.org/lkml/[email protected]/

001: WARNING: CPU: 1 PID: 1503 at fs/eventfd.c:73 eventfd_signal+0x85/0xa0 ---- snip ----
001: Call Trace:

001:  vhost_signal+0x15e/0x1b0 [vhost]
001:  vhost_add_used_and_signal_n+0x2b/0x40 [vhost]
001:  handle_rx+0xb9/0x900 [vhost_net]
001:  handle_rx_net+0x15/0x20 [vhost_net]
001:  vhost_worker+0xbe/0x120 [vhost]
001:  kthread+0x106/0x140
001:  ? log_used.part.0+0x20/0x20 [vhost]
001:  ? kthread_park+0x90/0x90
001:  ret_from_fork+0x35/0x40

https://lore.kernel.org/lkml/[email protected]/

Quoted 5 lines · He Zhe

The following was provided in this thread. The commit log contains the call traces that I met and fixed back to Apr. 2020.

https://lore.kernel.org/lkml/[email protected]/

Quoted 12 lines

001: WARNING: CPU: 1 PID: 1503 at fs/eventfd.c:73 eventfd_signal+0x85/0xa0 ---- snip ----
001: Call Trace:
001:  vhost_signal+0x15e/0x1b0 [vhost]
001:  vhost_add_used_and_signal_n+0x2b/0x40 [vhost]
001:  handle_rx+0xb9/0x900 [vhost_net]
001:  handle_rx_net+0x15/0x20 [vhost_net]
001:  vhost_worker+0xbe/0x120 [vhost]
001:  kthread+0x106/0x140
001:  ? log_used.part.0+0x20/0x20 [vhost]
001:  ? kthread_park+0x90/0x90
001:  ret_from_fork+0x35/0x40

This call trace is not of a reentrant call; there is only one call to eventfd_signal.  It does fit the symptoms that Daniel reported for PREEMPT_RT though.

Quoted 2 lines

https://lore.kernel.org/lkml/[email protected]/

This one is about PREEMPT_RT, so it would be fixed by local_lock.

There _may_ be two bugs, so let's start by fixing this one.  Once this one is fixed, we will examine the call stacks of any further reports, and diagnose whether the second bug (if it exists) is related to vDUSE, PREEMPT_RT or neeither.

Paolo

Quoted 22 lines · Paolo Bonzini

Quoted 4 lines · He Zhe

The following was provided in this thread. The commit log contains the call traces that I met and fixed back to Apr. 2020.

https://lore.kernel.org/lkml/[email protected]/

Quoted 12 lines

001: WARNING: CPU: 1 PID: 1503 at fs/eventfd.c:73 eventfd_signal+0x85/0xa0 ---- snip ----
001: Call Trace:
001:  vhost_signal+0x15e/0x1b0 [vhost]
001:  vhost_add_used_and_signal_n+0x2b/0x40 [vhost]
001:  handle_rx+0xb9/0x900 [vhost_net]
001:  handle_rx_net+0x15/0x20 [vhost_net]
001:  vhost_worker+0xbe/0x120 [vhost]
001:  kthread+0x106/0x140
001:  ? log_used.part.0+0x20/0x20 [vhost]
001:  ? kthread_park+0x90/0x90
001:  ret_from_fork+0x35/0x40

This call trace is not of a reentrant call; there is only one call to eventfd_signal.  It does fit the symptoms that Daniel reported for PREEMPT_RT though.

Quoted 1 line

https://lore.kernel.org/lkml/[email protected]/

Quoted 3 lines

This one is about PREEMPT_RT, so it would be fixed by local_lock.

There _may_ be two bugs, so let's start by fixing this one.  Once this one is fixed, we will examine the call stacks of any further reports, and diagnose whether the second bug (if it exists) is related to vDUSE, PREEMPT_RT or neeither.

For VDUSE we may still need the patch since it tries to relay notifications (eventfds) which means the recursion of the eventfd signal.

But looking at the comment in the eventfd_signal() which say we should check with eventfd_signal_count() and delay the signal into a safe context (e.g workqueue etc).

Thanks

Quoted 3 lines

Paolo

Quoted 20 lines · Jason Wang

Quoted 19 lines · Paolo Bonzini

Quoted 15 lines · He Zhe

The following was provided in this thread. The commit log contains the call traces that I met and fixed back to Apr. 2020.

https://lore.kernel.org/lkml/[email protected]/
001: WARNING: CPU: 1 PID: 1503 at fs/eventfd.c:73 eventfd_signal+0x85/0xa0 ---- snip ----
001: Call Trace:
001:  vhost_signal+0x15e/0x1b0 [vhost]
001:  vhost_add_used_and_signal_n+0x2b/0x40 [vhost]
001:  handle_rx+0xb9/0x900 [vhost_net]
001:  handle_rx_net+0x15/0x20 [vhost_net]
001:  vhost_worker+0xbe/0x120 [vhost]
001:  kthread+0x106/0x140
001:  ? log_used.part.0+0x20/0x20 [vhost]
001:  ? kthread_park+0x90/0x90
001:  ret_from_fork+0x35/0x40

This call trace is not of a reentrant call; there is only one call to eventfd_signal.  It does fit the symptoms that Daniel reported for PREEMPT_RT though.

Quoted 1 line

https://lore.kernel.org/lkml/[email protected]/

Quoted 7 lines

Quoted 3 lines

This one is about PREEMPT_RT, so it would be fixed by local_lock.

There _may_ be two bugs, so let's start by fixing this one.  Once this one is fixed, we will examine the call stacks of any further reports, and diagnose whether the second bug (if it exists) is related to vDUSE, PREEMPT_RT or neeither.

For VDUSE we may still need the patch since it tries to relay notifications (eventfds) which means the recursion of the eventfd signal.

But looking at the comment in the eventfd_signal() which say we should check with eventfd_signal_count() and delay the signal into a safe context (e.g workqueue etc).

The main concern when adding eventfd count at the very beginning is "Deadlock or stack overflow" in the inline comment. If we can avoid deadlock and one depth of nest is acceptable, I think it's safe to set the max count to 2.

The author of the eventfd count kind of also agrees with this.
https://lore.kernel.org/lkml/[email protected]/

Thanks,
Zhe

Quoted 5 lines

Thanks

Quoted 2 lines

Paolo

Quoted 10 lines · Paolo Bonzini

Quoted 3 lines · Jason Wang

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

Quoted 2 lines

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

the lock needs to be per-pcu...

The patch looks like this now:

------------------------- 8< ---------------------
Subject: [PATCH] eventfd: protect eventfd_wake_count with a local_lock

eventfd_signal assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

This obviously does not fly with PREEMPT_RT. If eventfd_signal is preempted and an unrelated thread calls eventfd_signal, the result is a spurious WARN. To avoid this, protect the percpu variable with a local_lock.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Suggested-by Paolo Bonzini <[email protected]>

Fixes b5e683d5cab8 ("eventfd: track eventfd_signal() recursion depth")

Cc [email protected]

Cc He Zhe <[email protected]>

Cc Jens Axboe <[email protected]>

Co-developed-by Paolo Bonzini <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Co-developed-by Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

---
  fs/eventfd.c            | 27 ++++++++++++++++++++++-----
  include/linux/eventfd.h |  7 +------
  2 files changed, 23 insertions(+), 11 deletions(-)

fs/eventfd.c +22 -5

index e265b6dd4f34..9754fcd38690 100644
@@ -12,6 +12,7 @@
  #include <linux/fs.h>
  #include <linux/sched/signal.h>
  #include <linux/kernel.h>
+#include <linux/local_lock.h>
  #include <linux/slab.h>
  #include <linux/list.h>
  #include <linux/spinlock.h>

@@ -25,8 +26,6 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
-DEFINE_PER_CPU(int, eventfd_wake_count);
-
  static DEFINE_IDA(eventfd_ida);
 
  struct eventfd_ctx {

@@ -45,6 +44,20 @@ struct eventfd_ctx {
  	int id;
  };
 
+struct event_fd_recursion {
+	local_lock_t lock;
+	int count;
+};
+
+static DEFINE_PER_CPU(struct event_fd_recursion, event_fd_recursion) = {
+	.lock = INIT_LOCAL_LOCK(lock),
+};
+
+bool eventfd_signal_count(void)
+{
+	return this_cpu_read(event_fd_recursion.count);
+}
+
  /**
   * eventfd_signal - Adds @n to the eventfd counter.
   * @ctx: [in] Pointer to the eventfd context.

@@ -71,18 +84,22 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
  	 * it returns true, the eventfd_signal() call should be deferred to a
  	 * safe context.
  	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	local_lock(&event_fd_recursion.lock);
+	if (WARN_ON_ONCE(this_cpu_read(event_fd_recursion.count))) {
+		local_unlock(&event_fd_recursion.lock);
  		return 0;
+	}
 
  	spin_lock_irqsave(&ctx->wqh.lock, flags);
-	this_cpu_inc(eventfd_wake_count);
+	this_cpu_inc(event_fd_recursion.count);
  	if (ULLONG_MAX - ctx->count < n)
  		n = ULLONG_MAX - ctx->count;
  	ctx->count += n;
  	if (waitqueue_active(&ctx->wqh))
  		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
-	this_cpu_dec(eventfd_wake_count);
+	this_cpu_dec(event_fd_recursion.count);
  	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+	local_unlock(&event_fd_recursion.lock);
 
  	return n;
  }

include/linux/eventfd.h +0 -0

index fa0a524baed0..ca89d6c409c1 100644
@@ -43,12 +43,7 @@ int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx,

wait_queue_entry_t *w
__u64 *cnt);
void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt);

include/linux/eventfd.h +1 -6

-DECLARE_PER_CPU(int, eventfd_wake_count);
-
-static inline bool eventfd_signal_count(void)
-{
-	return this_cpu_read(eventfd_wake_count);
-}
+bool eventfd_signal_count(void);
 
  #else /* CONFIG_EVENTFD */

Quoted 11 lines · Daniel Bristot de Oliveira

Quoted 10 lines · Paolo Bonzini

Quoted 3 lines · Jason Wang

I think this can be probably fixed here:

https://lore.kernel.org/lkml/[email protected]/

That seems wrong; in particular it wouldn't protect against AB/BA deadlocks. In fact, the bug is with the locking; the code assumes that spin_lock_irqsave/spin_unlock_irqrestore is non-preemptable and therefore increments and decrements the percpu variable inside the critical section.

Quoted 4 lines

Quoted 2 lines

This obviously does not fly with PREEMPT_RT; the right fix should be using a local_lock.  Something like this (untested!!):

the lock needs to be per-pcu...

Great, thanks for testing and fixing the patch!  Are you going to post it again once you've confirmed it works?

Paolo

Quoted 2 lines

the lock needs to be per-pcu... but so far, so good. I will continue using the system in the next days to see if it blows on another way.

Curiou, is it so good as to see the warning no longer printed? Or your box still works even with the warning in log?

Quoted 3 lines

With the patch:
- no warn
- continue using the VM normally...

Well with the patch applied, the VM works fine without the stuff protected by the spin_lock_irqsave(), then without the patch why simply printing a warning makes the VM dumb, given the warning is there actually also preventing you from touching the lock.

Quoted 9 lines · On Fri, 16 Jul 2021 08:54:58 +0200 Paolo Bonzini wrote:

Quoted 7 lines · Hillf Danton

Quoted 3 lines

With the patch:
- no warn
- continue using the VM normally...

Well with the patch applied, the VM works fine without the stuff protected by the spin_lock_irqsave(), then without the patch why simply printing a warning makes the VM dumb, given the warning is there actually also preventing you from touching the lock.

Quoted 1 line

If the warning is triggered, eventfd_signal will not do the wakeup.

[I am assuming we are not talking about the deadlock in the comment.]

No preemption occured without the warning printed. Why will the wakeup behavior change without peemption?

Quoted 3 lines · On Fri, 16 Jul 2021 09:59:15 +0200 Paolo Bonzini wrote:

* the warning only occurs if preemption occurs during the spin_lock_irqsave critical section (and therefore it can only occur in PREEMPT_RT kernels)

With that lock held, no waitqueue entry can be added on to the WQ - IOW no wakeup will go stray.

Quoted 1 line

* the warning causes an early return 0 that messes up the VM's networking

Is the messup due to the zero or wakeup?

Quoted 8 lines · Hillf Danton

Quoted 3 lines · On Fri, 16 Jul 2021 09:59:15 +0200 Paolo Bonzini wrote:

* the warning only occurs if preemption occurs during the spin_lock_irqsave critical section (and therefore it can only occur in PREEMPT_RT kernels)

With that lock held, no waitqueue entry can be added on to the WQ - IOW no wakeup will go stray.

Quoted 3 lines

Quoted 1 line

* the warning causes an early return 0 that messes up the VM's networking

Is the messup due to the zero or wakeup?

It's caused by the missing wakeup, i.e. eventfd_signal not really signaling anything.

Quoted 2 lines

It's caused by the missing wakeup, i.e. eventfd_signal not really signaling anything.

Can you please point me to the waiters in the mainline?

There are two cases of write_seqcount_begin in x/virt/kvm/eventfd.c, and in kvm_irqfd_deassign() it is surrounded by spin_lock_irq(&kvm->irqfds.lock) that also protects irqfd_update().

What isnt clear is if the risk is zero that either case can be preempted by seqcount reader. That risk may end up with the livelock described in x/Documentation/locking/seqlock.rst.

(quoted diff) +5 -0

+A sequence counter write side critical section must never be preempted
+or interrupted by read side sections. Otherwise the reader will spin for
+the entire scheduler tick due to the odd sequence count value and the
+interrupted writer. If that reader belongs to a real-time scheduling
+class, it can spin forever and the kernel will livelock.

Quoted 4 lines · Hillf Danton

Quoted 2 lines

It's caused by the missing wakeup, i.e. eventfd_signal not really signaling anything.

Can you please point me to the waiters in the mainline?

It's irqfd_wakeup.

But the preempting waker can not make sense without the waiter who is bloody special. Why is it so in the first place? Or it is not at all but the race existing from Monday to Friday.

Quoted 4 lines · Hillf Danton

But the preempting waker can not make sense without the waiter who is bloody special. Why is it so in the first place? Or it is not at all but the race existing from Monday to Friday.

See the large comment in eventfd_poll().

Quoted 7 lines · On Wed, 21 Jul 2021 09:25:32 +0200 Thomas Gleixner wrote:

Quoted 4 lines · Hillf Danton

But the preempting waker can not make sense without the waiter who is bloody special. Why is it so in the first place? Or it is not at all but the race existing from Monday to Friday.

See the large comment in eventfd_poll().

Is it likely for a reader to make eventfd_poll() return 0?

read	 *     poll                               write
----	 *     -----------------                  ------------

* count = ctx->count (INVALID!)
* lock ctx->qwh.lock
* ctx->count += n
* **waitqueue_active is false**
* **no wake_up_locked_poll!**
* unlock ctx->qwh.lock

lock ctx->qwh.lock
*cnt = (ctx->flags & EFD_SEMAPHORE) ? 1 : ctx->count; ctx->count -= *cnt;
**waitqueue_active is false**
unlock ctx->qwh.lock

	 *     lock ctx->wqh.lock (in poll_wait)
	 *     __add_wait_queue
	 *     unlock ctx->wqh.lock
	 *     eventfd_poll returns 0
	 */
	count = READ_ONCE(ctx->count);

Quoted 26 lines · Hillf Danton

Quoted 7 lines · On Wed, 21 Jul 2021 09:25:32 +0200 Thomas Gleixner wrote:

Quoted 4 lines · Hillf Danton

But the preempting waker can not make sense without the waiter who is bloody special. Why is it so in the first place? Or it is not at all but the race existing from Monday to Friday.

See the large comment in eventfd_poll().

Is it likely for a reader to make eventfd_poll() return 0?

read	 *     poll                               write
----	 *     -----------------                  ------------

* count = ctx->count (INVALID!)
* lock ctx->qwh.lock
* ctx->count += n
* **waitqueue_active is false**
* **no wake_up_locked_poll!**
* unlock ctx->qwh.lock

lock ctx->qwh.lock
*cnt = (ctx->flags & EFD_SEMAPHORE) ? 1 : ctx->count; ctx->count -= *cnt;
**waitqueue_active is false**
unlock ctx->qwh.lock

Quoted 7 lines

	 *     lock ctx->wqh.lock (in poll_wait)
	 *     __add_wait_queue
	 *     unlock ctx->wqh.lock
	 *     eventfd_poll returns 0
	 */
	count = READ_ONCE(ctx->count);

No, it's simply impossible. The same comment explains why: "count = ctx->count" cannot move above poll_wait's locking of ctx->wqh.lock.

Quoted 26 lines · Hillf Danton

Quoted 7 lines · On Wed, 21 Jul 2021 09:25:32 +0200 Thomas Gleixner wrote:

Quoted 4 lines · Hillf Danton

But the preempting waker can not make sense without the waiter who is bloody special. Why is it so in the first place? Or it is not at all but the race existing from Monday to Friday.

See the large comment in eventfd_poll().

Is it likely for a reader to make eventfd_poll() return 0?

read	 *     poll                               write
----	 *     -----------------                  ------------

* count = ctx->count (INVALID!)
* lock ctx->qwh.lock
* ctx->count += n
* **waitqueue_active is false**
* **no wake_up_locked_poll!**
* unlock ctx->qwh.lock

lock ctx->qwh.lock
*cnt = (ctx->flags & EFD_SEMAPHORE) ? 1 : ctx->count; ctx->count -= *cnt;
**waitqueue_active is false**
unlock ctx->qwh.lock

Quoted 7 lines

	 *     lock ctx->wqh.lock (in poll_wait)
	 *     __add_wait_queue
	 *     unlock ctx->wqh.lock
	 *     eventfd_poll returns 0
	 */
	count = READ_ONCE(ctx->count);

No, it's simply impossible. The same comment explains why: "count = ctx->count" cannot move above poll_wait's locking of ctx->wqh.lock.

Detect concurrent reader and writer by reading event counter before and after poll_wait(), and determine feedback with the case of unstable counter taken into account.

Cut the big comment as the added barriers speak for themselves.

+++ x/fs/eventfd.c

(quoted diff) +11 -40

@@ -131,49 +131,20 @@ static __poll_t eventfd_poll(struct file
  {
  	struct eventfd_ctx *ctx = file->private_data;
  	__poll_t events = 0;
-	u64 count;
+	u64 c0, count;
+
+	c0 = ctx->count;
+	smp_rmb();
 
  	poll_wait(file, &ctx->wqh, wait);
 
-	/*
-	 * All writes to ctx->count occur within ctx->wqh.lock.  This read
-	 * can be done outside ctx->wqh.lock because we know that poll_wait
-	 * takes that lock (through add_wait_queue) if our caller will sleep.
-	 *
-	 * The read _can_ therefore seep into add_wait_queue's critical
-	 * section, but cannot move above it!  add_wait_queue's spin_lock acts
-	 * as an acquire barrier and ensures that the read be ordered properly
-	 * against the writes.  The following CAN happen and is safe:
-	 *
-	 *     poll                               write
-	 *     -----------------                  ------------
-	 *     lock ctx->wqh.lock (in poll_wait)
-	 *     count = ctx->count
-	 *     __add_wait_queue
-	 *     unlock ctx->wqh.lock
-	 *                                        lock ctx->qwh.lock
-	 *                                        ctx->count += n
-	 *                                        if (waitqueue_active)
-	 *                                          wake_up_locked_poll
-	 *                                        unlock ctx->qwh.lock
-	 *     eventfd_poll returns 0
-	 *
-	 * but the following, which would miss a wakeup, cannot happen:
-	 *
-	 *     poll                               write
-	 *     -----------------                  ------------
-	 *     count = ctx->count (INVALID!)
-	 *                                        lock ctx->qwh.lock
-	 *                                        ctx->count += n
-	 *                                        **waitqueue_active is false**
-	 *                                        **no wake_up_locked_poll!**
-	 *                                        unlock ctx->qwh.lock
-	 *     lock ctx->wqh.lock (in poll_wait)
-	 *     __add_wait_queue
-	 *     unlock ctx->wqh.lock
-	 *     eventfd_poll returns 0
-	 */
-	count = READ_ONCE(ctx->count);
+	smp_rmb();
+	count = ctx->count;
+
+	if (c0 < count)
+		return EPOLLIN;
+	if (c0 > count)
+		return EPOLLOUT;
 
  	if (count > 0)
  		events |= EPOLLIN;

Quoted 5 lines · Hillf Danton

Detect concurrent reader and writer by reading event counter before and after poll_wait(), and determine feedback with the case of unstable counter taken into account.

Cut the big comment as the added barriers speak for themselves.

First and foremost, I'm not sure what you are trying to fix.

Second, the patch is wrong even without taking into account the lockless accesses, because the condition for returning EPOLLOUT is certainly wrong.

Third, barriers very rarely speak for themselves. In particular what do they pair with?

It seems to me that you are basically reintroducing
the same mistake that commit a484c3dd9426 ("eventfd: document lockless access in eventfd_poll", 2016-03-22) fixed, at the time where the big comment was introduced:

Things aren't as simple as the read barrier in eventfd_poll would suggest. In fact, the read barrier, besides lacking a comment, is not paired in any obvious manner with another read barrier, and it is pointless because it is sitting between a write (deep in poll_wait) and the read of ctx->count.

Paolo

Quoted 63 lines

+++ x/fs/eventfd.c

(quoted diff) +11 -40

@@ -131,49 +131,20 @@ static __poll_t eventfd_poll(struct file
  {
  	struct eventfd_ctx *ctx = file->private_data;
  	__poll_t events = 0;
-	u64 count;
+	u64 c0, count;
+
+	c0 = ctx->count;
+	smp_rmb();
 
  	poll_wait(file, &ctx->wqh, wait);
 
-	/*
-	 * All writes to ctx->count occur within ctx->wqh.lock.  This read
-	 * can be done outside ctx->wqh.lock because we know that poll_wait
-	 * takes that lock (through add_wait_queue) if our caller will sleep.
-	 *
-	 * The read _can_ therefore seep into add_wait_queue's critical
-	 * section, but cannot move above it!  add_wait_queue's spin_lock acts
-	 * as an acquire barrier and ensures that the read be ordered properly
-	 * against the writes.  The following CAN happen and is safe:
-	 *
-	 *     poll                               write
-	 *     -----------------                  ------------
-	 *     lock ctx->wqh.lock (in poll_wait)
-	 *     count = ctx->count
-	 *     __add_wait_queue
-	 *     unlock ctx->wqh.lock
-	 *                                        lock ctx->qwh.lock
-	 *                                        ctx->count += n
-	 *                                        if (waitqueue_active)
-	 *                                          wake_up_locked_poll
-	 *                                        unlock ctx->qwh.lock
-	 *     eventfd_poll returns 0
-	 *
-	 * but the following, which would miss a wakeup, cannot happen:
-	 *
-	 *     poll                               write
-	 *     -----------------                  ------------
-	 *     count = ctx->count (INVALID!)
-	 *                                        lock ctx->qwh.lock
-	 *                                        ctx->count += n
-	 *                                        **waitqueue_active is false**
-	 *                                        **no wake_up_locked_poll!**
-	 *                                        unlock ctx->qwh.lock
-	 *     lock ctx->wqh.lock (in poll_wait)
-	 *     __add_wait_queue
-	 *     unlock ctx->wqh.lock
-	 *     eventfd_poll returns 0
-	 */
-	count = READ_ONCE(ctx->count);
+	smp_rmb();
+	count = ctx->count;
+
+	if (c0 < count)
+		return EPOLLIN;
+	if (c0 > count)
+		return EPOLLOUT;
 
  	if (count > 0)
  		events |= EPOLLIN;

Quoted 1 line · On Fri, 23 Jul 2021 09:59:55 +0200 Paolo Bonzini wrote:

First and foremost, I'm not sure what you are trying to fix.

The change proposed builds the return value without assuming that the event count is stable across poll_wait(). If it is unstable then we know there are concurrent reader and/or writer who both are ingnored currently.

Quoted 2 lines

Second, the patch is wrong even without taking into account the lockless accesses, because the condition for returning EPOLLOUT is certainly wrong.

Given it is detected that event count was consumed, there is room, though as racy as it is, in the event count for writer to make some progress.

Quoted 2 lines

Third, barriers very rarely speak for themselves. In particular what do they pair with?

There is no need to consider pair frankly. Barriers are just readded for removing the seep in the comment. Then the comment goes with the seep.

What the comment does not cover is the cases of more-than-two-party race.

Quoted 1 line · Hillf Danton

What the comment does not cover is the cases of more-than-two-party race.

But, you still haven't explained what's the bug there.

		lock WQ
		count += n
		no waiter
		unlock WQ

lock WQ
add waiter for EPOLLIN unlock WQ

lock WQ count = 0 wakeup EPOLLOUT unlock WQ

lock WQ
count += n
no waiter
unlock WQ

------------------------------- c1 = count

On 14/07/21 11:23, Jason Wang wrote:

(quoted diff) +5 -1

+static local_lock_t eventfd_wake_lock = INIT_LOCAL_LOCK(eventfd_wake_lock);
  DEFINE_PER_CPU(int, eventfd_wake_count);
 
  static DEFINE_IDA(eventfd_ida);

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
  	 * it returns true, the eventfd_signal() call should be deferred to a
  	 * safe context.
  	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	local_lock(&eventfd_wake_lock);
+	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+		local_unlock(&eventfd_wake_lock);
  		return 0;
+	}
 
  	spin_lock_irqsave(&ctx->wqh.lock, flags);
  	this_cpu_inc(eventfd_wake_count);

Quoted 20 lines · Paolo Bonzini

On 14/07/21 11:23, Jason Wang wrote:

fs/eventfd.c +5 -1

+static local_lock_t eventfd_wake_lock = INIT_LOCAL_LOCK(eventfd_wake_lock);
   DEFINE_PER_CPU(int, eventfd_wake_count);
 
   static DEFINE_IDA(eventfd_ida);

@@ -71,8 +73,11 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
   	 * it returns true, the eventfd_signal() call should be deferred to a
   	 * safe context.
   	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	local_lock(&eventfd_wake_lock);
+	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count))) {
+		local_unlock(&eventfd_wake_lock);
   		return 0;
+	}
 
   	spin_lock_irqsave(&ctx->wqh.lock, flags);
   	this_cpu_inc(eventfd_wake_count);

fs/eventfd.c __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n) · line 91

Quoted 4 lines

fs/eventfd.c +1 -0

@@ -83,6 +88,7 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
   		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
   	this_cpu_dec(eventfd_wake_count);
   	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+	local_unlock(&eventfd_wake_lock);

Yes, that cures it, but if you think about what this wants to prevent, then having the recursion counter per CPU is at least suboptimal.

Something like the untested below perhaps?

Thanks,

tglx
---

fs/aio.c +1 -1

@@ -1695,7 +1695,7 @@ static int aio_poll_wake(struct wait_que
  		list_del(&iocb->ki_list);
  		iocb->ki_res.res = mangle_poll(mask);
  		req->done = true;
-		if (iocb->ki_eventfd && eventfd_signal_count()) {
+		if (iocb->ki_eventfd && !eventfd_signal_allowed()) {
  			iocb = NULL;
  			INIT_WORK(&req->work, aio_poll_put_work);
  			schedule_work(&req->work);

fs/eventfd.c +5 -7

@@ -25,8 +25,6 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
-DEFINE_PER_CPU(int, eventfd_wake_count);
-
  static DEFINE_IDA(eventfd_ida);
 
  struct eventfd_ctx {

@@ -67,21 +65,21 @@ struct eventfd_ctx {
  	 * Deadlock or stack overflow issues can happen if we recurse here
  	 * through waitqueue wakeup handlers. If the caller users potentially
  	 * nested waitqueues with custom wakeup handlers, then it should
-	 * check eventfd_signal_count() before calling this function. If
-	 * it returns true, the eventfd_signal() call should be deferred to a
+	 * check eventfd_signal_allowed() before calling this function. If
+	 * it returns false, the eventfd_signal() call should be deferred to a
  	 * safe context.
  	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	if (WARN_ON_ONCE(current->in_eventfd_signal))
  		return 0;
 
  	spin_lock_irqsave(&ctx->wqh.lock, flags);
-	this_cpu_inc(eventfd_wake_count);
+	current->in_eventfd_signal = 1;
  	if (ULLONG_MAX - ctx->count < n)
  		n = ULLONG_MAX - ctx->count;
  	ctx->count += n;
  	if (waitqueue_active(&ctx->wqh))
  		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
-	this_cpu_dec(eventfd_wake_count);
+	current->in_eventfd_signal = 0;
  	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
 
  	return n;

include/linux/eventfd.h +4 -6

@@ -43,11 +43,9 @@ int eventfd_ctx_remove_wait_queue(struct
  				  __u64 *cnt);
  void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt);
 
-DECLARE_PER_CPU(int, eventfd_wake_count);
-
-static inline bool eventfd_signal_count(void)
+static inline bool eventfd_signal_allowed(void)
  {
-	return this_cpu_read(eventfd_wake_count);
+	return !current->in_eventfd_signal;
  }
 
  #else /* CONFIG_EVENTFD */

@@ -78,9 +76,9 @@ static inline int eventfd_ctx_remove_wai
  	return -ENOSYS;
  }
 
-static inline bool eventfd_signal_count(void)
+static inline bool eventfd_signal_allowed(void)
  {
-	return false;
+	return true;
  }
 
  static inline void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt)

include/linux/sched.h +2 -0

@@ -863,6 +863,8 @@ struct task_struct {
  	/* Used by page_owner=on to detect recursion in page tracking. */
  	unsigned			in_page_owner:1;
  #endif
+	/* Recursion prevention for eventfd_signal() */
+	unsigned			in_eventfd_signal:1;
 
  	unsigned long			atomic_flags; /* Flags requiring atomic access. */
 

Quoted 5 lines · Thomas Gleixner

On Wed, Jul 14 2021 at 12:35, Paolo Bonzini wrote:
Yes, that cures it, but if you think about what this wants to prevent, then having the recursion counter per CPU is at least suboptimal.

Something like the untested below perhaps?

Yes, that works (it should just be #ifdef CONFIG_EVENTFD).

On !PREEMPT_RT the percpu variable consumes memory while your patch uses none (there are plenty of spare bits in current), but it is otherwise basically the same. On PREEMPT_RT the local_lock is certainly more expensive.

The recursion protection for eventfd_signal() is based on a per CPU variable and relies on the !RT semantics of spin_lock_irqsave() for protecting this per CPU variable. On RT kernels spin_lock_irqsave() neither disables preemption nor interrupts which allows the spin lock held section to be preempted. If the preempting task invokes eventfd_signal() as well, then the recursion warning triggers.

Paolo suggested to protect the per CPU variable with a local lock, but that's heavyweight and actually not necessary. The goal of this protection is to prevent the task stack from overflowing, which can be achieved with a per task recursion protection as well.

Replace the per CPU variable with a per task bit similar to other recursion protection bits like task_struct::in_page_owner. This works on both !RT and RT kernels and removes as a side effect the extra per CPU storage.

No functional change for !RT kernels.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

The recursion protection for eventfd_signal() is based on a per CPU variable and relies on the !RT semantics of spin_lock_irqsave() for protecting this per CPU variable. On RT kernels spin_lock_irqsave() neither disables preemption nor interrupts which allows the spin lock held section to be preempted. If the preempting task invokes eventfd_signal() as well, then the recursion warning triggers.

Paolo suggested to protect the per CPU variable with a local lock, but that's heavyweight and actually not necessary. The goal of this protection is to prevent the task stack from overflowing, which can be achieved with a per task recursion protection as well.

Replace the per CPU variable with a per task bit similar to other recursion protection bits like task_struct::in_page_owner. This works on both !RT and RT kernels and removes as a side effect the extra per CPU storage.

No functional change for !RT kernels.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

The recursion protection for eventfd_signal() is based on a per CPU variable and relies on the !RT semantics of spin_lock_irqsave() for protecting this per CPU variable. On RT kernels spin_lock_irqsave() neither disables preemption nor interrupts which allows the spin lock held section to be preempted. If the preempting task invokes eventfd_signal() as well, then the recursion warning triggers.

Paolo suggested to protect the per CPU variable with a local lock, but that's heavyweight and actually not necessary. The goal of this protection is to prevent the task stack from overflowing, which can be achieved with a per task recursion protection as well.

Replace the per CPU variable with a per task bit similar to other recursion protection bits like task_struct::in_page_owner. This works on both !RT and RT kernels and removes as a side effect the extra per CPU storage.

No functional change for !RT kernels.

Reported-by Daniel Bristot de Oliveira <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  fs/aio.c                |    2 +-
  fs/eventfd.c            |   12 +++++-------
  include/linux/eventfd.h |   11 +++++------
  include/linux/sched.h   |    4 ++++
  4 files changed, 15 insertions(+), 14 deletions(-)

fs/aio.c +1 -1

@@ -1695,7 +1695,7 @@ static int aio_poll_wake(struct wait_que
  		list_del(&iocb->ki_list);
  		iocb->ki_res.res = mangle_poll(mask);
  		req->done = true;
-		if (iocb->ki_eventfd && eventfd_signal_count()) {
+		if (iocb->ki_eventfd && eventfd_signal_allowed()) {
  			iocb = NULL;
  			INIT_WORK(&req->work, aio_poll_put_work);
  			schedule_work(&req->work);

fs/eventfd.c +5 -7

@@ -25,8 +25,6 @@
  #include <linux/idr.h>
  #include <linux/uio.h>
 
-DEFINE_PER_CPU(int, eventfd_wake_count);
-
  static DEFINE_IDA(eventfd_ida);
 
  struct eventfd_ctx {

@@ -67,21 +65,21 @@ struct eventfd_ctx {
  	 * Deadlock or stack overflow issues can happen if we recurse here
  	 * through waitqueue wakeup handlers. If the caller users potentially
  	 * nested waitqueues with custom wakeup handlers, then it should
-	 * check eventfd_signal_count() before calling this function. If
-	 * it returns true, the eventfd_signal() call should be deferred to a
+	 * check eventfd_signal_allowed() before calling this function. If
+	 * it returns false, the eventfd_signal() call should be deferred to a
  	 * safe context.
  	 */
-	if (WARN_ON_ONCE(this_cpu_read(eventfd_wake_count)))
+	if (WARN_ON_ONCE(current->in_eventfd_signal))
  		return 0;
 
  	spin_lock_irqsave(&ctx->wqh.lock, flags);
-	this_cpu_inc(eventfd_wake_count);
+	current->in_eventfd_signal = 1;
  	if (ULLONG_MAX - ctx->count < n)
  		n = ULLONG_MAX - ctx->count;
  	ctx->count += n;
  	if (waitqueue_active(&ctx->wqh))
  		wake_up_locked_poll(&ctx->wqh, EPOLLIN);
-	this_cpu_dec(eventfd_wake_count);
+	current->in_eventfd_signal = 0;
  	spin_unlock_irqrestore(&ctx->wqh.lock, flags);
 
  	return n;

include/linux/eventfd.h +5 -6

@@ -14,6 +14,7 @@
  #include <linux/err.h>
  #include <linux/percpu-defs.h>
  #include <linux/percpu.h>
+#include <linux/sched.h>
 
  /*
   * CAREFUL: Check include/uapi/asm-generic/fcntl.h when defining

@@ -43,11 +44,9 @@ int eventfd_ctx_remove_wait_queue(struct
  				  __u64 *cnt);
  void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt);
 
-DECLARE_PER_CPU(int, eventfd_wake_count);
-
-static inline bool eventfd_signal_count(void)
+static inline bool eventfd_signal_allowed(void)
  {
-	return this_cpu_read(eventfd_wake_count);
+	return !current->in_eventfd_signal;
  }
 
  #else /* CONFIG_EVENTFD */

@@ -78,9 +77,9 @@ static inline int eventfd_ctx_remove_wai
  	return -ENOSYS;
  }
 
-static inline bool eventfd_signal_count(void)
+static inline bool eventfd_signal_allowed(void)
  {
-	return false;
+	return true;
  }
 
  static inline void eventfd_ctx_do_read(struct eventfd_ctx *ctx, __u64 *cnt)

include/linux/sched.h +4 -0

@@ -863,6 +863,10 @@ struct task_struct {
  	/* Used by page_owner=on to detect recursion in page tracking. */
  	unsigned			in_page_owner:1;
  #endif
+#ifdef CONFIG_EVENTFD
+	/* Recursion prevention for eventfd_signal() */
+	unsigned			in_eventfd_signal:1;
+#endif
 
  	unsigned long			atomic_flags; /* Flags requiring atomic access. */