Page MenuHomeFreeBSD

bhyve: Use separate fds for stdin and stdout.
ClosedPublic

Authored by markj on Apr 2 2019, 1:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 18 2024, 7:55 PM
Unknown Object (File)
Sep 30 2024, 6:04 PM
Unknown Object (File)
Sep 30 2024, 4:37 PM
Unknown Object (File)
Sep 23 2024, 11:02 PM
Unknown Object (File)
Sep 23 2024, 7:52 PM
Unknown Object (File)
Sep 21 2024, 6:06 AM
Unknown Object (File)
Sep 21 2024, 5:51 AM
Unknown Object (File)
Sep 16 2024, 7:46 PM

Details

Summary

Without this change, bhyve's UART emulation uses the stdin fd for both
input and output. This makes it difficult to capture bhyve's output by
having it write to a pipe. This is helpful for example for Syzkaller,
which wants to be able to capture console output (so it can tell when a
VM has panicked and extract the panic string) but never needs to send
input to the console.

One solution to the above problem would be to use nmdm(4) instead, but
this turned out to be too complicated. You can't easily open the other
nmdm node and read from it as though it's a stream. Running a cu(1)
process and treating its standard output as a console stream kind of
works, but clean-up is hard to get right, partly because cu(1) uses a
child process to read from the modem device, and sending SIGINT to the
parent causes a hang. Meanwhile, other hypervisors (qemu, vmm, kvm)
have no problems running with redirected std* streams.

Note that bhyveload(8) already uses separate fds for input and output
when interacting with stdio, so no change was needed there.

I did not attempt to modify the bvmconsole driver (bhyve -b) since
it does not appear to be used much and the man page claims that it
will be deprecated.

Test Plan

I tested a FreeBSD guest with stdio and nmdm(4) consoles without seeing
any problems. I plan to try a couple of other guest OSs (OpenBSD and Linux)
before committing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Can we get a review to cause bhyve -b to emit a deprecation warning notice (gone_in(13)) if someone tries to use it? That could be merged to stable/12, then the code in head killed.
This would also apply to the bvmconsole driver, is there someway to emit a warning during config(8) when someone uses a feature being deprecated? Doing this provides us feedback
sooner rather than later that some feature was in use some place.

Can we get a review to cause bhyve -b to emit a deprecation warning notice (gone_in(13)) if someone tries to use it?

Sounds reasonable, but independent of this change...

jhb added inline comments.
usr.sbin/bhyve/bhyverun.c
1204 ↗(On Diff #55712)

So presumably this does break bvm_cons right now with capsicum (which is the default)? I guess we need unusual settings for stdout in the uart stdio case, so maybe consport.c needs to just call caph_limit_stdout directly?

usr.sbin/bhyve/uart_emul.c
643 ↗(On Diff #55712)

This is almost caph_limit_stream. :( The helper doesn't include TIOCSETA. It would be nice to avoid some of the code duplication this change adds, but I guess it can't be helped.

696 ↗(On Diff #55712)

I'm not sure why we need this. In particular, if you use two uarts (I commonly map COM1 to stdio and COM2 to a nmdm for use with kgdb), initializing COM2 will call caph_limit_stdio() after the uart console was setup possibly breaking it. I think you should just not call this at all and only let parts of bhyve that need stdin/stdout set it up.

usr.sbin/bhyve/bhyverun.c
1204 ↗(On Diff #55712)

I don't see why it would break it? The only thing we do with stdout here or in uart_emul.c is write to it, so caph_limit_stdout() is fine.

Rereading the diff, I can't remember why I moved the caph_limit_stdout() call to uart_emul.c.

usr.sbin/bhyve/uart_emul.c
643 ↗(On Diff #55712)

Yeah, I noticed that too. Adding TIOCSETA to the set of permitted ioctls in caph_limit_stream() might be reasonable, but if not I don't think we can simplify this code at all.

696 ↗(On Diff #55712)

Oops, yes.

Wasn't the old code broken when COM1 is nmdm and COM2 is stdio? We will execute the !uart_stdio block when configuring the nmdm UART and call caph_limit_stdin().

markj marked 2 inline comments as done.

Address feedback.

jhb added inline comments.
usr.sbin/bhyve/uart_emul.c
696 ↗(On Diff #55712)

Yes, I suspect that probably was broken.

This revision is now accepted and ready to land.Apr 20 2019, 4:50 PM
This revision was automatically updated to reflect the committed changes.