fifo: don't reopen a writer-less non-blocking reader as a closed pipe

[emirbuljubasic]

do_open_fifo() opens a temporary O_RDWR fake writer to unblock the restore, then opens the real descriptor while that writer is still present. For an O_RDONLY|O_NONBLOCK reader, like OpenRC's /run/openrc/init.ctl, this leaves every poll() returning POLLHUP instead of blocking, so an epoll loop spins at 100% CPU.

As far as I can tell, fifo_open() latches pipe->w_counter into filp->f_version only when such an end is opened with no writer around, and pipe_poll() suppresses POLLHUP while the two still match. So the fake writer has to be gone before the real descriptor is opened.

Doing that unconditionally is wrong, as @avagin pointed out: an end whose writer has already been opened and closed is a closed pipe and must keep reporting POLLHUP. The two states seem to differ nowhere but in poll(), so this tells them apart on dump by polling the descriptor drained from the dumpee (it is the dumpee's own struct file, so f_version is intact) and records the answer in a new fifo_entry.wait_writer. Images without the field keep the old behaviour.

Per @rst0git: closing the fake writer drops the last reference to the pipe and takes the just restored pipe size with it, so the pipe parameters are restored on the final descriptor in that case.

The test covers both states. It needs two fifos, since w_counter is per inode and a writer opened on one end hangs up every reader of that fifo. I checked that both assertions actually catch something: with the fake writer always kept the test fails on the waiting end, and with it always dropped it fails on @avagin's closed pipe reproducer. fifo, fifo_ro, fifo_wronly and fifo-ghost still pass.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 30.55556% with 25 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (criu-dev@5d8b3db). Learn more about missing BASE report.
⚠️ Report is 46 commits behind head on criu-dev.

Files with missing lines	Patch %	Lines
criu/fifo.c	30.55%	25 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             criu-dev    #3109   +/-   ##
===========================================
  Coverage            ?   57.75%           
===========================================
  Files               ?      161           
  Lines               ?    43951           
  Branches            ?     9644           
===========================================
  Hits                ?    25386           
  Misses              ?    18326           
  Partials            ?      239           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adrianreber]

The CentOS Stream 10 CI failures are unrelated. Looks like the iptables based network locking is not working. On CentOS Stream 10 we should switch to nftables. Let me do that.

[avagin]

LGTM. Usually, we put test and criu changes into separate patches.

[emirbuljubasic]

LGTM. Usually, we put test and criu changes into separate patches.

I split it into two commits.

[avagin]

Here is one more issue introduced by this patch. We need to distinguish two cases:

• a read-only descriptor created before the write one (waiting for the write one).
• the write end has been closed and the read-only descriptor basically a closed pipe.

Here is a reproducer for the second case:

diff --git a/test/zdtm/static/fifo_ro_nonblock.c b/test/zdtm/static/fifo_ro_nonblock.c
index ab0a5e902faa..711421b58187 100644
--- a/test/zdtm/static/fifo_ro_nonblock.c
+++ b/test/zdtm/static/fifo_ro_nonblock.c
@@ -30,7 +30,8 @@ static int hup(int fd, int timeout)
 
 int main(int argc, char **argv)
 {
-       int fd;
+       int fd, fdw;
+       char buf[1024];
 
        test_init(argc, argv);
 
@@ -57,9 +58,16 @@ int main(int argc, char **argv)
                return 1;
        }
 
+       fdw = open(filename, O_WRONLY);
+       close(fdw);
+
        test_daemon();
        test_waitsig();
 
