Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vmm: Avoid zombie sigwinch_listener processes #6208

Merged
merged 1 commit into from
Feb 19, 2024
Merged

vmm: Avoid zombie sigwinch_listener processes #6208

merged 1 commit into from
Feb 19, 2024

Conversation

arachsys
Copy link
Contributor

When a guest running on a terminal reboots, the sigwinch_listener subprocess exits and a new one restarts. The parent never wait()s for children, so the old subprocess remains as a zombie. With further reboots, more and more zombies build up.

As there are no other children whose exit status we want, the easiest fix is to take advantage of the implicit reaping specified by POSIX when we set the disposition of SIGCHLD to SIG_IGN.

For this to work, we also need to set the correct default exit signal of SIGCHLD when using clone3() CLONE_CLEAR_SIGHAND. Unlike the fallback fork() path, clone_args::default() initialises the exit signal to zero, which results in a child with non-standard reaping behaviour.

@arachsys arachsys requested a review from a team as a code owner February 18, 2024 16:54
@rbradford
Copy link
Member

Thank you for contribution. Please look at the commit messages for this file for the correct format of commit message.

@arachsys
Copy link
Contributor Author

Thank you for contribution. Please look at the commit messages for this file for the correct format of commit message.

Ah sorry, I see you have subsystem: prefixes. I've added vmm: to match the other commits affecting vmm/src/sigwinch_listener.rs.

(There doesn't seem to be any consistency about whether the summary line following the subsystem prefix has an initial capital or not, but capitalised seems to be slightly more prevalent so I went with that.)

@arachsys arachsys changed the title Avoid zombie sigwinch_listener processes vmm: Avoid zombie sigwinch_listener processes Feb 19, 2024
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyssais You're the expert on this file - can you give a review?

Copy link
Contributor

@alyssais alyssais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good find and a very elegant fix — I didn't know that ignoring SIGCHLD provides automatic reaping! I hadn't considered that the SIGWINCH listener would ever exit without Cloud Hypervisor as a whole exiting.

src/main.rs Outdated Show resolved Hide resolved
@arachsys
Copy link
Contributor Author

I didn't know that ignoring SIGCHLD provides automatic reaping!

It's quite a handy trick if you genuinely don't need the exit status of any children. /bin/init on a POSIX system can just consist of

signal(SIGCHLD, SIG_IGN);
pause();

which is fun. :)

When a guest running on a terminal reboots, the sigwinch_listener
subprocess exits and a new one restarts. The parent never wait()s
for children, so the old subprocess remains as a zombie. With further
reboots, more and more zombies build up.

As there are no other children for which we want the exit status,
the easiest fix is to take advantage of the implicit reaping specified
by POSIX when we set the disposition of SIGCHLD to SIG_IGN.

For this to work, we also need to set the correct default exit signal
of SIGCHLD when using clone3() CLONE_CLEAR_SIGHAND. Unlike the fallback
fork() path, clone_args::default() initialises the exit signal to zero,
which results in a child with non-standard reaping behaviour.

Signed-off-by: Chris Webb <chris@arachsys.com>
@rbradford rbradford merged commit 09f3658 into cloud-hypervisor:main Feb 19, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants