Re: [PATCH 1/8] epoll: check for events when removing a timed out thread from the wait queue

[Davidlohr Bueso]

On Fri, 06 Nov 2020, Soheil Hassas Yeganeh wrote:

> From: Soheil Hassas Yeganeh <soheil@xxxxxxxxxx>
>
> After abc610e01c66 ("fs/epoll: avoid barrier after an epoll_wait(2)
> timeout"), we break out of the ep_poll loop upon timeout, without checking
> whether there is any new events available.  Prior to that patch-series we
> always called ep_events_available() after exiting the loop.
>
> This can cause races and missed wakeups.  For example, consider the
> following scenario reported by Guantao Liu:
>
> Suppose we have an eventfd added using EPOLLET to an epollfd.
>
> Thread 1: Sleeps for just below 5ms and then writes to an eventfd.
> Thread 2: Calls epoll_wait with a timeout of 5 ms. If it sees an
>          event of the eventfd, it will write back on that fd.
> Thread 3: Calls epoll_wait with a negative timeout.
>
> Prior to abc610e01c66, it is guaranteed that Thread 3 will wake up either
> by Thread 1 or Thread 2.  After abc610e01c66, Thread 3 can be blocked
> indefinitely if Thread 2 sees a timeout right before the write to the
> eventfd by Thread 1.  Thread 2 will be woken up from
> schedule_hrtimeout_range and, with evail 0, it will not call
> ep_send_events().
>
> To fix this issue:
> 1) Simplify the timed_out case as suggested by Linus.
> 2) while holding the lock, recheck whether the thread was woken up
>   after its time out has reached.
>
> Note that (2) is different from Linus' original suggestion: It do not
> set "eavail = ep_events_available(ep)" to avoid unnecessary contention
> (when there are too many timed-out threads and a small number of events),
> as well as races mentioned in the discussion thread.
>
> This is the first patch in the series so that the backport to stable
> releases is straightforward.
>
> Link: https://lkml.kernel.org/r/CAHk-=wizk=OxUyQPbO8MS41w2Pag1kniUV5WdD5qWL-gq1kjDA@xxxxxxxxxxxxxx
> Fixes: abc610e01c66 ("fs/epoll: avoid barrier after an epoll_wait(2) timeout")
> Tested-by: Guantao Liu <guantaol@xxxxxxxxxx>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@xxxxxxxxxx>
> Reported-by: Guantao Liu <guantaol@xxxxxxxxxx>
> Reviewed-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> Reviewed-by: Khazhismel Kumykov <khazhy@xxxxxxxxxx>

Thanks for providing the fix and a testcase.

Reviewed-by: Davidlohr Bueso <dbueso@xxxxxxx>