+       if (read(fd, buf, 1) != 0) {
+               fail("fifo hasn't been closed");
+               return 1;
+       }
        /*
         * After restore poll() must still not report POLLHUP. If it does,
         * CRIU reopened the reader while a (fake) writer was present, and the
@@ -69,9 +77,12 @@ int main(int argc, char **argv)
        case -1:
                fail("poll() failed after restore");
                return 1;
-       case 1:
-               fail("fifo reports POLLHUP after restore -- would busy-loop");
+       case 0:
+               fail("no hup");
                return 1;
+       case 1:
+               //fail("fifo reports POLLHUP after restore -- would busy-loop");
+               break;
        }
 
        if (close(fd) < 0) {

[avagin]

@emirbuljubasic any update?

[emirbuljubasic]

@emirbuljubasic any update?

Sorry for the delay, I'm on vacation. Will be back on this on monday.

[emirbuljubasic]

Here is one more issue introduced by this patch. We need to distinguish two cases:

* a read-only descriptor created before the write one (waiting for the write one).

* the write end has been closed and the read-only descriptor basically a closed pipe.

Here is a reproducer for the second case:

diff --git a/test/zdtm/static/fifo_ro_nonblock.c b/test/zdtm/static/fifo_ro_nonblock.c
index ab0a5e902faa..711421b58187 100644
--- a/test/zdtm/static/fifo_ro_nonblock.c
+++ b/test/zdtm/static/fifo_ro_nonblock.c
@@ -30,7 +30,8 @@ static int hup(int fd, int timeout)
 
 int main(int argc, char **argv)
 {
-       int fd;
+       int fd, fdw;
+       char buf[1024];
 
        test_init(argc, argv);
 
@@ -57,9 +58,16 @@ int main(int argc, char **argv)
                return 1;
        }
 
+       fdw = open(filename, O_WRONLY);
+       close(fdw);
+
        test_daemon();
        test_waitsig();
 
+       if (read(fd, buf, 1) != 0) {
+               fail("fifo hasn't been closed");
+               return 1;
+       }
        /*
         * After restore poll() must still not report POLLHUP. If it does,
         * CRIU reopened the reader while a (fake) writer was present, and the
@@ -69,9 +77,12 @@ int main(int argc, char **argv)
        case -1:
                fail("poll() failed after restore");
                return 1;
-       case 1:
-               fail("fifo reports POLLHUP after restore -- would busy-loop");
+       case 0:
+               fail("no hup");
                return 1;
+       case 1:
+               //fail("fifo reports POLLHUP after restore -- would busy-loop");
+               break;
        }
 
        if (close(fd) < 0) {

@avagin Thanks, I completely missed that case. I've tried to handle it by telling the two apart on dump. The only difference I could find is in poll(), so I poll the descriptor drained from the dumpee (its own struct file, so f_version is still intact) and store the answer in a new fifo_entry.wait_writer. Restore only drops the fake writer when it's set, and images without he field keep the old behaviour.

I wasn't sure about adding a field to the image just for this, so if there's a better place to get this from, or you'd prefer a different approach entirely, I'm happy to redo it.

Your reproducer is in the test now, as a second fifo, since w_counter is per inode and the writer would otherwise hang up the first reader too. One thing I noticed while writing it: read() seems to return 0 in both cases, so I used the poll() check as the actual assertion. Let me know if I've got that wrong.

Thank you everybody for being patient with me and for the informative comments. This is very much outside of my comfort zone.

[Copilot]

Pull request overview

This PR fixes a FIFO restore corner case where an O_RDONLY|O_NONBLOCK reader that was originally opened without any writer can be restored in a way that makes poll() report POLLHUP continuously (causing busy-looping in epoll/poll driven daemons). It does so by recording, at dump time, whether the reader should “wait for a writer” (i.e., should not report POLLHUP) and then using that information to decide when to drop CRIU’s temporary fake writer during restore.

Changes:

• Add a new fifo_entry.wait_writer image field and populate it at dump time via a poll() probe on read-only FIFO fds.
• Adjust FIFO restore logic to optionally close the temporary O_RDWR “fake writer” before opening the real O_RDONLY|O_NONBLOCK fd (only for the “wait writer” state and only when no buffered FIFO data needs preserving).
• Add a new ZDTM test (fifo_ro_nonblock) covering both “wait for writer” and “closed pipe” FIFO poll semantics, and verifying pipe size restoration.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
test/zdtm/static/Makefile	Adds the new static ZDTM test to the build/run set.
test/zdtm/static/fifo_ro_nonblock.c	New regression test for POLLHUP behavior across C/R for `O_RDONLY
images/fifo.proto	Extends FIFO image schema with wait_writer to preserve poll semantics.
criu/fifo.c	Dumps wait_writer state and updates restore to drop the fake writer early only when appropriate.

Suppressed comments (1)

criu/fifo.c:188

• Same as above: add braces around the outer if guarding restore_pipe_data() so the control flow is unambiguous and resistant to accidental future edits.

	if (info->restore_data && drop_writer_early)
		if (restore_pipe_data(CR_FD_FIFO_DATA, new_fifo, info->fe->pipe_id, pd_hash_fifo)) {
			close(new_fifo);
			new_fifo = -1;
		}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new wait_writer detection/metadata has a couple of correctness/semantic issues (notably POLLERR handling and documenting/recording wait_writer for blocking readers) that should be fixed to avoid misclassification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

criu/fifo.c:101

• dump_one_fifo() sets fifo_entry.wait_writer for all O_RDONLY fds, but the surrounding comment describes the kernel behavior specifically for non-blocking readers. Since the restore-side logic only consults wait_writer for O_NONBLOCK anyway, limiting the field to O_RDONLY|O_NONBLOCK keeps the image semantics aligned with the documentation and avoids recording a potentially misleading value for blocking readers.

	if ((p->flags & O_ACCMODE) == O_RDONLY) {
		int wait_writer = fifo_wait_writer(lfd);

		if (wait_writer < 0)
			return -1;

		e.has_wait_writer = true;
		e.wait_writer = wait_writer;
	}

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite