Page MenuHomeFreeBSD

Capsicumize cpuset_*
ClosedPublic

Authored by allanjude on Mar 29 2017, 2:22 AM.

Details

Summary

bhyve was recently converted to the capsicum sandbox, and needs to be able to control the CPU sets of its vcpu threads

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude created this revision.Mar 29 2017, 2:22 AM
allanjude added inline comments.Mar 29 2017, 2:28 AM
sys/kern/kern_cpuset.c
339 ↗(On Diff #26746)

should this be allowed inside capsicum? check against IN_CAPABILITY_MODE(curthread) or something?

357 ↗(On Diff #26746)

same here

516 ↗(On Diff #26746)

allowed?

694 ↗(On Diff #26746)

only if id = -1?

995 ↗(On Diff #26746)

eventually this might allow setting on a thread, should the if consider that, so that if the following condition is changed/removed, it doesn't block things?

cem added a subscriber: cem.Mar 29 2017, 3:54 AM
cem added inline comments.
sys/kern/kern_cpuset.c
50–51 ↗(On Diff #26746)

These seem sorted wrong

339 ↗(On Diff #26746)

Why not allow it? It's just a helper and can only restrict the set(s) further. I don't think a check is needed here.

516 ↗(On Diff #26746)

Only if pid == -1, right?

694 ↗(On Diff #26746)

Right.

995 ↗(On Diff #26746)

Yeah, why not follow the same pattern you've used below?

emaste edited edge metadata.Mar 29 2017, 11:43 PM

Oops, forgot to hit "Submit," so sorry if some of these comments are OBE.

sys/kern/kern_cpuset.c
50 ↗(On Diff #26746)

Hmm, this file seems to be mostly unsorted already :( I suppose supporting with another 'c' header is as good as anywhere.

339 ↗(On Diff #26746)

Yep, this helper doesn't need to be aware of capsicum.

995 ↗(On Diff #26746)

These CPU sets refer to a global namespace; these should not be permitted in capability mode at all I think?

1108–1112 ↗(On Diff #26746)

This one looks good to me

1218 ↗(On Diff #26746)

I think that's out of scope for Capsicum.

allanjude updated this revision to Diff 26798.Mar 30 2017, 12:36 AM

Update based on feedback

sys/kern/kern_cpuset.c
995 ↗(On Diff #26746)

the setid being a global thing? I suppose that makes sense.

allanjude marked 10 inline comments as done.Mar 30 2017, 12:37 AM
allanjude updated this revision to Diff 26871.Mar 31 2017, 2:09 PM
allanjude removed a reviewer: bkidney_briankidney.ca.

Address feedback from capsicum call

emaste accepted this revision.Mar 31 2017, 3:43 PM
emaste added inline comments.
sys/kern/init_sysent.c
532–536 ↗(On Diff #26871)

Presumably this was just not regenerated

sys/kern/kern_cpuset.c
527 ↗(On Diff #26871)

leftover blank line from refactoring?

This revision is now accepted and ready to land.Mar 31 2017, 3:43 PM
oshogbo accepted this revision.Apr 4 2017, 2:33 PM
rwatson edited edge metadata.Apr 4 2017, 3:21 PM

I like the overall approach especially after various changes to do the checks only in the system calls themselves, not in the common helper functions.

sys/kern/capabilities.conf
137 ↗(On Diff #26871)

This comment will need updating.

sys/kern/kern_cpuset.c
1106 ↗(On Diff #26871)

I find this check pretty hard to read and understand. Is there some way to refactor it to use a nested if in an outside block conditioned in IN_CAPABILITY_MODE() and possibly a check on level to make it more readable?

1214 ↗(On Diff #26871)

Ditto here. I find the complex boolean expression hard to follow, and hence convince myself it is right. Can this become a nested if..?

emaste added inline comments.Apr 4 2017, 3:31 PM
sys/kern/kern_cpuset.c
1106 ↗(On Diff #26871)

So something like:

if (IN_CAPABILITY_MODE(td)) {
        if (level ...
                return (ECAPMODE);
}

Then do you think the inner if is more clear as
if (!(level == CPU_LEVEL_WHICH && (which == CPU_WHICH_TID || which == CPU_WHICH_PID) && id == -1))
or
if (level != CPU_LEVEL_WHICH || (which != CPU_WHICH_TID && which != CPU_WHICH_PID) || id != -1)

emaste added inline comments.Apr 4 2017, 3:34 PM
sys/kern/kern_cpuset.c
1106 ↗(On Diff #26871)

or even something like

if (level != CPU_LEVEL_WHICH)
        return (ECAPMODE);
if (which != CPU_WHICH_TID && which != CPU_WHICH_PID)
        return (ECAPMODE);
if (id != -1)
        return (ECAPMODE);
rwatson added inline comments.Apr 4 2017, 3:45 PM
sys/kern/kern_cpuset.c
1106 ↗(On Diff #26871)

Yes, this sounds good (one big outside check for IN_CAPABILITY_MODE(td), and then the sequence of if() checks). It's now much more obvious to me what the policy is.

allanjude updated this revision to Diff 27502.Apr 18 2017, 3:22 AM
allanjude edited edge metadata.

Update with feedback from rwatson

This revision now requires review to proceed.Apr 18 2017, 3:22 AM
rwatson added a subscriber: kib.Apr 18 2017, 7:19 AM
rwatson added inline comments.
sys/kern/capabilities.conf
136 ↗(On Diff #27502)

Possibly should be "caller's" or "callers'" (depending on whether you intend the plural or not).

sys/kern/init_sysent.c
535 ↗(On Diff #27502)

Do be sure to commit generated system-call files in a seperate commit (as @kib has rightly reminded me regarding recent commits).

FYI, it may be desirable to add a note about scoping of cpuset*(2) to the capsicum(4) man page. We should probably extend that man page in other ways to describe other sorts of scoping in place, but that's a separate task...

kib added a comment.Apr 18 2017, 11:46 AM

compat32 capabilities.conf requires the same changes, and corresponding tables must be regenerated as well, in the follow-up commit.

FYI, it may be desirable to add a note about scoping of cpuset*(2) to the capsicum(4) man page. We should probably extend that man page in other ways to describe other sorts of scoping in place, but that's a separate task...

Ah, yes, and also add ECAPMODE to .Sh ERRORS.

emaste accepted this revision.Apr 29 2017, 2:06 PM

Looks good (with the recent comments by kib and rwatson addressed)

This revision is now accepted and ready to land.Apr 29 2017, 2:06 PM
grehan added a subscriber: grehan.May 18 2017, 2:33 AM

Any chance this could be committed ?

Any chance this could be committed ?

Yes, sorry, I just had not had time to write the man page changes for it

allanjude updated this revision to Diff 28499.May 18 2017, 4:21 AM
allanjude edited edge metadata.

Add man page changes

This revision now requires review to proceed.May 18 2017, 4:21 AM

The two 'sysent' files should be done as a second commit, is that all that needs to be done?

kib added a comment.May 18 2017, 8:38 AM

The two 'sysent' files should be done as a second commit, is that all that needs to be done?

Yes, generated files should be committed separately.

wblock added a subscriber: wblock.May 19 2017, 3:36 PM
wblock added inline comments.
lib/libc/sys/cpuset_getaffinity.2
152 ↗(On Diff #28499)

Probably better to say "is only allowed to" instead of "may".

155 ↗(On Diff #28499)

This needs to be added to the SEE ALSO section, too.

emaste accepted this revision.May 19 2017, 3:45 PM

LGTM in general. Minor comment on the man page wording.

lib/libc/sys/cpuset_getaffinity.2
152 ↗(On Diff #28499)

For consistency with typical error descriptions, should it not be something like "The calling process attempted to act on a process other than itself, in capability mode."? It seems a bit tortured, but the error descriptions always seem to be "this is what the caller did wrong."

This revision is now accepted and ready to land.May 19 2017, 3:45 PM
This revision was automatically updated to reflect the committed changes.
wblock added inline comments.Jul 14 2017, 3:17 PM
head/lib/libc/sys/cpuset_getaffinity.2
158

This is in the wrong place. This list is sorted by category first, then alpha.