Re: [patch 13/15] epoll: check ep_events_available() upon timeout

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Mon, Nov 2, 2020 at 10:51 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'll go stare at it some more.

That code is fundamentally broken in so many ways.

Look at how  ep_poll() does that ep_events_available() without
actually holding the ep->lock (or the ep->mtx) in half the cases.

End result: it works in 99.9% of all cases, but there's a race with
somebody else doing

        WRITE_ONCE(ep->ovflist, EP_UNACTIVE_PTR);
        /*
         * Quickly re-inject items left on "txlist".
         */
        list_splice(&txlist, &ep->rdllist);

when that code can see an empty rdllist and an inactive ovflist and
thus decide that there are no events available.

I think the "Quickly re-inject" comment may be because some people
knew of that race.

The signal handling is also odd and looks broken.

The "short circuit on fatal signals" means that ep_send_events() isn't
actually done on a SIGKILL, but the code also used an exclusive wait,
so nobody else will be woken up either.

Admittedly you can steal wakeups other ways, by simply not caring
about the end result, so maybe that's all just inherent in epoll
anyway. But it looks strange, and it seems pointless: the right thing
to do would seem to be simply to have a regular check for
signal_pending(), and returning -EINTR if rather than looping.

And that do { } while (0) is entirely pointless. It seems to exist in
order to use "break" instead of the goto that everything else does,
which I guess is nice, except the whole need for that comes from how
oddly the code is written.

Why doesn't this all do something like the attached instead?

NOTE! I did not bother to fix that ep_events_available() race.

                      Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 4551 bytes --]

 fs/eventpoll.c | 114 +++++++++++++++++++++++++++------------------------------
 1 file changed, 54 insertions(+), 60 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 4df61129566d..d4732327f57a 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1864,85 +1864,79 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
 	 */
 	ep_reset_busy_poll_napi_id(ep);
 
-	do {
-		/*
-		 * Internally init_wait() uses autoremove_wake_function(),
-		 * thus wait entry is removed from the wait queue on each
-		 * wakeup. Why it is important? In case of several waiters
-		 * each new wakeup will hit the next waiter, giving it the
-		 * chance to harvest new event. Otherwise wakeup can be
-		 * lost. This is also good performance-wise, because on
-		 * normal wakeup path no need to call __remove_wait_queue()
-		 * explicitly, thus ep->lock is not taken, which halts the
-		 * event delivery.
-		 */
-		init_wait(&wait);
+	if (signal_pending(current))
+		return -EINTR;
 
-		write_lock_irq(&ep->lock);
-		/*
-		 * Barrierless variant, waitqueue_active() is called under
-		 * the same lock on wakeup ep_poll_callback() side, so it
-		 * is safe to avoid an explicit barrier.
-		 */
-		__set_current_state(TASK_INTERRUPTIBLE);
-
-		/*
-		 * Do the final check under the lock. ep_scan_ready_list()
-		 * plays with two lists (->rdllist and ->ovflist) and there
-		 * is always a race when both lists are empty for short
-		 * period of time although events are pending, so lock is
-		 * important.
-		 */
-		eavail = ep_events_available(ep);
-		if (!eavail) {
-			if (signal_pending(current))
-				res = -EINTR;
-			else
-				__add_wait_queue_exclusive(&ep->wq, &wait);
-		}
-		write_unlock_irq(&ep->lock);
-
-		if (eavail || res)
-			break;
+	/*
+	 * Internally init_wait() uses autoremove_wake_function(),
+	 * thus wait entry is removed from the wait queue on each
+	 * wakeup. Why it is important? In case of several waiters
+	 * each new wakeup will hit the next waiter, giving it the
+	 * chance to harvest new event. Otherwise wakeup can be
+	 * lost. This is also good performance-wise, because on
+	 * normal wakeup path no need to call __remove_wait_queue()
+	 * explicitly, thus ep->lock is not taken, which halts the
+	 * event delivery.
+	 */
+	init_wait(&wait);
 
-		if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS)) {
-			timed_out = 1;
-			break;
-		}
+	write_lock_irq(&ep->lock);
+	/*
+	 * Barrierless variant, waitqueue_active() is called under
+	 * the same lock on wakeup ep_poll_callback() side, so it
+	 * is safe to avoid an explicit barrier.
+	 */
+	__set_current_state(TASK_INTERRUPTIBLE);
 
-		/* We were woken up, thus go and try to harvest some events */
-		eavail = 1;
+	/*
+	 * Do the final check under the lock. ep_scan_ready_list()
+	 * plays with two lists (->rdllist and ->ovflist) and there
+	 * is always a race when both lists are empty for short
+	 * period of time although events are pending, so lock is
+	 * important.
+	 */
+	eavail = ep_events_available(ep);
+	if (!eavail)
+		__add_wait_queue_exclusive(&ep->wq, &wait);
+	write_unlock_irq(&ep->lock);
 
-	} while (0);
+	if (!eavail)
+		timed_out = !schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS);
 
 	__set_current_state(TASK_RUNNING);
 
+	/*
+	 * If we were woken up, assume there's something available, otherwise
+	 * remove ourselves from the wait queue and check carefully (since we
+	 * hold the lock anyway).
+	 */
+	eavail = 1;
 	if (!list_empty_careful(&wait.entry)) {
 		write_lock_irq(&ep->lock);
 		__remove_wait_queue(&ep->wq, &wait);
+		eavail = ep_events_available(ep);
 		write_unlock_irq(&ep->lock);
 	}
 
 send_events:
-	if (fatal_signal_pending(current)) {
-		/*
-		 * Always short-circuit for fatal signals to allow
-		 * threads to make a timely exit without the chance of
-		 * finding more events available and fetching
-		 * repeatedly.
-		 */
-		res = -EINTR;
+	if (res)
+		return res;
+	if (eavail) {
+		res = ep_send_events(ep, events, maxevents);
+		if (res)
+			return res;
 	}
+	if (signal_pending(current))
+		return -EINTR;
+
 	/*
-	 * Try to transfer events to user space. In case we get 0 events and
-	 * there's still timeout left over, we go trying again in search of
-	 * more luck.
+	 * In case we get 0 events and there's still timeout left over, we
+	 * go trying again in search of more luck.
 	 */
-	if (!res && eavail &&
-	    !(res = ep_send_events(ep, events, maxevents)) && !timed_out)
+	if (!timed_out)
 		goto fetch_events;
 
-	return res;
+	return 0;
 }
 
 /**