Page MenuHomeFreeBSD

bsdinstall: Fix race condition when shutting down after installation
ClosedPublic

Authored by jrtc27 on Oct 5 2022, 2:29 AM.
Tags
None
Referenced Files
F102461703: D36879.diff
Tue, Nov 12, 3:01 PM
Unknown Object (File)
Mon, Nov 11, 10:18 AM
Unknown Object (File)
Mon, Oct 21, 10:47 AM
Unknown Object (File)
Mon, Oct 21, 10:47 AM
Unknown Object (File)
Mon, Oct 21, 10:47 AM
Unknown Object (File)
Mon, Oct 21, 10:12 AM
Unknown Object (File)
Oct 7 2024, 12:03 AM
Unknown Object (File)
Sep 27 2024, 9:07 PM
Subscribers

Details

Summary

Whilst reboot(8) will block whilst it runs, shutdown(8) does not,
daemonizing instead. This means that we must wait after running it,
otherwise we will exit and cause the system to attempt to go multi-user
in parallel with the shutdown daemon killing init. With the new
multi-console support in the installer, runconsoles will immediately
kill this daemon, racing with the daemon being able to signal init as
desired, and I have seen this race be lost in QEMU with a single CPU. In
the past this wasn't such an issue, since shutdown's daemon puts itself
in a new session group immediately after fork (and the parent doesn't
wait until that has happened, so whilst there's technically a race
condition in there where it could receive a SIGHUP from the death of the
parent's session leader, in practice this is very unlikely to be hit.
This means that the only consequence of this oversight before was that
you might get the beginnings of more console output on the way to
multi-user and thus the console would look a little confusing.

Fixes: e4505364c087 ("release/rc.local: Provide option to shutdown after installation complete")
Fixes: a09af1b7fd95 ("bsdinstall release: Start installer on multiple consoles")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 47693
Build 44580: arc lint + arc unit

Event Timeline

jrtc27 requested review of this revision.Oct 5 2022, 2:29 AM
jrtc27 created this revision.
gjb added a subscriber: gjb.

Minor nit: please adjust the first line of the commit log message to fit within 80 characters. Otherwise, looks good to me.

This revision is now accepted and ready to land.Oct 5 2022, 12:29 PM

Interesting - I hadn’t noticed any issue during my previous testing - thanks for finding and fixing this

Interesting - I hadn’t noticed any issue during my previous testing - thanks for finding and fixing this

Yeah, I think in practice it was hard to notice before, but my patch made it a much bigger problem

In D36879#837343, @gjb wrote:

Minor nit: please adjust the first line of the commit log message to fit within 80 characters. Otherwise, looks good to me.

I measure it at 68 characters already for the subject, and 65 for the body? Not sure what you're referring to.

In D36879#837343, @gjb wrote:

Minor nit: please adjust the first line of the commit log message to fit within 80 characters. Otherwise, looks good to me.

I measure it at 68 characters already for the subject, and 65 for the body? Not sure what you're referring to.

Ignore me. I don't know what I was looking at.. Sorry for the noise.

In D36879#837817, @gjb wrote:
In D36879#837343, @gjb wrote:

Minor nit: please adjust the first line of the commit log message to fit within 80 characters. Otherwise, looks good to me.

I measure it at 68 characters already for the subject, and 65 for the body? Not sure what you're referring to.

Ignore me. I don't know what I was looking at.. Sorry for the noise.

No worries, just wanted to check before committing. Thanks for the quick review.