read(2) on pipes sometimes misses EOF (reproducible code attached) #665

haesbaert · 2022-09-26T14:28:45Z

I've noticed some hangs in our tests of Eio when using ocaml-uring (ocaml-multicore/eio#319).

I've broken it down to a simple C program that triggers it in a few seconds: https://gist.github.com/haesbaert/10d3e3bb5fa9171dfcf65e1f5b58e95c (apologies for not cleaning it up more)

Basically it creates a pipe, writes on one end, and closes. Running that in a loop causes sometimes the EOF to be "lost":

cc -o uring_tests uring_tests.c -Wall -luring && while true;do date; ./uring_tests;done
Mon 26 Sep 16:22:35 CEST 2022
read(A) cqe->res = 6 (buf=foobar)
read(B) cqe->res = 0
Mon 26 Sep 16:22:35 CEST 2022
read(A) cqe->res = 6 (buf=foobar)
read(B) cqe->res = 0
Mon 26 Sep 16:22:35 CEST 2022
read(A) cqe->res = 6 (buf=foobar)
read(B) cqe->res = 0
Mon 26 Sep 16:22:35 CEST 2022
read(A) cqe->res = 6 (buf=foobar)
---> hangs here waiting on read (confirmed via strace)

The problem seems to go away if I turn both pipe FDs to non-blocking, which would make me happy if I hadn't problems trying to splice(2) non-blocking pipes, but that's another issue that needs more investigation.

Kernel:
Linux sam 5.18.16-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 03 Aug 2022 11:25:04 +0000 x86_64 GNU/Linux

The text was updated successfully, but these errors were encountered:

axboe · 2022-09-29T19:50:14Z

I'll take a look, thanks for the report!

axboe · 2022-09-29T21:20:37Z

I think I see what may be happening. Are you able to test a kernel patch?

haesbaert · 2022-09-29T21:26:08Z

I think I see what may be happening. Are you able to test a kernel patch?

It's been years but I can figure out how to build it.

axboe · 2022-09-29T21:28:48Z

Here's the patch, fwiw:

diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 177bd55357d7..48ce2348c8c1 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -231,11 +231,11 @@ static inline unsigned int io_sqring_entries(struct io_ring_ctx *ctx)
 
 static inline int io_run_task_work(void)
 {
-	if (test_thread_flag(TIF_NOTIFY_SIGNAL)) {
+	if (task_work_pending(current)) {
+		if (test_thread_flag(TIF_NOTIFY_SIGNAL))
+			clear_notify_signal();
 		__set_current_state(TASK_RUNNING);
-		clear_notify_signal();
-		if (task_work_pending(current))
-			task_work_run();
+		task_work_run();
 		return 1;
 	}

What's happening here is that when a pipe is closed, and it's the last user, then fput() queues up task_work in the kernel. The ->release() method is called off that task_work, and EOF isn't triggered (or in our case, EPOLLHUP) before this happens. The io_uring wait logic only checks for signal based task_work, which fput() doesn't use. This means we could be waiting for events with the fput() task_work pending, and it'd never be run. Hence the read would sit forever with poll armed and not get triggered.

The above patch just makes it so that we do notice non-TWA_SIGNAL task_work. It's for the current tree, will be happy to provide a backport for 5.18-stable if that is what you are running?

axboe · 2022-09-29T21:32:16Z

Also, would be great if you could provide your real name and email so I can properly attribute the patch to your report. This is generally done with a Reported-by: Real Name my@email.com in the kernel. If you don't want that, I'm also fine just linking to this bug report. I'll do the latter anyway.

axboe · 2022-09-29T21:34:36Z

It's been years but I can figure out how to build it.

Excellent, I'll spin a 5.18-stable version of the patch.

And let me not forget to state just how great it is to have a bug report with a reproducer. It makes everything so much faster in figuring out the issue, when I don't have to spend a lot of time trying to come up with a reproducer myself!

haesbaert · 2022-09-29T21:40:56Z

That's sweet 😄 , I can appreciate it :) I've spent ~10 years hacking a BSD kernel, reproducible reports are nice !
I'll be on 5.19.12, I'm just updating my machine before, then I'll build it and let you know, might take me a day though.

The real name for the report is Christiano Haesbaert haesbaert@haesbaert.org
Thanks a lot for looking into this, I think I have a couple of more bugs to report, I just want to isolate them better.

axboe · 2022-09-29T22:01:37Z

OK, I'll prep one for 5.19-stable! And yes, reproducible bug reports are nice for ANY project. It's the best way to make the experience scale, there's only so much time to go around for everyone...

axboe · 2022-09-29T22:07:31Z

5.19-stable.txt

Here's the patch for 5.19-stable.

axboe · 2022-09-29T22:08:28Z

The real name for the report is Christiano Haesbaert haesbaert@haesbaert.org

Added, thanks!

This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk>

axboe · 2022-09-29T23:50:29Z

I'll preemptive close this one so I don't forget, just reopen if there's anything else to talk about on this particular issue.

haesbaert · 2022-09-29T23:53:23Z

I'll preemptive close this one so I don't forget, just reopen if there's anything else to talk about on this particular issue.

Just to let you know I've tested the patch and it fixes for me well, many thanks for the awesome support 👯

axboe · 2022-09-29T23:54:21Z

Great, thanks for the quick testing too!

DylanZA · 2022-09-30T12:04:26Z

should we add this reproducer as a test case? @haesbaert are you happy for that?

haesbaert · 2022-09-30T12:18:23Z

should we add this reproducer as a test case? @haesbaert are you happy for that?

Absolutely 😄

add a test [dylany reformatted it a bit] Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

DylanZA · 2022-09-30T12:36:53Z

ok there is a branch linked - I'm not really sure how to format this to give you attribution - @axboe can tell me what I did wrong if I did

add a test [dylany reformatted it a bit] Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

add a test for "io_uring: don't gate task_work run on TIF_NOTIFY_SIGNAL" [dylany reformatted it a bit] Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

add a test for "io_uring: don't gate task_work run on TIF_NOTIFY_SIGNAL" Based on a test case from Christiano Haesbaert <haesbaert@haesbaert.org> Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

axboe · 2022-09-30T14:01:34Z

Pulled in, thanks @DylanZA

This issue turned out to be a kernel bug which has been fixed in: torvalds/linux@46a525e Reported here: axboe/liburing#665 (comment) We can workaround it by making sure pipes are non-blocking, uring considers pipes unbounded work and relies on uring worker threads if they are blocking, the bug is only triggered if this is the case, so force them to be non-blocking. If we use splice, the splice call itself needs the worker threads and the bug surfaces again (I verified this with perf probes), so disable splice for now. Even with the fix, it's desirable to keep pipes as non-blocking to avoid thread pooling. The splice call can return EAGAIN in uring, this happens even with the kernel patched, so handle it for the future. We can tune this better by disabling splice only for the unpatched kernels.

[ Upstream commit 46a525e ] This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: I042b07491afac06692639d91bdf7dd21a2405651 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Bug: 268174392 (cherry picked from commit 2fd232b) Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

Source: Kernel.org MR: 125008 Type: Integration Disposition: Backport from git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable linux-5.10.y ChangeID: 2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9 Description: commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Signed-off-by: Armin Kuster <akuster@mvista.com>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: Iafd72e8b4854e1b86adcd8b611d167f52fcbb970 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> (cherry picked from commit a9aa4aa) Bug: 268174392 Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: I042b07491afac06692639d91bdf7dd21a2405651 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Bug: 268174392 (cherry picked from commit 2fd232b) Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

BugLink: https://bugs.launchpad.net/bugs/2008933 commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Signed-off-by: Kamal Mostafa <kamal@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: I042b07491afac06692639d91bdf7dd21a2405651 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Bug: 268174392 (cherry picked from commit 2fd232b) Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

stable inclusion from stable-v5.10.165 commit 2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9 category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I7T7G4 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9 -------------------------------- commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Signed-off-by: sanglipeng <sanglipeng1@jd.com>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> (cherry picked from commit 2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9) Orabug: 35495339 Signed-off-by: Prasad Singamsetty <prasad.singamsetty@oracle.com> Reviewed-by: Alan Adamson <alan.adamson@oracle.com> Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> Signed-off-by: Brian Maly <brian.maly@oracle.com>

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: I042b07491afac06692639d91bdf7dd21a2405651 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Bug: 268174392 (cherry picked from commit 2fd232b) Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

stable inclusion from stable-v5.10.165 commit 2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9 category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I7T7G4 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=2fd232bbd66fafdf7af3cc155c7bb82d96d07bc9 -------------------------------- commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Signed-off-by: sanglipeng <sanglipeng1@jd.com> (cherry picked from commit bfa6c0a)

commit 46a525e upstream. This isn't a reliable mechanism to tell if we have task_work pending, we really should be looking at whether we have any items queued. This is problematic if forward progress is gated on running said task_work. One such example is reading from a pipe, where the write side has been closed right before the read is started. The fput() of the file queues TWA_RESUME task_work, and we need that task_work to be run before ->release() is called for the pipe. If ->release() isn't called, then the read will sit forever waiting on data that will never arise. Fix this by io_run_task_work() so it checks if we have task_work pending rather than rely on TIF_NOTIFY_SIGNAL for that. The latter obviously doesn't work for task_work that is queued without TWA_SIGNAL. Reported-by: Christiano Haesbaert <haesbaert@haesbaert.org> Cc: stable@vger.kernel.org Link: axboe/liburing#665 Change-Id: I042b07491afac06692639d91bdf7dd21a2405651 Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Sasha Levin <sashal@kernel.org> Bug: 268174392 (cherry picked from commit 2fd232b) Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>

haesbaert mentioned this issue Sep 26, 2022

Hang in eio_linux tests ocaml-multicore/eio#319

Closed

axboe closed this as completed Sep 29, 2022

DylanZA pushed a commit to DylanZA/liburing that referenced this issue Sep 30, 2022

io_uring: add a test for missing task work

bccdb25

add a test [dylany reformatted it a bit] Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

DylanZA pushed a commit to DylanZA/liburing that referenced this issue Sep 30, 2022

io_uring: add a test for missing task work

d147e51

add a test [dylany reformatted it a bit] Link: axboe#665 Signed-off-by: Dylan Yudaken <dylany@fb.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read(2) on pipes sometimes misses EOF (reproducible code attached) #665

read(2) on pipes sometimes misses EOF (reproducible code attached) #665

haesbaert commented Sep 26, 2022 •

edited

Loading

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

DylanZA commented Sep 30, 2022

haesbaert commented Sep 30, 2022

DylanZA commented Sep 30, 2022

axboe commented Sep 30, 2022

read(2) on pipes sometimes misses EOF (reproducible code attached) #665

read(2) on pipes sometimes misses EOF (reproducible code attached) #665

Comments

haesbaert commented Sep 26, 2022 • edited Loading

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

axboe commented Sep 29, 2022

haesbaert commented Sep 29, 2022

axboe commented Sep 29, 2022

DylanZA commented Sep 30, 2022

haesbaert commented Sep 30, 2022

DylanZA commented Sep 30, 2022

axboe commented Sep 30, 2022

haesbaert commented Sep 26, 2022 •

edited

Loading