If the MAC policies must be able to hide jails, then this case should be completely indistinguishable from the "the jail doesn't exist" one, which means returning EINVAL and the exact same error message, which varies depending on the code point.

That's 4 new different calls to mac_prison_check_get(). Could you please try to move these out into a single occurrence after the found_prison label, along with the test on !(prison_isalive(pr) || (flags & JAIL_DYING)) (because that one must be after it)? That would make the latter redundant in the jailid case, but that's really nothing compared to the gain in clarity. You would also have to introduce some variable to hold the error message corresponding to the "the jail doesn't exist" case, since it's the one to be returned when mac_prison_check_get() returns some special error value indicating hiding (EINVAL?).

All hooks are using _NOSLEEP() variants, except for mac_prison_created and mac_prison_attached. Starting with something restrictive is fine (if sufficient for your use cases). In some cases, only the allprison_lock sx is held, so the corresponding hooks could sleep (although not necessarily recommended with sx). In any case, it's reasonable to keep mac_prison_check_*() checks as non-sleepable. That mac_prison_created and mac_prison_attached can sleep is fine with the current jail code (the former is called only with allprison_lock, the second without locks).

"Needs changes" set for the missing unlocks, see inline comments.

Ping

In D53954#1238908, @kevans wrote:

Ping

Sorry, was ill the past days and couldn't get back to it. Will do soon.

Coming back to highlights, there's indeed still the problem that enumeration returns an error if the last jail is denied via mac_prison_check_get(), so doing what you suggest is probably mandatory: Put back the MAC check in the loop, and once a jail is returned, jump to a prison_found_nomac label, even a _nomac_noalive variant as to eliminate also the (harmless) redundant test.

Additionally, as mentioned before, there's that, if mac_prison_check_get() denies access, we return a VFS error saying "access denied by MAC" which would be an info leak for situations where the jail must remain hidden. E.g., we could have mac_prison_check_get() return EPERM for a visible deny and ENOENT for an invisible one, and then treat them differently with respect to vfs_opterror(). But it seems that goes farther than what you're envisioning at this point (I haven't checked further revisions in the series thoroughly yet, to see if they need something like this).

In D53954#1239654, @olce wrote:

Coming back to highlights, there's indeed still the problem that enumeration returns an error if the last jail is denied via mac_prison_check_get(), so doing what you suggest is probably mandatory: Put back the MAC check in the loop, and once a jail is returned, jump to a prison_found_nomac label, even a _nomac_noalive variant as to eliminate also the (harmless) redundant test.

Additionally, as mentioned before, there's that, if mac_prison_check_get() denies access, we return a VFS error saying "access denied by MAC" which would be an info leak for situations where the jail must remain hidden. E.g., we could have mac_prison_check_get() return EPERM for a visible deny and ENOENT for an invisible one, and then treat them differently with respect to vfs_opterror(). But it seems that goes farther than what you're envisioning at this point (I haven't checked further revisions in the series thoroughly yet, to see if they need something like this).

I'm not too worried about that, though it does occur to me that it probably makes sense to stop setting an error in the opts for any of those checks. The MAC policy should probably have free reign over both errno and error probably in case it's some kind of custom vendor integration.

In D53954#1239668, @kevans wrote:

I'm not too worried about that, though it does occur to me that it probably makes sense to stop setting an error in the opts for any of those checks. The MAC policy should probably have free reign over both errno and error probably in case it's some kind of custom vendor integration.

I agree, we should probably stop setting the err opt. But that doesn't solve the discoverability of whether MAC (and not, e.g., real nonexistence of a jail) was the reason some access was denied, as in non-MAC we still set "errmsg" in the VFS options.

Giving this control to MAC modules would be nice; that requires obviously changing the interface of the hooks (if you do it, please don't pass struct vfsoptlist all around, but instead some function pointer to a printf-like function, so that we can clean up the use of struct vfsoptlist in jails at some point).

In D53954#1239691, @olce wrote:

In D53954#1239668, @kevans wrote:

I'm not too worried about that, though it does occur to me that it probably makes sense to stop setting an error in the opts for any of those checks. The MAC policy should probably have free reign over both errno and error probably in case it's some kind of custom vendor integration.

I agree, we should probably stop setting the err opt. But that doesn't solve the discoverability of whether MAC (and not, e.g., real nonexistence of a jail) was the reason some access was denied, as in non-MAC we still set "errmsg" in the VFS options.

Giving this control to MAC modules would be nice; that requires obviously changing the interface of the hooks (if you do it, please don't pass struct vfsoptlist all around, but instead some function pointer to a printf-like function, so that we can clean up the use of struct vfsoptlist in jails at some point).

I'm not sure I understand this last bit- I'm already passing the struct vfsoptlist all around so that MAC modules can reject jail_[sg]et operations based on the parameters they want to fetch or set. Are you implicitly suggesting that I should proactively give them a callback instead to work with the option list instead? I wasn't planning on limiting what they can do with the option list at all (e.g., if they wanted to override a parameter that'is to be set), so I guess I'd pass around an opaque pointer instead and provide sme KBI to fetch/set jail opts (that just defer to the vfsoptlist equivalents for now).

In D53954#1239729, @kevans wrote:

I'm not sure I understand this last bit

Oh... It's just a pilot error on my part, sorry!

I'm already passing the struct vfsoptlist all around so that MAC modules can reject jail_[sg]et operations based on the parameters they want to fetch or set.

Don't know how I managed to forget that for a while.

I just wanted to limit propagation of struct vfsoptlist elsewhere, but passing it makes sense for some hooks, so nevermind.

In D53954#1239739, @olce wrote:

In D53954#1239729, @kevans wrote:

I'm not sure I understand this last bit

Oh... It's just a pilot error on my part, sorry!

I'm already passing the struct vfsoptlist all around so that MAC modules can reject jail_[sg]et operations based on the parameters they want to fetch or set.

Don't know how I managed to forget that for a while.

I just wanted to limit propagation of struct vfsoptlist elsewhere, but passing it makes sense for some hooks, so nevermind.

Ok, thanks- to be clear, I'm still not against the idea of providing an abstraction layer to avoid some future headache for MAC modules when the core implementation changes. Did you have some sketch of what you're thinking in the area, by chance?