[PATCH] sched/wait: Queue non-exclusive waiters at the head in do_wait_intr*()

[PATCH] sched/wait: Queue non-exclusive waiters at the head in do_wait_intr*()

[Mani Tofigh via B4 Relay]

From: Mani Tofigh <mt4022@columbia.edu>

A task blocked in read() on an eventfd or timerfd can sleep forever
while the file is readable, if an epoll(EPOLLEXCLUSIVE) watcher of the
same file was added before it.

eventfd and timerfd wait with wait_event_interruptible_locked_irq().
do_wait_intr_irq() adds the reader at the tail, behind the exclusive
epoll entry, and __wake_up_common() stops after waking that entry.  If
the woken epoll task is killed or leaves without reading, nothing wakes
the reader:

  Task E                   Task A                 Task C
  epoll_wait(ep, -1)
    /* EPOLLEXCLUSIVE */
                           read(efd)
                             /* queued after E */
                                                  write(efd, 1)
                                                    /* wakes E, stops */
                                                  kill(E, SIGKILL)
                           /* still asleep, efd is readable */

A periodic timerfd doesn't wake A either, since the timer is restarted
only by a read.

prepare_to_wait_event() and add_wait_queue() add non-exclusive waiters
at the head, which is what __wake_up_common() relies on.  eventfd did
the same itself before it switched to these helpers.

Do the same in do_wait_intr() and do_wait_intr_irq().  This only changes
the order in which non-exclusive waiters are woken.  Each of them still
gets every wakeup.  With the fix, A is woken by the write.

This bug was identified and confirmed with the help of a deterministic
hypervisor currently under development.  The C reproducer below however
hits it within about 2s.

Fixes: 22c43c81a51e ("wait_event_interruptible_locked() interface")
Cc: stable@vger.kernel.org
Signed-off-by: Mani Tofigh <mt4022@columbia.edu>
---
Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/evexcl.c

gpiolib-cdev and cros_ec_chardev use the same helpers and can be
polled too, so this fixes them as well.
---
 kernel/sched/wait.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index f8d6d8b9d03e..342dec5fd937 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -319,8 +319,12 @@ EXPORT_SYMBOL(prepare_to_wait_event);
  */
 int do_wait_intr(wait_queue_head_t *wq, wait_queue_entry_t *wait)
 {
-	if (likely(list_empty(&wait->entry)))
-		__add_wait_queue_entry_tail(wq, wait);
+	if (likely(list_empty(&wait->entry))) {
+		if (wait->flags & WQ_FLAG_EXCLUSIVE)
+			__add_wait_queue_entry_tail(wq, wait);
+		else
+			__add_wait_queue(wq, wait);
+	}
 
 	set_current_state(TASK_INTERRUPTIBLE);
 	if (signal_pending(current))
@@ -336,8 +340,12 @@ EXPORT_SYMBOL(do_wait_intr);
 
 int do_wait_intr_irq(wait_queue_head_t *wq, wait_queue_entry_t *wait)
 {
-	if (likely(list_empty(&wait->entry)))
-		__add_wait_queue_entry_tail(wq, wait);
+	if (likely(list_empty(&wait->entry))) {
+		if (wait->flags & WQ_FLAG_EXCLUSIVE)
+			__add_wait_queue_entry_tail(wq, wait);
+		else
+			__add_wait_queue(wq, wait);
+	}
 
 	set_current_state(TASK_INTERRUPTIBLE);
 	if (signal_pending(current))

---
base-commit: d7e670601af6c564ccf6d3b2ddf70ee4f41a6d1c
change-id: 20261111-b4-sched-wait-intr-head-c5e8f0b541f4

Best regards,
--  
Mani Tofigh <mt4022@columbia.edu>

Re: [PATCH] sched/wait: Queue non-exclusive waiters at the head in do_wait_intr*()

[Oleg Nesterov]

On 11/11, Mani Tofigh via B4 Relay wrote:
>
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -319,8 +319,12 @@ EXPORT_SYMBOL(prepare_to_wait_event);
>   */
>  int do_wait_intr(wait_queue_head_t *wq, wait_queue_entry_t *wait)
>  {
> -	if (likely(list_empty(&wait->entry)))
> -		__add_wait_queue_entry_tail(wq, wait);
> +	if (likely(list_empty(&wait->entry))) {
> +		if (wait->flags & WQ_FLAG_EXCLUSIVE)
> +			__add_wait_queue_entry_tail(wq, wait);
> +		else
> +			__add_wait_queue(wq, wait);

This look right and matches prepare_to_wait_event(). But it seems there is
another problem if WQ_FLAG_EXCLUSIVE, see the
"Exclusive waiter must not fail if it was selected by wakeup" comment in
prepare_to_wait_event().

Perhaps it makes more sense to simply kill __wait_event_interruptible_locked()
and reimplement wait_event_interruptible_locked() on top of
___wait_event(cmd => do_wait_intr) ? This way do_wait_intr() will only need
to do unlock + schedule + lock.

Oleg.