Page MenuHomeFreeBSD

Simplify capsicumization of bhyve.
ClosedPublic

Authored by araujo on Jan 4 2019, 12:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 22 2024, 1:54 PM
Unknown Object (File)
Feb 16 2024, 10:04 AM
Unknown Object (File)
Jan 28 2024, 6:38 AM
Unknown Object (File)
Jan 14 2024, 6:56 AM
Unknown Object (File)
Jan 2 2024, 6:51 AM
Unknown Object (File)
Dec 25 2023, 5:36 PM
Unknown Object (File)
Dec 20 2023, 4:34 AM
Unknown Object (File)
Dec 11 2023, 12:43 AM

Details

Summary

Use capsicum_helpers(3) that allow us to simplify the code and
its functions will return success when the kernel is built without
support of the capability mode.

NOTE: This is the first round of capsicum review, the next round would be to apply cap_fileargs(3).
Test Plan
  • Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"
  • Tested with guests: Linux Fedora 29 and FreeBSD HEAD.
  • Tested connection with VNC.
  • Tested connection with CONSOLE (nmdm).

Diff Detail

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

Event Timeline

araujo added reviewers: oshogbo, bhyve.

Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"

But all of the WITHOUT_CAPSICUM #ifdefs have been removed?

usr.sbin/bhyve/Makefile
78 ↗(On Diff #52549)

Is this part of the next change?

Smoke test built without CAPSICUM and CASPER. Ex.: "-DWITHOUT_CAPSICUM"

But all of the WITHOUT_CAPSICUM #ifdefs have been removed?

Most of it were remove with one single exception that is related with a header vmm_dev.h that I need to check if it is possible to merge with vmm.h.

usr.sbin/bhyve/Makefile
78 ↗(On Diff #52549)

Yes it is! Also as I'm working on review D12419, I thought would be OK to add cap_fileargs right now.

I stopped saying "same" after a while to avoid being too repetitious. I think it would make sense to have one commit that first fixes existing places not yet using helpers to use the helpers by replacing all uses of cap_rights_limit and cap_ioctls_limit with the helper variants. I would keep the == -1 checks though. The syscalls are defined to return -1 on error, not a random -ve value. From cap_rights_limit(2):

RETURN VALUES
     Upon successful completion, the value 0 is returned; otherwise the value -1
     is returned and the global variable errno is set to indicate the error.

You could instead perhaps fail for any return value that isn't 0 (e.g use '!= 0'), but using '< 0' is just kind of odd (we fail for half of the undefined values (negative ones), but succeed for the other half (positive values > 0). Using either '== -1' or '!= 0' is more consistent in that all undefined values are treated the same. Personally I would probably use != 0, but the existing code already uses == -1, so I would leave it as is to minimize the diff.

I would then probably do a second commit to remove WITHOUT_CAPSICUM and a third commit to add casper for the audio stuff.

usr.sbin/bhyve/bhyverun.c
942 ↗(On Diff #52549)

I don't think this comment adds much value so would probably leave it out.

944 ↗(On Diff #52549)

This needs to be caph_rights_limit instead of cap_rights_limit? Also, checking for == -1 as the existing code did is ok. (Syscalls are generally defined as returning exactly -1 on error)

950 ↗(On Diff #52549)

Similar, s/cap_/caph_/ and I would keep the == -1 to minimize the diff.

1198 ↗(On Diff #52549)

I think == -1 is fine and would just leave that as is. The only real change here is to remove the WITHOUT_CAPSICUM it seems.

usr.sbin/bhyve/block_if.c
471 ↗(On Diff #52549)

Similar comments, s/cap_/caph_/ and I would keep the == -1.

500 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/consport.c
136 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/dbgport.c
165 ↗(On Diff #52549)

Same.

usr.sbin/bhyve/gdb.c
164 ↗(On Diff #52549)

I would keep the == -1.

Following @jhb suggestion, I will break down these updates in chunks.

First of all, I will replace functions that are included on capsicum_helpers(3),
then after a while I will remove the #ifndef.

Second of all, I need to apologize, I have used devel/coccinelle to make this
code transformation and I wrote coccinelle's script wrong regarding the
replacement of s/cap_/caph_/. I won't do that again.

Thanks! Hopefully this means 'grep -c ENOSYS' in bhyve is now zero. :)

This revision is now accepted and ready to land.Jan 15 2019, 6:32 PM
In D18744#402454, @jhb wrote:

Thanks! Hopefully this means 'grep -c ENOSYS' in bhyve is now zero. :)

I was curious too:
[araujo@pipoca] /usr/src/usr.sbin/bhyve# grep -c ENOSYS .
0

:P

This revision was automatically updated to reflect the committed changes.